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.

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

Arrow headers installed to /usr/local can break perspective's subdirectory Arrow build #2792

Open
texodus opened this issue Oct 15, 2024 Discussed in #2791 · 2 comments · May be fixed by #2793
Open

Arrow headers installed to /usr/local can break perspective's subdirectory Arrow build #2792

texodus opened this issue Oct 15, 2024 Discussed in #2791 · 2 comments · May be fixed by #2793
Labels
bug Concrete, reproducible bugs

Comments

@texodus
Copy link
Member

texodus commented Oct 15, 2024

Discussed in #2791

Originally posted by tomjakubowski October 15, 2024

Bug Report

Steps to Reproduce:

  1. Build and install Apache Arrow to /usr/local
  2. Try and build perspective, with its own included Arrow dependency

Expected Result:

Perspective builds Arrow with its own set of options (defined in FindInstallDependency.cmake)

Actual Result:

Flags from the global installation, defined in /usr/local/include/arrow/util/config.h, will interfere with perspective's Arrow build.

In my case, that global config file had #define ARROW_JEMALLOC, while perspective's build had (set ARROW_JEMALLOC OFF) in cmake, which are inconsistent flags and so Arrow was miscompiled with some missing symbols referencing their Jemalloc memory pool implementation.

nm -mg /Users/tom/perspective/perspective/rust/perspective-python/perspective/perspective.abi3.so | grep malloc

                 (undefined) external __ZN5arrow11memory_pool8internal17JemallocAllocator13ReleaseUnusedEv (dynamically looked up)
                 (undefined) external __ZN5arrow11memory_pool8internal17JemallocAllocator15AllocateAlignedExxPPh (dynamically looked up)
                 (undefined) external __ZN5arrow11memory_pool8internal17JemallocAllocator17DeallocateAlignedEPhxx (dynamically looked up)
                 (undefined) external __ZN5arrow11memory_pool8internal17JemallocAllocator17ReallocateAlignedExxxPPh (dynamically looked up)

Found this by running a make VERBOSE=1 from the arrow-build directory (at rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-build/). Logs below. Notice that the -I for /usr/local/include comes before the -I for arrow's own source file.

cd
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-build/src/arrow
&& /usr/bin/c++ -DARROW_WITH_TIMING_TESTS
-I/Users/tom/perspective/perspective/rust/perspective-server/cpp/perspective/src/include
-I/usr/local/include
-I/Users/tom/perspective/perspective/rust/perspective-server/cpp/perspective/../../python/perspective/perspective/perspective/include
-I/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-src/cpp/src
-I/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-src/cpp/src/generated
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/date-src/extras/date/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/date-src/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/date-src
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/hopscotch-src/extras/hopscotch/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/hopscotch-src/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/hopscotch-src
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/ordered-map-src/extras/ordered-map/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/ordered-map-src/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/ordered-map-src
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/rapidjson-src/extras/rapidjson/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/rapidjson-src/include
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/rapidjson-src
-isystem
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-build/src
-fno-aligned-new  -O3  -Qunused-arguments -fcolor-diagnostics  -Wall
-Wno-unknown-warning-option -Wno-pass-failed  -O3 -DNDEBUG -O2  -std=c++17
-arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.3.sdk
-fPIC -MD -MT
arrow-build/src/arrow/CMakeFiles/arrow_memory_pool.dir/memory_pool.cc.o -MF
CMakeFiles/arrow_memory_pool.dir/memory_pool.cc.o.d -o
CMakeFiles/arrow_memory_pool.dir/memory_pool.cc.o -c
/Users/tom/perspective/perspective/rust/target/release/build/perspective-server-bb669fae7f7ee29c/out/build/arrow-src/cpp/src/arrow/memory_pool.cc

Environment:

macOS 13.3.1 perspective master, 13b26d7

Additional Context:

Here is the #include which was picking up from /usr/local instead of the build directory.

This seems like a potentially common hazard/footgun for people building from source, who may have an installed Arrow elsewhere on their system. I do not know if a libarrow installed to /usr (as might be done by a Linux distro package manager) causes the same problem but would be worth investigating.

@texodus texodus added the bug Concrete, reproducible bugs label Oct 15, 2024
@tomjakubowski
Copy link
Contributor

tomjakubowski commented Oct 15, 2024

What I'm working on to fix this is to remove all calls to include_directories and instead to collect them into a single target_include_directories call on just the psp target. This will help us better control the order of include paths when building perspective. Importantly, we want Boost_INCLUDE_DIRS to be last in the list, because it's likely to be in a system directory where other dependencies with headers, like re2 or arrow, might be installed.

It's possible I'll need to back some of those out in order to get dependencies to build, but I hope not!

@tomjakubowski
Copy link
Contributor

It would be even better not to depend on find_package(Boost), and add Boost as a psp_build_dep(), which should be easier now that we've discovered in the Conda work that the build only requires header-only Boost.

But I'd like to punt on that for future work and instead do the cruder thing first with target_include_directories. Boost would be a nice first step at working out how we can support either find_package() or psp_build_dep() for each of our dependencies. This will help us further down the road of making the Conda build fully dynamically link its dependencies.

tomjakubowski added a commit to tomjakubowski/perspective that referenced this issue Oct 15, 2024
`psp_build_dep(name)` now exports in its parent scope a
`${name}_INCLUDE_DIRS` variable which contains a list of include
directories containing that dependency's header files.

These variables are then joined into a list in CMakeLists.txt, which is
then passed to `target_include_directories(psp)`

There are no longer any calls to `include_directories()`.  Some extra
include paths were also removed, like `/usr/local/include` for Boost,
which is better covered by `Boost_INCLUDE_DIRS`.

Also removes `boost_system` from the list of Boost requirements; we only
need Boost headers.

closes finos#2792
tomjakubowski added a commit to tomjakubowski/perspective that referenced this issue Oct 15, 2024
`psp_build_dep(name)` now exports in its parent scope a
`${name}_INCLUDE_DIRS` variable which contains a list of include
directories containing that dependency's header files.

These variables are then joined into a list in CMakeLists.txt, which is
then passed to `target_include_directories(psp)`

There are no longer any calls to `include_directories()`.  Some extra
include paths were also removed, like `/usr/local/include` for Boost,
which is better covered by `Boost_INCLUDE_DIRS`.

Also removes `boost_system` from the list of Boost requirements; we only
need Boost headers.

closes finos#2792

Signed-off-by: Tom Jakubowski <[email protected]>
@tomjakubowski tomjakubowski linked a pull request Oct 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants