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

Use a version check to provide backwards comatible CMake imported target names #1245

Conversation

chuckatkins
Copy link
Contributor

Fixes #1243

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3b1a5ca on chuckatkins:fix-target-namespace-backward-compatibility into 99939d6 on nlohmann:develop.

@nlohmann nlohmann self-assigned this Sep 19, 2018
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann added this to the Release 3.2.1 milestone Sep 19, 2018
@nlohmann nlohmann merged commit e4bc98d into nlohmann:develop Sep 19, 2018
@nlohmann
Copy link
Owner

Thanks a lot for the quick reaction!

@SylvainCorlay
Copy link

I like the fix. Did not know about PACKAGE_FIND_VERSION.

@SylvainCorlay
Copy link

@nlohmann @chuckatkins this does not appear to work. People are complaining that they cannot build against master and getting.

CMake Error at nlohmann_jsonConfig.cmake:30 (add_library):
  add_library cannot create ALIAS target "nlohmann_json" because target
  "nlohmann_json::nlohmann_json" is IMPORTED.
Call Stack (most recent call first):
  CMakeLists.txt:33 (find_package)

I think that we should revert that change.

@chuckatkins
Copy link
Contributor Author

Bad on my part :-(. I'll push a fix today, but also add a test to validate import configs to prevent future breakage.

@chuckatkins
Copy link
Contributor Author

@ericu65 this should be fixed by #1260

@ericu65
Copy link

ericu65 commented Sep 30, 2018

@chuckatkins Thanks. On first look this would seems to fix it. Looking forward to a merge.

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.

5 participants