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

[bug] cmake generator: wrong/inconsistent casing CONAN_PKG::xxxx #6269

Closed
dmn-star opened this issue Dec 21, 2019 · 9 comments
Closed

[bug] cmake generator: wrong/inconsistent casing CONAN_PKG::xxxx #6269

dmn-star opened this issue Dec 21, 2019 · 9 comments
Assignees
Milestone

Comments

@dmn-star
Copy link

dmn-star commented Dec 21, 2019

I have tried to use the recipes ( libxml, boost etc) from the Conan Center Index repo.

Target "test_conan_index" links to target "CONAN_PKG::libxml2" but the target
was not found. Perhaps a find_package() call is missing for an IMPORTED

from my CMakeLists.txt.
..
conan_basic_setup(TARGETS)
...
target_link_libraries( # Specifies the target library.
test_conan_index
${log-lib} CONAN_PKG::boost CONAN_PKG::cpprestsdk CONAN_PKG::libxml2 CONAN_PKG::libxslt)
....

I've changed from CONAN_PKG::libxml2 to CONAN_PKG::LibXml2 to compile successfully.

After the next update, nothing compile again because of boost.
I've changed from CONAN_PKG:: boost to CONAN_PKG::Boost to compile successfully.

Please note CONAN_PKG::libxslt is still lower case.

Please make all CONAN_PKG lower case again.

Environment Details (include every applicable attribute)

  • Operating System+version: cpprestsdk/2.10.14@bincrafters/stable
  • Compiler+version: macOS Catalina 10.15.1
  • Conan version: NDK r20 clang (8?)
  • Python version: Conan 1.20.5

Steps to reproduce (Include if Applicable)

conanfile.txt
[requires]
cpprestsdk/2.10.14@bincrafters/stable
openssl/1.1.1c
libxml2/2.9.9
zlib/1.2.11
boost/1.70.0
libxslt/1.1.33@bincrafters/stable

[generators]
cmake

[options]
cpprestsdk:exclude_websockets = True

Use the dependencies from my conanfile.txt for a test executable.

Logs (Executed commands with output) (Include/Attach if Applicable)

@memsharded
Copy link
Member

The .name field has been introduced in cpp_info to be able to define the target name in cmake. Not all recipes in ConanCenter are using it yet, it has been started to be used. This was done to align with the target names defined by existing find.cmake scripts (outside of Conan), for a more transparent integration. It is true that I thought that it was applied only to the cmake_find_package generator targets, and not the CONAN_PKG targets, but I cannot find now if that was a conscious decision or not. I think using it in CONAN_PKG NOT matching the exact package name makes it more complicated, because it is not easy at all to know the name that you should use. cc/ @uilianries @danimtb @SSE4

@danimtb
Copy link
Member

danimtb commented Dec 23, 2019

The cpp_info.name is also used by the targets generated in the cmake generator, not only for the cmake_find_package* ones

@danimtb danimtb self-assigned this Dec 23, 2019
@dmn-star
Copy link
Author

@memsharded Thank you for clearing up. I quite agree with you. The CONAN_PKG naming was clear and simple. I don't see any reason to changes the CONAN_PKG naming convention to follow the pure chaos of cmake find_package(...) ones.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 2, 2020

HI! IMO the implementation is right as it is now, but we are not using it in the right way in some recipes. And this is due to the order of implementation of the cpp_info.name and cpp_info.names features. Let me recap the features with an example:

Given this recipe:

class Recipe(ConanFile):
    name = "lzma_sdk"

    def package_info(self):
        self.cpp_info.name = "lzma"
        self.cpp_info.names["cmake_find_package"] = "LZMA"
        self.cpp_info.names["cmake_find_package_multi"] = "LZMA"
  • The reference name is lzma_sdk. It is how other packages require this one from the Conan perspective, it is the name of the reference, it can be painful to change it, so we implemented the cpp_info.name
  • Every generator will use lzma as the name, but some generators need a different name, and we implemented cpp_info.names, so
  • cmake_find_package[_multi] will use LZMA instead.

The problem with zLiB

The current revision of the zlib package is as follows:

class ZlibConan(ConanFile):
    name = "zlib"

    def package_info(self):
        self.cpp_info.name = "ZLIB"
        self.cpp_info.names["pkg_config"] = "zlib"

but, IMO, it should be:

class ZLibConan(ConanFile):
    name = "zlib"

    def package_info(self):
        self.cpp_info.names["cmake_find_package"] = "ZLIB"
        self.cpp_info.names["cmake_find_package_multi"] = "ZLIB"

with this usage of the features, we would have preserved previous behavior and achieve the aim of these features.

TL; DR: only the generators that do not match the name of the package (overridden by cpp_info.name) should explicit a different name.


c4a

We agree that we have broken behavior somehow, so the idea is the following one:

  • Conan v1.21.1: make cmake generator use the package name as it was before CONAN_PKG::<pkg_name> (or the explicit cpp_info.names["cmake"] if provided)
  • Conan v1.22 will keep using current implementation (won't merge these changes from v1.21.1)
  • ⚠️ conan-center-index:
    • Ideally, no recipe should use cpp_info.name, the package name should be the right one.
    • If we want to match CMake's FindXXX.cmake variables we should use cpp_info.names["..."]

@Morwenn
Copy link
Contributor

Morwenn commented Jan 2, 2020

I want to add something to the last point: indeed I more and more feel like cpp_info.name should not be used, and the package name should be used everywhere unless compatibility is needed for specific generators, then cpp_info.names[generator] should be used.

I also feel like not specifying cpp_info.name and not specifying cpp_info.names["cmake"] is one way to fix #6001 by which I was bitten some time ago, so I would go as far as saying that it should be turned into a rule enforced by the CCI hook in the future.

@memsharded memsharded added this to the 1.21.1 milestone Jan 8, 2020
@memsharded
Copy link
Member

Fixed, will be released soon in Conan 1.21.1

@jmarrec
Copy link
Contributor

jmarrec commented Jan 9, 2020

jmarrec added a commit to NREL/OpenStudio that referenced this issue Jan 9, 2020
…nSSL is an exception

Read the issue, and the specific OpenSSL comment here: conan-io/conan#6269 (comment)
@jgsogo
Copy link
Contributor

jgsogo commented Jan 9, 2020

Hi @jmarrec . There is PR already opened conan-io/conan-center-index#604, we need to review it, but it will be merged soon.

@jmarrec
Copy link
Contributor

jmarrec commented Jan 9, 2020

@jgsogo thanks, I missed it in the list of referenced PRs above

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

No branches or pull requests

6 participants