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

json-schema-validator: restore correct CMake target name in CMakeDeps #14194

Merged

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Nov 14, 2022

handle this regression: #13959 (comment)


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 1 (3943d341b141948c7574eb35c29582cdb208abd0):

  • json-schema-validator/2.0.0@:
    All packages built successfully! (All logs)

  • json-schema-validator/2.1.0@:
    All packages built successfully! (All logs)

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit 3943d34
json-schema-validator/2.0.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libjson-schema-validator.so' links to system library 'm' but it is not in cpp_info.system_libs.
json-schema-validator/2.1.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libnlohmann_json_schema_validator.so' links to system library 'm' but it is not in cpp_info.system_libs.

@garethsb
Copy link
Contributor

garethsb commented Nov 15, 2022

I don't quite understand...

I have a project which has always used the target nlohmann_json_schema_validator::nlohmann_json_schema_validator, which worked with the pre- and post-13959 recipe, and having tested it again, doesn't work with this PR.

Ah, did the cmake_find_package generator produce the :: target name, and that's why this recipe uses _create_cmake_module_alias_targets?

It may be the case that this alias was not aligned with the upstream CMakeLists.txt, and the downstream project is therefore at fault relying on a conan target that didn't exist in the upstream, but its absence is unfortunately still a breaking change.

The downstream project is like:

  • conanfile.txt
    [requires]
    json-schema-validator/2.1.0
    
  • CMakeLists.txt
    find_package(nlohmann_json_schema_validator REQUIRED)
    ...
    add_library(json_schema_validator INTERFACE)
    target_link_libraries(json_schema_validator INTERFACE nlohmann_json_schema_validator::nlohmann_json_schema_validator)
    

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 15, 2022

It's not a breaking change because CMakeDeps has always produced non-namespaced target for json-schema-validator until your PR (which was a breaking change). Non-namespaced target is the official upstream target, namespaced target was a side effect of cmake_find_package behavior. Purpose of _create_cmake_module_alias_targets was to provide non-namespaced target in cmake_find_package as well.

@garethsb
Copy link
Contributor

For sure we need the non-namespaced target back, but this is a breaking change removing the namespaced target. In that a downstream project that previously used this Conan recipe will no longer work.

For many libraries, many projects will never have used upstream packages, only conan packages.

It may be that the CCI answer for those projects that (without realising, and why would they?) relied on the side effect of the cmake_find_package generator is that it's their problem, but that's unfortunate.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 15, 2022

For sure we need the non-namespaced target back, but this is a breaking change removing the namespaced target. In that a downstream project that previously used this Conan recipe will no longer work.

It doesn't break anything for a given generator. Folks using CMakeDeps were already relying on non-namespaced target, and if it doesn't work for you anymore it's because you have switched from cmake_find_package to CMakeDeps, so a good opportunity to fix your CMakeLists.

I don't like the idea to provide alias targets in CMakeDeps just because we would have to support unwanted targets which were generated by cmake_find_package in conan v1.

@uilianries @danimtb @prince-chrismc what are your thoughts?

@garethsb
Copy link
Contributor

OK, makes sense. Thanks very much for the explanation.

@prince-chrismc
Copy link
Contributor

@conan-center-bot conan-center-bot merged commit 266ec62 into conan-io:master Nov 15, 2022
@SpaceIm SpaceIm deleted the fix/json-schema-validator-regression branch November 15, 2022 18:14
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 15, 2022

@prince-chrismc
Copy link
Contributor

@SpaceIm can you please becareful with your choice of words when answer?

Instead of what you wrote you could have said

"Hey Chris, I think you might have linked the wrong line. The install function that sets the namespace is actually... and you can see it here"

This would provide a far superior experience to contributors by politely correcting them :)

Hopefully you can apply this advice when interacting with others!

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