-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-4599: [C++] Add support for system GFlags #3675
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
ci/travis_before_script_cpp.sh
Outdated
| @@ -53,7 +53,8 @@ CMAKE_COMMON_FLAGS="\ | |||
| -DARROW_NO_DEPRECATED_API=ON \ | |||
| -DARROW_EXTRA_ERROR_CONTEXT=ON" | |||
| CMAKE_LINUX_FLAGS="" | |||
| CMAKE_OSX_FLAGS="" | |||
| CMAKE_OSX_FLAGS="\ | |||
| -DARROW_GFLAGS_USE_SHARED=OFF" | |||
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.
Because @rpath can't be resolved with GFlags installed by conda-forge.
https://travis-ci.org/kou/arrow/jobs/494364820#L1993
2/111 Test #1: plasma-serialization_tests ...................***Failed 1.36 sec Running plasma-serialization_tests, redirecting output into /Users/travis/build/kou/arrow/cpp-build/build/test-logs/plasma-serialization_tests.txt (attempt 1/1) dyld: Library not loaded: @rpath/libgflags.2.2.dylib Referenced from: /Users/travis/build/kou/arrow/cpp-install/lib/libarrow.13.dylib Reason: image not found
We will be able to resolve it by running each test with DYLD_LIBRARY_PATH environment variable:
DYLD_LIBRARY_PATH=${MINICONDA}/lib ${ARROW_BUILD_TYPE}/plasma-serialization_testsBecause DYLD_LIBRARY_PATH can affect only the child process.
It's not useful. So I use static library than shared library.
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.
This is working for all other libraries? Then there must be something wrong with libgflags.
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.
Other libraries are used as static library. So they work.
gflags-config.cmake provides @rpath/libgflags.2.2.dylib for IMPORTED_LOCATION. If gflags-config.cmake doesn't use @rpath, this will not be caused.
Generally, using @rpath isn't bad. GFlags installed from conda-forge can't assume where it's installed. So I think that using @rpath is reasonable for this case.
We will be able to resolve @rpath in our FindGFlags.cmake. This is another solution.
We can specify DYLD_LIBRARY_PATH to resolve @rpath in cpp/build-support/run-test.sh. This is also another solution.
|
A job on AppVeyor is failed:
Using vendored version for this case is expected. Build type may be conflicted. I need to look into this. |
884a418 to
7ce098c
Compare
ci/appveyor-cpp-build.bat
Outdated
| @@ -44,6 +44,7 @@ if "%JOB%" == "Static_Crt_Build" ( | |||
| cmake --build . --config Debug || exit /B | |||
| ctest --output-on-failure -j2 || exit /B | |||
| popd | |||
| rmdir /S /Q cpp\build-debug | |||
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.
Because find_package(gflags CONFIG) can find GFlags in cpp\build-debug\gflags_ep-prefix\.
|
CI is passed. |
If old build directory exists, CMake finds <package>-config.cmake from old build directory. It's incompatible with new configuration.
Change-Id: Ied430fc43304f26e7d8490fbb3818ad4e9ceb510
7ce098c to
38f910f
Compare
wesm
left a comment
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. I rebased, will merge once the build passes
|
CI is green. I'll merge this. |
This is needed to build tests with MinGW.