-
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-44384: [C++] Use CMAKE_LIBTOOL on macOS #44385
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
Could you fix a lint error by pre-commit run --show-diff-on-failure --color=always --all
?
When a builder sets `CMAKE_LIBTOOL`, use that as the program to bundle dependencies. This matches the behavior of the Windows build. Also make a nitpicky minor update to the error message when a non-Apple libtool is detected.
d00ff7f
to
f2b12a3
Compare
Updated the formatting and got myself set up with pre-commit. Thanks for the pointer! |
C GLib & Ruby / AMD64 Ubuntu 22.04 GLib & Ruby (pull_request) Failing after 10m
looks like a network outage maybe? R / Check minimum supported Arrow C++ Version (15.0.2) (pull_request) Failing after 25s
Not sure what that one is about. |
two other build failures also seem unrelated (they have the same error): C++ / AMD64 Conda C++ AVX2 (pull_request) Failing after 22sC++ / AMD64 Ubuntu 22.04 C++ ASAN UBSAN (pull_request)
|
The GLib failure is just a network error. It'll be fixed by re-run. Other failures are already fixed on main. |
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 4 benchmarking runs that have been run so far on merge-commit 25a2440. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 19 possible false positives for unstable benchmarks that are known to sometimes produce them. |
When a builder sets
CMAKE_LIBTOOL
, use that as the program to bundle dependencies. This matches the behavior of the Windows build.Also make a nitpicky minor update to the error message when a non-Apple libtool is detected.
Are these changes tested?
I did not add a test case for this build configuration. I confirmed the behavior when
CMAKE_LIBTOOL
is set, using this script on a Mac. It creates an ad-hoc libtool wrapper and then passes it asCMAKE_LIBTOOL
.I am not sure how to best add a test for this to the repository.
Are there any user-facing changes?
This changes how bundled builds behave when an Apple build-user sets
CMAKE_LIBTOOL
in their CMake invocation. I think the change is a sensible one and wouldn't be surprising to users, but it may break existing builds in environments where /usr/bin/libtool is Apple's libtool, andCMAKE_LIBTOOL
is set to a non-Apple one.