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

Option to export multiple targets #36

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

yesint
Copy link

@yesint yesint commented Jul 9, 2023

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.

Copy link
Owner

@TheLartians TheLartians left a 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
Comment on lines 8 to 14
# Add custom TO_INSTALL property to the project targets
define_property(
TARGET
PROPERTY TO_INSTALL
BRIEF_DOCS ""
FULL_DOCS ""
)
Copy link
Owner

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?

Copy link
Author

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
Comment on lines 25 to 26
# The list of targets to install. If empty single target ${PROJECT_NAME} is installed.
TARGETS_TO_INSTALL ${PROJECT_TARGETS_TO_INSTALL}
Copy link
Owner

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
Comment on lines 21 to 24
foreach(subdir ${subdirs})
get_all_cmake_targets(subdir_targets ${subdir})
list(APPEND targets ${subdir_targets})
endforeach()
Copy link
Owner

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.

Copy link
Author

@yesint yesint Jul 12, 2023

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.

Copy link
Author

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.

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

Successfully merging this pull request may close these issues.

2 participants