-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[CMake] Modify swift_install_in_component to support cmake install components #24168
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
Conversation
…mponents CMake supports the notion of installation components. Right now we have some custom code for supporting swift components. I think that for installation purposes, it would be nice to use the CMake component system. This should be a non-functional change. We should still only be generating install rules for targets and files in components we want to install, and we still use the install ninja target to install everything.
"" # options | ||
"COMPONENT" # single-value args | ||
"" # multi-value args | ||
${ARGN}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually this is nicer with variables for the individual options rather than comments, but, this should be short lived, so meh.
This is NFC. |
@swift-ci please test |
Build failed |
@swift-ci please test macOS platform |
Build failed |
@swift-ci please test macOS platform |
Build failed |
@swift-ci please test macOS platform |
@Rostepher Mind taking a quick look at this when you have the chance? |
@Rostepher @gottesmm friendly ping |
# | ||
# Executes the specified installation actions if the named component is | ||
# requested to be installed. | ||
# | ||
# This function accepts the same parameters as install(). | ||
function(swift_install_in_component component) | ||
precondition(component MESSAGE "Component name is required") | ||
function(swift_install_in_component) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the only big change here passing in COMPONENT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the only big change in this PR.
LGTM |
@xiaobai next time put that into the commit msg. It will make it easier to review. |
CMake supports the notion of installation components. Right now we have some
custom code for supporting swift components. I think that for installation
purposes, it would be nice to use the CMake component system.
This should be a non-functional change. We should still only be generating
install rules for targets and files in components we want to install, and we
still use the install ninja target to install everything.
I originally made this change and more to simplify the build system. I've decided to break up that change into smaller pieces so that it is easier to reason about and review. This is the first part of that change. The original change is here: #24137
cc @compnerd @gottesmm @Rostepher