Skip to content

Support libraries consisting of various components #48

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

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

Conversation

Krzmbrzl
Copy link

@Krzmbrzl Krzmbrzl commented Jan 7, 2025

This PR does a bit more than "merely" the components support. It improves upon some things that I have noticed along the way. "Unrelated" changes are strictly kept in separate, preceding commits though, so hopefully that's okay...

This PR
Fixes #47
Fixes #22
Fixes #43 (kinda)


TODO:

  • Update README with documentation on components
  • Add in-source documentation to the various helper functions
  • Make helper functions use cmake_parse_arguments for more readable function calls
  • Check whether CPack still works as expected (I guess the CI will do that once approved)
  • Handle OPTIONAL_COMPONENTS of find_package call

By using the <lowercasePackageName>-config.cmake and
<lowercasePackageName>-config-version.cmake naming schemes for the
generated CMake config files, we make it possible for these config files
to be found, irrespective of the capitalization used in the original
find_package call. That is, find_package(NaMe) and find_package(name)
will both find the same config file.

This is already the case no case-insensitive filesystems (e.g. under
Windows). However, on case-sensitive filesystems (e.g. most Linux
setups) the used capitalization makes a difference. This can be a source
of confusion and annoyance in cross-platform development.

With this commit, this platform differences will vanish.
The config file will now detect wrong capitalization of the package name
and emit a warning about it. Furthermore, it will try its best to
subsequently use the package name in the capitalization as used in the
respective find_package call. This should minimize the issues that can
arise from wrong capitalization (such as mismatched capitalization in
the resulting variables).
Nonetheless, there are likely some issues remaining, which is why the
warning requests fixing the capitalization of the package name.
Fixes TheLartians#22
Fixes TheLartians#43 (if using multiple targets as components)
@ClausKlein
Copy link
Contributor

see my PR Krzmbrzl#1

@TheLartians
Copy link
Owner

Hey, thanks for the PR! I quickly looked at the individual commits and liked the changes so far. However I find the full PR diff hard to overlook. Do you think it would make sense to separate each commit into its own PR? I think that would make it easier to discuss and trace the individual changes.

@Krzmbrzl
Copy link
Author

Do you think it would make sense to separate each commit into its own PR?

Some parts, maybe but other parts are inter-dependent so splitting would probably produce interdependent PRs. Besides, I don't know yet when I might have the time again to do the splitting.

If you do the review commit by commit, things should be clearly separated though. I tried to make one commit per logical change group. Maybe that helps?

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