-
Notifications
You must be signed in to change notification settings - Fork 15
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
Option to export multiple targets #36
base: master
Are you sure you want to change the base?
Conversation
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.
Hey, thanks for the PR! I can see how installing multiple targets can be useful.
- The unit tests seem to failing in CI, can you investigate?
- Can you add a test case, so we see the function working and don't accidentally break it in the future?
- Can you reformat the code using cmake-format?
CMakeLists.txt
Outdated
# Add custom TO_INSTALL property to the project targets | ||
define_property( | ||
TARGET | ||
PROPERTY TO_INSTALL | ||
BRIEF_DOCS "" | ||
FULL_DOCS "" | ||
) |
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.
Can you explain the purpose of this property?
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.
The idea is that one can collect the targets to install from deeply nested sub-directories automatically by setting this custom target property. This is also very convenient if some targets are optional, so the installation list is updated automatically.
README.md
Outdated
# The list of targets to install. If empty single target ${PROJECT_NAME} is installed. | ||
TARGETS_TO_INSTALL ${PROJECT_TARGETS_TO_INSTALL} |
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.
Could you also document the new AUTO_FIND_TARGETS_TO_INSTALL
argument?
CMakeLists.txt
Outdated
foreach(subdir ${subdirs}) | ||
get_all_cmake_targets(subdir_targets ${subdir}) | ||
list(APPEND targets ${subdir_targets}) | ||
endforeach() |
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.
Does it make sense to install targets from all subdirectories as well? This could accidentally result in unrelated dependencies being installed as part of the main package.
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.
Only the targets explicitly marked with TO_INSTALL property will be installed, so this should be safe.
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.
If some nested directory uses dependency, which sets this property forcefully than yes, it will be installed, but then this is not a well-behaving dependency.
This change adds an optional arguments TARGETS_TO_INSTALL that allows to add several targets, not just a single target PROJECT_NAME. When this option is not set the previous behavior is preserved and only a single target PROJECT_NAME is installed.