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

[cmake] consolidate set_target_properties() calls #6594

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

jameslamb
Copy link
Collaborator

Another step towards simplifying this project's CMake configuration (are you sensing a theme in my recent PRs? 😂 ).

CMake's set_target_properties() accepts multiple targets and multiple properties (CMake docs).

This proposes taking advantage of that to consolidate a few calls.

@jameslamb jameslamb changed the title WIP: [cmake] consolidate set_target_properties() calls [cmake] consolidate set_target_properties() calls Aug 9, 2024
@jameslamb jameslamb marked this pull request as ready for review August 9, 2024 02:07
lightgbm_objs _lightgbm
PROPERTIES
CUDA_ARCHITECTURES ${CUDA_ARCHS}
CUDA_SEPARABLE_COMPILATION ON
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CUDA_SEPARABLE_COMPILATION was not set for _lightgbm target in previous config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! Thank you for catching that, I've updated it in recent commits.

This also led me to question what this property actually does. Here's a simple explanation of it: https://stackoverflow.com/a/50557765/3986677

endif()
set_target_properties(_lightgbm PROPERTIES CUDA_RESOLVE_DEVICE_SYMBOLS ON)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was lost in new config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! Too many similarly-named properties 😂

Thank you for catching this, I've fixed that in recent commits.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for solving this CMake puzzle! 😄

@StrikerRUS StrikerRUS merged commit 874f6fb into master Aug 15, 2024
44 checks passed
@StrikerRUS StrikerRUS deleted the more-cmake branch August 15, 2024 14:48
@jameslamb
Copy link
Collaborator Author

Sure! I've been learning a lot about CMake lately, hoping to continue simplifying and improving LightGBM's CMake stuff.

I'm especially interested in making LightGBM more friendly to build and link to for repackagers like @barracuda156 (see for example #6489).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants