-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-43284: [Release] Fix version detection timing for bump deb package names on post-12-bump-versions.sh script #43294
Conversation
|
I am not sure why
cc @kou |
dev/release/post-12-bump-versions.sh
Outdated
@@ -64,7 +64,7 @@ if [ ${BUMP_VERSION_POST_TAG} -gt 0 ]; then | |||
fi | |||
|
|||
if [ ${BUMP_DEB_PACKAGE_NAMES} -gt 0 ] && \ | |||
[ "${next_version}" != "$(current_version)" ]; then | |||
[ "${next_version}" != "${version}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.
$(current_version)
is a function call not a variable reference.
So current_version
function call is intentional here.
This is happen because there are no files that are matched against The pattern at arrow/dev/release/utils-prepare.sh Line 238 in 4161898
git mv . So arrow/dev/release/utils-prepare.sh Line 231 in 4161898
|
Could you share the command line you tried? |
Ah, we need to run diff --git a/dev/release/post-12-bump-versions.sh b/dev/release/post-12-bump-versions.sh
index 422821a66b..206da6ccb4 100755
--- a/dev/release/post-12-bump-versions.sh
+++ b/dev/release/post-12-bump-versions.sh
@@ -41,6 +41,8 @@ version=$1
next_version=$2
next_version_snapshot="${next_version}-SNAPSHOT"
+current_version_before_bump="$(current_version)"
+
case "${version}" in
*.0.0)
is_major_release=1
@@ -64,7 +66,7 @@ if [ ${BUMP_VERSION_POST_TAG} -gt 0 ]; then
fi
if [ ${BUMP_DEB_PACKAGE_NAMES} -gt 0 ] && \
- [ "${next_version}" != "$(current_version)" ]; then
+ [ "${next_version}" != "${current_version_before_bump}" ]; then
update_deb_package_names "${version}" "${next_version}"
fi
|
I was just trying what the test was doing on the branch:
I also added the diff --git a/dev/release/post-12-bump-versions.sh b/dev/release/post-12-bump-versions.sh
index 2eeb715..5ffc7d3 100755
--- a/dev/release/post-12-bump-versions.sh
+++ b/dev/release/post-12-bump-versions.sh
@@ -17,7 +17,7 @@
# specific language governing permissions and limitations
# under the License.
#
-set -ue
+set -uex
SOURCE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
source "${SOURCE_DIR}/git-vars.sh" |
(I'm writing a test for this case...) |
Could you apply this? diff --git a/dev/release/post-12-bump-versions-test.rb b/dev/release/post-12-bump-versions-test.rb
index 2bd1458746..f31e1a3122 100644
--- a/dev/release/post-12-bump-versions-test.rb
+++ b/dev/release/post-12-bump-versions-test.rb
@@ -358,8 +358,15 @@ class PostBumpVersionsTest < Test::Unit::TestCase
def test_deb_package_names
omit_on_release_branch unless bump_type.nil?
current_commit = git_current_commit
- stdout = bump_versions("DEB_PACKAGE_NAMES")
- changes = parse_patch(git("log", "-p", "#{current_commit}.."))
+ stdout = bump_versions("VERSION_POST_TAG", "DEB_PACKAGE_NAMES")
+ log = git("log", "-p", "#{current_commit}..")
+ # Remove a commit for VERSION_POST_TAG
+ if log.scan(/^commit/).size == 1
+ log = ""
+ else
+ log.gsub!(/\A(commit.*?)^commit .*\z/um, "\\1")
+ end
+ changes = parse_patch(log)
sampled_changes = changes.collect do |change|
first_hunk = change[:hunks][0]
first_removed_line = first_hunk.find { |line| line.start_with?("-") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit fe51029. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
Rationale for this change
The script fails to bump deb package names at the moment due to an unmatching if.
What changes are included in this PR?
Use current_version before bumping the versions in order to match debian packages to be updated.
Are these changes tested?
It has been tested locally to bump the versions for the debian package names on main for 17.0.0
Are there any user-facing changes?
No