apache / incubator-gluten

Gluten is a middle layer responsible for offloading JVM-based SQL engines' execution to native engines.
https://gluten.apache.org/
Apache License 2.0
1.22k stars 439 forks source link

[VL] Do not use --version-script link option on Darwin #7820

Closed PHILO-HE closed 3 weeks ago

PHILO-HE commented 3 weeks ago

What changes were proposed in this pull request?

This is a follow-up pr for this comment.

If not hidden, the below error can be reported when running gluten CPP test.

something wrong with flag 'velox_memory_num_shared_leaf_pools' in file '/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/velox/flag_definitions/flags.cpp'.  One possibility: file '/__w/incubator-gluten/incubator-gluten/ep/build-velox/build/velox_ep/velox/flag_definitions/flags.cpp' is being linked both statically and dynamically into this executable.
CMake Error at /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/GoogleTestAddTests.cmake:112 (message):
Error running test executable.

  Path: '/__w/incubator-gluten/incubator-gluten/cpp/build/velox/tests/velox_shuffle_writer_test'

velox_memory_num_shared_leaf_pools is declared in Velox under google namespace. See DEFINE_int32 used by this velox code. Even though local: *; is defined for version script, we have to explicitly make all symbols under *google::* namespace hidden. The possible reason is that the specified global *facebook::velox::* symbols cover those *google::* symbols.

How was this patch tested?

CI.

github-actions[bot] commented 3 weeks ago

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

PHILO-HE commented 3 weeks ago

@zml1206, could you take a look? This is a follow-up for a comment in your merged pr.

PHILO-HE commented 3 weeks ago

Just found some other issue. Let's hold on.

PHILO-HE commented 3 weeks ago

This pr once intended to use one same symbols.map set for version-script in both static build and dynamic build. But it seems we cannot achieve this. For static build (with vcpkg), the below symbol patterns need to be put inside extern "C++" block. But for dynamic build, they should be moved out from this block. Otherwise, symbol conflict issue will be reported when running gluten cpp tests. One possible root cause is the use of different linking tool or linking options.

local:
    extern "C++" {
      *fL*::*;
      *google::*;
    };

As currently dynamic build has no symbol conflict so far, let's simply exclude the use of symbols.map in this build path.