You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 rapidsai/build-planning#33, we discovered that some of the objects from those targets were carrying around unnecessarily-public symbols, leading to runtime symbol conflicts.
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() 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.
(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 rapidsai/rmm#1644 (comment), with 0 chagnes to rapids-cmake.
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.
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 rapidsai/build-planning#33, we discovered that some of the objects from those targets were carrying around unnecessarily-public symbols, leading to runtime symbol conflicts.ref: rapidsai/rmm#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:
(Option 2) - new target properties argument
rapids_cython_create_modules()
could accept an argumentADDITIONAL_TARGET_PROPERTIES
which contains a map with an arbitrary set of target properties to set on all the targets it creates.Which that function would turn into calls like this:
(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 rapidsai/rmm#1644 (comment), with 0 chagnes torapids-cmake
.Describe alternatives you've considered
N/A - see above
Additional context
Related to rapidsai/build-planning#33
The text was updated successfully, but these errors were encountered: