-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47830: [Release] Run RC verification source testing step in a subshell #47831
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
Conversation
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
Or we can restore DYLD_LIBRARY_PATH
/LD_LIBRARY_PATH
at the end of test_source_distribution
.
I prefer the subshell approach so we don't need to worry about which environment variables might have changed, but I'm happy to do it that way if you'd prefer. |
I don't have a strong opinion for it. We can merge this as-is or wait for comments from others in a few days before we merge this. |
👍 let's leave this for a few days in case someone else has comments |
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.
The comment is great! I am happy with this approach.
I was wondering why we haven't experienced this before and I just realized we never test sources and binaries on the same CI verification jobs (we always do it separately).
I also don't test them on the same pass locally, I usually test sources and afterwards binaries so it makes sense we haven't noticed!
Thanks for the fix!
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f6e48b4. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 23 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Isolates environment variable changes made when verifying the source from the tests that verify the binaries, to prevent these causing errors like in #47830.
What changes are included in this PR?
Changes
verify-release-candidate.sh
to runtest_source_distribution
in a subshell.Are these changes tested?
I've tested this manually, I'm not sure if there are any existing CI tests that run this script on PRs.
Are there any user-facing changes?
No