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

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

Open
jameslamb opened this issue Aug 13, 2024 · 1 comment
Labels
? - Needs Triage Need team to review and classify feature request New feature or request

Comments

@jameslamb
Copy link
Member

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:

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 rapidsai/rmm#1644 (comment), 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 rapidsai/build-planning#33

@jameslamb jameslamb added feature request New feature or request ? - Needs Triage Need team to review and classify labels Aug 13, 2024
@vyasr
Copy link
Contributor

vyasr commented Aug 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify feature request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants