rapidsai / rapids-cmake

https://docs.rapids.ai/api/rapids-cmake/stable/
Apache License 2.0
30 stars 46 forks source link

[FEA] allow symbol visibility control for targets created by rapids_cython_create_modules() #672

Open jameslamb opened 2 months ago

jameslamb commented 2 months ago

Is your feature request related to a problem? Please describe.

rapids_cython_create_modules() automatically creates targets based on Cython source files.

During the development of libcudf wheels for https://github.com/rapidsai/build-planning/issues/33, we discovered that some of the objects from those targets were carrying around unnecessarily-public symbols, leading to runtime symbol conflicts.

ref: https://github.com/rapidsai/rmm/pull/1644

rapids_cython_create_modules() should support modifying the symbol visibility related properties of targets it creates.


Describe the solution you'd like

I can think of at least 4 options for this.

(Option 1) - new visibility argument

rapids_cython_create_modules() could take on a higher-level visibility argument, that then translates into setting possibly multiple target properties. e.g.

rapids_cython_create_modules(
   ...
  DEFAULT_VISIBILITY "hidden"
)

Which that function would turn into calls like this:

set_target_properties(
  ${target}
  PROPERTIES
    C_VISIBILITY_PRESET "hidden"
    CXX_VISIBILITY_PRESET "hidden"
)

(Option 2) - new target properties argument

rapids_cython_create_modules() could accept an argument ADDITIONAL_TARGET_PROPERTIES which contains a map with an arbitrary set of target properties to set on all the targets it creates.

rapids_cython_create_modules(
   ...
  ADDITIONAL_TARGET_PROPERTIES
      C_VISIBILITY_PRESET hidden
      CXX_VISIBILITY_PRESET hidden
)

Which that function would turn into calls like this:

set_target_properties(
  ${target}
  PROPERTIES ${ADDITIONAL_TARGET_PROPERTIES}
)

(Option 3) - new per-target target properties argument

Similar to option 2, but instead of a set of target properties applied to all targets, a mapping from target name to properties.


(Option 4) - do nothing

rapids_cython_create_modules() already sets a variable containing the names of the targets it created. That could just be re-used like in the proposal from https://github.com/rapidsai/rmm/pull/1644#issuecomment-2285079979, with 0 chagnes to rapids-cmake.

rapids_cython_create_modules(...)

foreach(_cython_target IN LISTS RAPIDS_CYTHON_CREATED_TARGETS)
    set_target_properties(
        ${_cython_target}
        PROPERTIES
            C_VISIBILITY_PRESET hidden
            CXX_VISIBILITY_PRESET hidden
    )
endforeach()

Describe alternatives you've considered

N/A - see above

Additional context

Related to https://github.com/rapidsai/build-planning/issues/33

vyasr commented 2 months ago

Technically rapids-cython has never promised that target names would be generated in any particular way, but in practice the name generation has always been stable and we rely on it in many places throughout RAPIDS to be able to modify specific targets. I would propose that we make the naming scheme explicit and something that can be publicly relied upon. In that world, I would vote against option 3 because if consumers need target-specific behavior they can just set properties on the targets of interest directly. I think any of the other options are OK. I have a weak preference for 1/2 over 4 since presumably this is going to be something that most users of rapids-cmake are going to want at some level.