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 namespace fixes #513

Merged
merged 2 commits into from
May 25, 2017
Merged

cmake namespace fixes #513

merged 2 commits into from
May 25, 2017

Conversation

niosHD
Copy link
Contributor

@niosHD niosHD commented May 23, 2017

Hi,

this PR addresses two small issues which have been missed in PR #511. First, the second target in the find-package-test has not been updated (see 9f48bb8). Second, in my opinion it should not matter how (add_subdirectory or find_package) the library has been added into a consuming project. I therefore added aliases with the prefix which makes it possible to always use the namespace version (fmt::fmt) consistently (see a68c7a8).

Regards,
niosHD

For the consumer it should not matter if fmt has been added to the
project as subdirectory or via find_package. With the alias targets
the library can be always imported via fmt::fmt.
@vitaut vitaut merged commit ac5484c into fmtlib:master May 25, 2017
@vitaut
Copy link
Contributor

vitaut commented May 25, 2017

Seems reasonable. Thanks for the PR!

@wieland400
Copy link

Hi,

what happened to this change? The alias is not defined in the current version. As a result

target_link_libraries(foo PRIVATE fmt::fmt-header-only)

from the documentation http://fmtlib.net/latest/usage.html#header-only-usage-with-cmake yields a

Target "foo" links to target "fmt::fmt-header-only" but the target was
not found. Perhaps a find_package() call is missing for an IMPORTED
target, or an ALIAS target is missing?

@niosHD
Copy link
Contributor Author

niosHD commented Mar 5, 2018

It seems it was lost when the std branch was merged. In the 4.x branch it is still present. @vitaut would you mind to apply the patch again or should I send a new PR?

@vitaut
Copy link
Contributor

vitaut commented Mar 6, 2018

Hmm, indeed looks like I missed it while merging. I'll reapply.

@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2018

On a second thought, I'm leaning towards killing the CMake namespace altogether (or, rather, not migrating it from the 4.x branch) because there are just two exported targets and both already have fmt in their names so adding a namespace seems redundant.

@niosHD
Copy link
Contributor Author

niosHD commented Mar 7, 2018

FWIW, I would keep the namespace. Like mentioned in the original PR #511, having the namespace is recommended by CMake's develop documentation and influences how errors are reported when something is wrong with the imported target.

@vitaut
Copy link
Contributor

vitaut commented Mar 13, 2018

Good point, re-merged and added a comment why the namespace is useful.

@vitaut
Copy link
Contributor

vitaut commented Mar 21, 2019

@niosHD, could you please review the updated CONTIBUTING document, particularly the part about licensing, and let me know if you agree with it being applied both to your contributions to {fmt}? The library is likely to be relicensed (#1073) so I'm collecting approval from all earlier contributors.

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.

3 participants