Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-40248: [R] fallback to the correct libtool when we find a GNU one #40259

Merged
merged 11 commits into from
Feb 28, 2024
Merged

GH-40248: [R] fallback to the correct libtool when we find a GNU one #40259

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
50a64a7
brew install libtool
jonkeane Feb 26, 2024
77188a0
remove an errant echo
jonkeane Feb 27, 2024
87fa3b7
Clean up / use find_program
jonkeane Feb 27, 2024
19461ca
Push a known-bad setup to fail
jonkeane Feb 27, 2024
a62b889
Add the search path
jonkeane Feb 27, 2024
bbd9959
Update cpp/cmake_modules/BuildUtils.cmake
jonkeane Feb 27, 2024
150deb0
Update dev/tasks/r/github.macos-linux.local.yml
jonkeane Feb 27, 2024
e1cac51
Use only find_program + hints
jonkeane Feb 28, 2024
ee8e590
Only muck with the path on macOS
jonkeane Feb 28, 2024
a98893a
lint
jonkeane Feb 28, 2024
97bd2aa
Merge branch 'gnulibtool' of github.com:jonkeane/arrow into gnulibtool
jonkeane Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 24 additions & 1 deletion cpp/cmake_modules/BuildUtils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,30 @@ function(arrow_create_merged_static_lib output_target)
endforeach()

if(APPLE)
set(BUNDLE_COMMAND "libtool" "-no_warning_for_no_symbols" "-static" "-o"
# The apple-distributed libtool is what we want for bundling, but there is
# a GNU libtool that has a namecollision (and happens to be bundled with R, too).
# We are not compatible with GNU libtool, so we need to avoid it.
# TODO: use a VALIDATOR when we require cmake >= 3.25

# check in the obvious places first to find Apple's libtool
find_program(LIBTOOL_MACOS libtool
PATHS /usr/bin /Library/Developer/CommandLineTools/usr/bin
NO_DEFAULT_PATH)

# if it's not found in obvious places, check on the standard path
if(LIBTOOL_MACOS-NOTFOUND)
set(LIBTOOL_MACOS "libtool")
endif()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this could technically be find_program(LIBTOOL_MACOS libtool) and that would find libtool along the search path, but be a no-op if line 106 above it found libtool.

Does that look better? Or just an obfuscated version of this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just using default paths? $PATH is used after PATHS.

Suggested change
PATHS /usr/bin /Library/Developer/CommandLineTools/usr/bin
NO_DEFAULT_PATH)
# if it's not found in obvious places, check on the standard path
if(LIBTOOL_MACOS-NOTFOUND)
set(LIBTOOL_MACOS "libtool")
endif()
PATHS /usr/bin /Library/Developer/CommandLineTools/usr/bin)

https://cmake.org/cmake/help/latest/command/find_program.html

  1. Search the paths specified by the HINTS option. These should be paths computed by system introspection, such as a hint provided by the location of another item already found. Hard-coded guesses should be specified with the PATHS option.

  2. Search the standard system environment variables. This can be skipped if NO_SYSTEM_ENVIRONMENT_PATH is passed or by setting the CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH to FALSE.

    • The directories in PATH itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try this. I will admit I read that list in the find docs a few times and was not 100% certain it would follow the path I was hoping it would (use our provided paths first, then project paths, then system paths).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this works though I had to switch from PATHS to HINTS since HINTS is higher in the search. This is a bit funny given the docs:

  1. Search the paths specified by the HINTS option. These should be paths computed by system introspection, such as a hint provided by the location of another item already found. Hard-coded guesses should be specified with the PATHS option.

But it does seem to work!


# confirm that the libtool we found is not GNU libtool
execute_process(COMMAND ${LIBTOOL_MACOS} -V
OUTPUT_VARIABLE LIBTOOL_V_OUTPUT
OUTPUT_STRIP_TRAILING_WHITESPACE)
if(NOT "${LIBTOOL_V_OUTPUT}" MATCHES ".*cctools-([0-9.]+).*")
message(FATAL_ERROR "libtool found appears to be the incompatible GNU libtool")
jonkeane marked this conversation as resolved.
Show resolved Hide resolved
endif()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider raising this as a warning and proceeding anyway. If it is GNU libtool, it fails pretty clearly next. That would insulate us incase Apple's libtool has a slightly different version string or something (although with a spurious error). I'm split on if this is better or not.


set(BUNDLE_COMMAND ${LIBTOOL_MACOS} "-no_warning_for_no_symbols" "-static" "-o"
${output_lib_path} ${all_library_paths})
elseif(CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|GNU|Intel|IntelLLVM)$")
set(ar_script_path ${CMAKE_BINARY_DIR}/${ARG_NAME}.ar)
Expand Down
5 changes: 5 additions & 0 deletions dev/tasks/r/github.macos-linux.local.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ jobs:
- uses: r-lib/actions/setup-r@v2
with:
use-public-rspm: true
- name: Add R.framework/Resources/bin to the path
run: |
# CRAN builders have the entire bin here added to the path. This sometimes
# includes things like GNU libtool which name-collide with what we expect
echo "/Library/Frameworks/R.framework/Resources/bin" >> $GITHUB_PATH
jonkeane marked this conversation as resolved.
Show resolved Hide resolved
- name: Install dependencies
uses: r-lib/actions/setup-r-dependencies@v2
with:
Expand Down