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

[feature] Define file name for CMake find generators #7320

Conversation

TimSimpson
Copy link
Contributor

@TimSimpson TimSimpson commented Jul 7, 2020

Changelog: Feature: Adds "filenames" to cppinfo attribute, and changes cmake_find_package and cmake_find_package_multi generators so that they support it.
Docs: conan-io/docs#1768

See issue 7254

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jul 7, 2020

CLA assistant check
All committers have signed the CLA.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 7, 2020

Apologies for opening up a PR with failing tests. I'm not sure what has gone wrong, though to be frank I ran into trouble making all the nose tests pass locally on my box even when using the develop branch. I've tested it against this repo and the new behavior appears to work as intended.

@prince-chrismc
Copy link

@prince-chrismc prince-chrismc commented Jul 7, 2020

It would be amazing if you could add some tests

there's lots of sections, this is an example
https://github.com/conan-io/conan/blob/develop/conans/test/functional/generators/cmake_components_test.py

@jgsogo jgsogo added this to the 1.28 milestone Jul 8, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jul 8, 2020

I'm running again the CI, the failure was not related to this PR at all. We don't develop using the CI, but it is totally fine to open the PR if some tests are failing, better to show first the idea and receive early feedback.

We need some tests for this feature, inside the module conans/test/functional/generators/ could be a good place to add a new file with these tests.

And finally, you are really BRAVE, the source code around the CMake generators is a mess (I'm preparing a big refactor) and I think you have nailed it 🎉

@TimSimpson TimSimpson force-pushed the feature/define_file_name_for_cmake_find_generators branch from 4561d40 to 3446b95 Compare Jul 12, 2020
@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 12, 2020

@prince-chrismc @jgsogo I've added a test for the filenames attribute. It's not a lot but I think it gets to the heart of the filename feature by creating two packages with the same "name" attribute, ie the same exported target namespace.

One thing I noticed is right now the "global" target, which is produced even if components are present (for example, with package name ACME it's typically ACME::ACME), is unusable between two packages which share a name. Assuming anyone cares I'm not sure what the fix could be beyond introducing another attribute. If this is merged I'll make a separate issue for that.

@prince-chrismc
Copy link

@prince-chrismc prince-chrismc commented Jul 13, 2020

This looks it provides enough functionality to cover the current need.

the "global" target, which is produced even if components are present (for example, with package name ACME it's typically ACME::ACME), is unusable between two packages which share a name

That may be an issue for very rare occurrences. in CCI there's a potential for this with https://github.com/TartanLlama projects, there optional, expected, and tl which will all have the same name "tl", including two of them would cause a collision.

What's the value of filename if it's not set?
The proposed change sounds like it would for us to migrate a lot of recipes that user the generator name to change the filename (which will no longer be the case)

This begs the questions, what if we left the Find{project}.cmake as it was based on the generator name, and added a namespace attribute which would change the namespace for components and leave the generic component (which has the added benefit of not breaking existing recipes or consumers) as the generator name

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 13, 2020

What's the value of filename if it's not set?

It defaults to names["generator"]. In theory it should be backwards compatible with per-existing usage.

This begs the questions, what if we left the Find{project}.cmake as it was based on the generator name, and added a namespace attribute which would change the namespace for components and leave the generic component (which has the added benefit of not breaking existing recipes or consumers) as the generator name

This was my original take as well (I proposed calling it cmake_target or cmake_namespace). I do still wonder if it would be more intuitive. I'd be willing to open a PR tackling it from that angle to see if there'd be interest in comparing the two approaches.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jul 13, 2020

We all agree that reproducing CMake's behavior from a generic perspective is very challenging, there will always be exceptions in individual FindXXX.cmake files and we need to stop adding complexity at some point in time.

We also need to keep in mind that the information in cpp_info must be meaningful for every generator out-there. We know CMake is a big player, but there are others out there and we shouldn't provide things too specific for it.


Maybe we need to step back. Alternatives in this thread:

  1. introduce a feature for filename (usable by every generator): every generator uses some kind of file/s, this feature will let the user define a different name for the file and for the thing provided inside the file.

    cpp_info.names["cmake_find_package"] = "protobuf"
    cpp_info.filename["cmake_find_package"] = "Protobuf"
    cpp_info.components["libprotobuf"].names["cmake_find_package"] = "libprotobuf"
    FindProtobuf.cmake --> protobuf::protobuf, protobuf::libprotobuf,...
    

    Problem: two packages with the same cpp_info.names["xxx"] will generate the same all-package target in two different files. Only one of them will be available to the consumers.

    cpp_info.names["cmake_find_package"] = "tl"
    cpp_info.filename["cmake_find_package"] = "tl-expected"
    cpp_info.components["tl-expected"].names["cmake_find_package"] = "tl-expected"
    Findtl-expected.cmake --> tl::tl, tl::tl-expected
    Findtl-optional.cmake --> tl::tl, tl::tl-optional,...
    

    ...and there is no way to merge all the components from different packages into a single tl::tl target. This will require to refactor all these recipes into a single one to provide the tl::tl target with them all.

  2. introduce a feature for namespace (only usable by CMake?): the last proposal in this thread is to use this namespace only for the components, not for the all-package target. It will allow different packages to provide component-targets in the same namespace without having a collision with the name of the all-package target.

    cpp_info.names["cmake_find_package"] = "Protobuf"
    cpp_info.cmake_namespace = "protobuf"
    cpp_info.components["libprotobuf"].names["cmake_find_package"] = "libprotobuf"
    FindProtobuf.cmake --> Protobuf::Protobuf, protobuf::libprotobuf,...
    

    Problem: filename and all-package target will use the same information, collision means that one package will override the file generated by the other.

    cpp_info.names["cmake_find_package"] = "tl-expected"
    cpp_info.cmake_namespace = "tl"
    cpp_info.components["tl-expected"].names["cmake_find_package"] = "tl-expected"
    Findtl-expected.cmake --> tl-expected::tl-expected, tl::tl-expected
    Findtl-optional.cmake --> tl-optional::tl-optional, tl::tl-optional,...
    

    Still not possible to provide a tl::tl target without refactoring all the recipes into a single one.

    Problem: to rename the all-package target (without changing the filename) it is needed to provide a component that requires all the other components

    cpp_info.names["cmake_find_package"] = "Protobuf"
    cpp_info.cmake_namespace = "protobuf"
    cpp_info.components["libprotobuf"].names["cmake_find_package"] = "libprotobuf"
    cpp_info.components["__all__"].names["cmake_find_package"] = "protobuf"
    FindProtobuf.cmake --> Protobuf::Protobuf, protobuf::protobuf, protobuf::libprotobuf,...
    

We need examples for other generators

@prince-chrismc
Copy link

@prince-chrismc prince-chrismc commented Jul 13, 2020

This looks to provide enough functionality to cover the current need.

It looks like we are all on the same page, the PR as it stands will certainly bridge the gap nicely. This topic may unfortunately be revisited but all solutions since have limitation a @jgsogo points out.

I am 💯 okay with this PR being merged


As for solution two,

Problem: filename and all-package target will use the same information, collision means that one package will override the file generated by the other.

This is actually a problem already in CMake, where is two projects use the same file name, the second one will override the first. Projects of responsible for providing their own unique name. Consumers should check before to be safe.

Problem: to rename the all-package target (without changing the filename) it is needed to provide a component that requires all the other components

I do not think this is desirable. I think it should match the filename. Currently if you check the provided modules, this is the theme for many of them example, Boost and OpenSSL are others.

Lastly, the tl::tl is not a merge of tl-expected and tl-optional, it is a very separate project (oddly enough). I am in the same that merging recipes in the generators is the wrong direction.

We also need to keep in mind that the information in cpp_info must be meaningful for every generator out-there.

This will be true for every other attribute, it's cumbersome to have specific one for a generator.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 14, 2020

I'd love to see this merged soon, but I also want to point out one more thing that's bothering me about the PR, which is the interface Conan users will end up with.

If filenames is merged, there will be two attributes for related packages (such as tl-expected and tl-optional): the idiomatically unique field filenames["generator"], and the no longer entirely unique field names["generator"].

Generator and package writers would need to understand that names even among well-behaved packages may not be totally unique, and filenames should be used to ensure actual uniqueness. For instance in this PR I changed the two CMake generators I touched so that several of the global CMake variables they create derive their name from filenames instead of names.

There I get hung up on a mild sense of shame, because this feels unintuitive. names feels like it should be a unique value. Having to set / use the filenames attribute seems esoteric.

If there was an attribute with a name like namespaces IMO most people would understand that value might be shared between multiple Conan packages even if they didn't read the docs.

Thinking about other generators, a situation that would be regrettable would be if a generator had to resort to using filenames everywhere because it was unique instead names even if it didn't offer CMake style namespacing.

That said, I have no knowledge on build systems today outside of a little on CMake so I'll cede to the experts here.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 19, 2020

I wrote up the namespaces version of this just to show what it would be like:

https://github.com/conan-io/conan/compare/develop...TimSimpson:feature/define_namespace_for_cmake_find_generators?expand=1

IMO it's simpler.

To reiterate, my biggest qualms with using the filenames attribute to solve the "common CMake namespace issue" is if we go that route we'll need to update all generators to use the filenames attribute instead of names in places where unique values are required before we can start updating packages in Conan Central to share values for their names attribute- otherwise the packages won't work anymore with the other generators. Updating a namespaces attribute would cause no friction or dread at all when updating the Conan Central recipes. If needed we could still keep filename presented here and use it strictly for the generated filenames but stick to the names attribute for anything else such as global variables.

Let me know if there's any way I can be of help with this.

@prince-chrismc
Copy link

@prince-chrismc prince-chrismc commented Jul 19, 2020

Open the new PR 😝 the diff is much less intrusive and it's much more explicit.

Any generator which needs to prefix it's parts could use this new attribute

It does not break existing recipes which makes it ideal.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 19, 2020

@prince-chrismc Opened here.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 19, 2020

I've created an issue for namespaces here.

I can understand how introducing two attributes here may seem like overkill at first glance, but after thinking about this for awhile I think if both features were offered it would be a relief to people using Conan and trying to emulate setups desired - rightly or wrongly- by their consumers.

If issue 7385 seems acceptable I can modify this PR to avoid using the filename in the various global variables, so that cppinfo.name will still be used there.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 20, 2020

@prince-chrismc one thing I continuously forget is that because names has a different value for each generator, changes made to one generator won't affect each other. So disregard what I said earlier about needing to update every generator to use filename as a more unique value- they can just keep using whatever they do today.

Copy link
Member

@uilianries uilianries left a comment

I would like to see a test for cmake_find_package_multi.

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 22, 2020

Unfortunately, I think this does not allow to cover the freetype use case: https://cmake.org/cmake/help/latest/module/FindFreetype.html
in this case the filename should be FindFreetype.cmake, global variables should be FREETYPE_FOUND, and target should be Freetype::Freetype
Also BZip2 https://cmake.org/cmake/help/latest/module/FindBZip2.html:
in this case the filename should be FindBZip2.cmake, global variables should be BZIP2_FOUND, and target should be BZip2::BZip2
Also OpenSSL https://cmake.org/cmake/help/latest/module/FindOpenSSL.html:
in this case the filename should be FindOpenSSL.cmake, global variables should be OPENSSL_FOUND, and target should be OpenSSL::SSL

@uilianries
Copy link
Member

@uilianries uilianries commented Jul 22, 2020

@ericLemanissier I think it deserves another issue/PR, but you point is totally valid.

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 23, 2020

The corollary is: is it a good move to change the cmake variable names in this PR, knowing that it will have to be changed again ?

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 24, 2020

I just made changes to the code so that the filename variable isn't used anymore. In the process the tests broke, but it illustrates something so I left it like that for now.

The find_dependency macro docs state that return is called if a dependency can't be found. What I'm seeing is that when the XYZ_FOUND variable is changed back to being the name, but the filename passed to find_dependency is different, CMake assumes the dependency wasn't found and returns from the find module that called it which causes problems.

In this test there are two packages created: hello_1 and hello_2.

The generated Findhello_2.cmake has code which looks like this:

if(NOT MYHELLO_FOUND)
    find_dependency(hello_1 REQUIRED)
else()
    conan_message(STATUS "Conan: Dependency hello_1 already found")
endif()

Findhello_1.cmake then has this code:

conan_message(STATUS "Conan: Using autogenerated Findhello_1.cmake")
set(MYHELLO_FOUND 1)

I added calls to message in Findhello_2.cmake and found that after find_dependency no other code is executed. However, if I change Findhello_1.cmake by adding set(hello_1_FOUND 1) to it, everything works.

So I do not think it will be possible for the found variables to be different from the filenames generally. Maybe additional variables for certain projects could be created using build modules.

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 24, 2020

... well it fails on my box anyway. 😕

python -m nose conans/test/functional/generators/cmake_find_package_test.py:CMakeFindPathGeneratorTest

...S....E.
======================================================================
ERROR: cpp_info_filename_test (conan.conans.test.functional.generators.cmake_find_package_test.CMakeFindPathGeneratorTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ts/code/conan/conans/test/functional/generators/cmake_find_package_test.py", line 563, in cpp_info_filename_test
    client.run("build .")
  File "/home/ts/code/conan/conans/test/utils/tools.py", line 849, in run
    self._handle_cli_result(command_line, assert_error=assert_error, error=error)
  File "/home/ts/code/conan/conans/test/utils/tools.py", line 873, in _handle_cli_result
    raise Exception(exc_message)
Exception:
------------------------ Command failed (unexpectedly): ------------------------
build .
----------------------------------- Output: ------------------------------------
Using lockfile: '/tmp/tmpxansakyqconans/path with spaces/conan.lock'
Using cached profile from lockfile
conanfile.py: Calling build()
conanfile.py: WARN: compiler setting should be defined.
-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Conan: Using autogenerated Findhello_2.cmake
-- Found MYHELLO2: 1.0 (found version "1.0")
-- Library hello2 found /tmp/tmpdx92uj65conans/path with spaces/data/hello2/1.0/_/_/package/78085be3281648f3205515e0445494a00c7ecd0f/l
ib/libhello2.a
-- Found: /tmp/tmpdx92uj65conans/path with spaces/data/hello2/1.0/_/_/package/78085be3281648f3205515e0445494a00c7ecd0f/lib/libhello2.a
-- Conan: Using autogenerated Findhello_1.cmake
-- Found MYHELLO: 1.0 (found version "1.0")
-- Library hello found /tmp/tmpdx92uj65conans/path with spaces/data/hello/1.0/_/_/package/0ab9fcf606068d4347207cc29edd400ceccbc944/lib
/libhello.a
-- Found: /tmp/tmpdx92uj65conans/path with spaces/data/hello/1.0/_/_/package/0ab9fcf606068d4347207cc29edd400ceccbc944/lib/libhello.a
-- Library hello found /tmp/tmpdx92uj65conans/path with spaces/data/hello/1.0/_/_/package/0ab9fcf606068d4347207cc29edd400ceccbc944/lib
/libhello.a
-- Found: /tmp/tmpdx92uj65conans/path with spaces/data/hello/1.0/_/_/package/0ab9fcf606068d4347207cc29edd400ceccbc944/lib/libhello.a
-- Found: /tmp/tmpdx92uj65conans/path with spaces/data/hello/1.0/_/_/package/0ab9fcf606068d4347207cc29edd400ceccbc944/lib/libhello.a
CMake Error at CMakeLists.txt:6 (get_target_property):
  get_target_property() called with non-existent target "MYHELLO2::HELLO2".


Target libs (hello2):
Target libs (hello): CONAN_LIB::MYHELLO_HELLO1_hello;$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,SHARED_LIBRARY>:>;$<$<STREQUAL:$<TARGET_PROP
ERTY:TYPE>,MODULE_LIBRARY>:>;$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>:>
-- Configuring incomplete, errors occurred!
See also "/tmp/tmpxansakyqconans/path with spaces/CMakeFiles/CMakeOutput.log".
ERROR: conanfile.py: Error in build() method, line 11
        cmake.configure()
        ConanException: Error 1 while executing cd '/tmp/tmpxansakyqconans/path with spaces' && cmake -G "Unix Makefiles" -DCONAN_IN_L
OCAL_CACHE="OFF" -DCMAKE_INSTALL_PREFIX="/tmp/tmpxansakyqconans/path with spaces/package" -DCMAKE_INSTALL_BINDIR="bin" -DCMAKE_INSTALL
_SBINDIR="bin" -DCMAKE_INSTALL_LIBEXECDIR="bin" -DCMAKE_INSTALL_LIBDIR="lib" -DCMAKE_INSTALL_INCLUDEDIR="include" -DCMAKE_INSTALL_OLDI
NCLUDEDIR="include" -DCMAKE_INSTALL_DATAROOTDIR="share" -DCMAKE_MODULE_PATH="/tmp/tmpxansakyqconans/path with spaces" -DCMAKE_EXPORT_N
O_PACKAGE_REGISTRY="ON" -DCONAN_EXPORTED="1" -Wno-dev '/tmp/tmpxansakyqconans/path with spaces'

--------------------------------------------------------------------------------


----------------------------------------------------------------------
Ran 10 tests in 24.429s

FAILED (SKIP=1, errors=1)

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 24, 2020

Maybe the best default would be to use the capital version of the filename as a base for all variables ?
EDIT: cmake seems to be setting both Foo_FOUND and FOO_FOUND, so conan should probably follow this. It should be probably safe to set all other variables in both the Foo_VAR and FOO_VAR casing, don't you think ?

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 26, 2020

@ericLemanissier I tried changing it to set(HELLO_1_FOUND 1) and it didn't work. The code in Findhello_1.cmake has to be set(hello_1_FOUND 1) or else the call to find_dependency(hello_1 REQUIRED) in Findhello_2.cmake initiates a return and the targets needed in Findhello_2.cmake don't get created.

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 26, 2020

@TimSimpson I think you did not notice my edit above (?)

EDIT: cmake seems to be setting both Foo_FOUND and FOO_FOUND, so conan should probably follow this. It should be probably safe to set all other variables in both the Foo_VAR and FOO_VAR casing, don't you think ?

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 26, 2020

@ericLemanissier I was looking at the email notification when I crafted my response.

OK, then the direction is to use filename as the source of the FOUND variables, but also make an equivalent all upper case variable name (if it would be different).

I'm guessing then the _VERSION variable should use filename as well? What about _COMPONENTS?

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 27, 2020

are not _COMPONENTS variables implementation details? My take is that the generator could provide both "filename" case and upper case for all the variables documented here: https://docs.conan.io/en/latest/reference/generators/cmake_find_package.html#variables-in-find-pkg-name-cmake

Copy link
Member

@danimtb danimtb left a comment

Let's keep the variables untouched for this feature and address that separately in another PR is necessary


foreach(f ${{CONFIG_FILES}})
include(${{f}})
endforeach()
""")

#TODO: pretty sure {name}_ below will need to be the filename, but not sure
Copy link
Member

@danimtb danimtb Jul 27, 2020

Choose a reason for hiding this comment

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

remove this comment

if({{ pkg_name }}_FIND_COMPONENTS)
foreach(_FIND_COMPONENT {{ '${'+pkg_name+'_FIND_COMPONENTS}' }})
list(FIND {{ pkg_name }}_COMPONENTS_{{ build_type }} "{{ pkg_name }}::${_FIND_COMPONENT}" _index)
if({{ pkg_filename }}_FIND_COMPONENTS)
foreach(_FIND_COMPONENT {{ '${'+pkg_filename+'_FIND_COMPONENTS}' }})
list(FIND {{ pkg_filename }}_COMPONENTS_{{ build_type }} "{{ pkg_name }}::${_FIND_COMPONENT}" _index)
Copy link
Member

@danimtb danimtb Jul 27, 2020

Choose a reason for hiding this comment

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

please keep the same variables names, this change seems unrelated to the filename implementation

deps_names = ";".join(["{n}::{n}".format(n=n) for n in public_deps_names])
find_lib = target_template.format(name=pkg_findname, deps=deps,
deps_names = ";".join(["{n}::{n}".format(n=n['name']) for n in public_deps_names])
find_lib = target_template.format(name=pkg_filename, deps=deps,
Copy link
Member

@danimtb danimtb Jul 27, 2020

Choose a reason for hiding this comment

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

please keep the same name, the filename field should not affect variables

@@ -298,7 +310,7 @@ def content(self):
# Note these are in reversed order, from more dependent to less dependent
pkg_components = " ".join(["{p}::{c}".format(p=pkg_findname, c=comp_findname) for
comp_findname, _ in reversed(components)])
global_target_variables = target_template.format(name=pkg_findname, deps=pkg_info,
global_target_variables = target_template.format(name=pkg_filename, deps=pkg_info,
Copy link
Member

@danimtb danimtb Jul 27, 2020

Choose a reason for hiding this comment

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

please keep the same name, the filename field should not affect variables

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 28, 2020

@danimtb OK: all code that changed the variables to anything to do with filename has been removed, EXCEPT the _FOUND variables. Those have been changed to the filename, because if they aren't set to that things break as described above.

@ericLemanissier I also set the upper case version of the filename as well based on your request.

My assumption would be that {name}_VERSION should be {filename}_VERSION in case CMake bases any internal machinery on it, the way it does for {filename}_FOUND, but I've left it alone for now.

@uilianries I added a test for cmake_find_package_multi.

Copy link
Member

@danimtb danimtb left a comment

I really appreciate the work here and interaction in this PR and I believe we are move in the right direction to achieve a good integration with CMake.

However, I see we are trying to solve more than one thing at the same time. The purpose of this PR should be to support a different file name for the Find-cmake files and similar. Later we can discuss if the different use cases of this feature and improve the implementation evaluating the risks and without breaking.

As we want to get this merged for the next release (will be released this week), I think we should keep it simple and avoid changing the CMake variables, even the _FOUND ones, that could lead to undesired cases.

Hope it makes sense and thank you for the work and contribution!

# Global approach
set({name}_FOUND 1)
set({filename}_FOUND 1)
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

This will be braking for packages that don't use cpp_info.filename

Copy link
Contributor Author

@TimSimpson TimSimpson Jul 28, 2020

Choose a reason for hiding this comment

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

filename defaults to cpp_info.names['generator'] which defaults to cpp_info.filename, so this should be safe.

if pkg_filename != pkg_filename.upper():
set_found_filename_uppercase = 'set({}_FOUND 1)'.format(pkg_filename.upper())
else:
set_found_filename_uppercase = ''
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

This is a different issue that should be fixed in a different pull request

Copy link
Contributor Author

@TimSimpson TimSimpson Jul 28, 2020

Choose a reason for hiding this comment

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

@ericLemanissier asked that I add an upper case version here. I believe it would break anything though if I did this in another PR.

@@ -485,6 +485,101 @@ def build(self):
"$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>:>",
client.out)

def cpp_info_filename_test(self):
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

Although this test is not specific to components, I believe it will be better to move it here: https://github.com/conan-io/conan/blob/develop/conans/test/functional/generators/components/cmake_find_package_multi_test.py

client.run("create .")

client.run("new hello2/1.0 -s")
replace_in_file(
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

There are many replace_in_file() that make the test difficult to read. I'd rather use a minimal conanfile here with the components or things that are needed to the test and avoid tying the test to the conan new generated files.

print('~' * 120)
print(client.out)
print('~' * 120)
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

remove prints

print('~' * 120)
print(client.out)
print('~' * 120)
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

remove prints

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 28, 2020

As we want to get this merged for the next release (will be released this week), I think we should keep it simple and avoid changing the CMake variables, even the _FOUND ones, that could lead to undesired cases.

With all due respect, I don't think so This is an issue created by this PR, so it has to be addressed in this PR. Before this PR, FindFoo.cmake and Foo_FOUND variables always used the same case for Foo, which allowed cmake to recognize that the file was already loaded. If this PR changes the case of Foo in FindFoo.cmake but not in Foo_FOUND, then the feature is unusable, see #7320 (comment)

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 28, 2020

@ericLemanissier

If this PR changes the case of Foo in FindFoo.cmake but not in Foo_FOUND

Just to be clear, when you say the PR changes the case, I don't recall any code that explicitly changed the case here (other than the code I added to create upper case versions of XYZ_FOUND variables).

Hopefully I'm not picking a nit, but the way I'd summarize the issue is the new filename cppinfo attribute is used in calls like find_package(Foo) but the name might be something totally different, like F_O_O. If then the name is used for the FOUND variables, it leads to something like F_O_O_FOUND at which point calls to find_dependency break.

@danimtb that's why the _FOUND variables must change as part of this PR. Otherwise the feature will be unusable.

Copy link
Member

@danimtb danimtb left a comment

Thanks for the feedback and clarification. Having a look at the CMake's documentation know I understand the _found logic https://cmake.org/cmake/help/latest/command/find_package.html

However, it is not said in the documentation that both variables foo_FOUND and FOO_FOUND are set, so adding this in this PR is not clear to me. I'd suggest opening a new issue with the Freetype use case to discuss the different approaches to support it.

# Global approach
set({name}_FOUND 1)
set({filename}_FOUND 1)
{set_found_filename_uppercase}
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

please implement this as

Suggested change
{set_found_filename_uppercase}
set({filename_upper}_FOUND 1)

set({name}_VERSION "{version}")

find_package_handle_standard_args({name} REQUIRED_VARS
{name}_VERSION VERSION_VAR {name}_VERSION)
mark_as_advanced({name}_FOUND {name}_VERSION)
mark_as_advanced({filename}_FOUND {name}_VERSION)
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

Mark both variables as advanced

Suggested change
mark_as_advanced({filename}_FOUND {name}_VERSION)
mark_as_advanced({filename}_FOUND {filename_upper}_FOUND {name}_VERSION)

{% set pkg_filename_upper = pkg_filename | upper %}
{% if pkg_filename == pkg_filename_upper %}
set({ pkg_filename_upper }_FOUND 1)
{% endif %}
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

Probably defining it twice doesn't harm, so just avoid adding more templating logic

Suggested change
{% set pkg_filename_upper = pkg_filename | upper %}
{% if pkg_filename == pkg_filename_upper %}
set({ pkg_filename_upper }_FOUND 1)
{% endif %}
set({ pkg_filename | upper }_FOUND 1)

set({{ pkg_name }}_VERSION "{{ pkg_version }}")
set({{ pkg_name }}_COMPONENTS {{ pkg_components }})

find_package_handle_standard_args({{ pkg_name }} REQUIRED_VARS
{{ pkg_name }}_VERSION VERSION_VAR {{ pkg_name }}_VERSION)
mark_as_advanced({{ pkg_name }}_FOUND {{ pkg_name }}_VERSION)
mark_as_advanced({{ pkg_filename }}_FOUND {{ pkg_name }}_VERSION)
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

Suggested change
mark_as_advanced({{ pkg_filename }}_FOUND {{ pkg_name }}_VERSION)
mark_as_advanced({{ pkg_filename }}_FOUND {{ pkg_filename | upper }}_FOUND {{ pkg_name }}_VERSION)

pkg_public_deps_names = ";".join(["{n}::{n}".format(n=n) for n in pkg_public_deps])
pkg_public_deps = [
{
"name": self._get_name(self.deps_build_info[public_dep]),
Copy link
Member

@danimtb danimtb Jul 28, 2020

Choose a reason for hiding this comment

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

Is this information used at all? I see no public_dep["name"]

@danimtb
Copy link
Member

@danimtb danimtb commented Jul 29, 2020

@TimSimpson as we would like to have this ready for the 1.28 release, I am going to merge it I will improve the tests from my side, Also, I think the variable names in upper case deserve another issue to take into account the case described by @ericLemanissier regarding Freetype and other. I know there are other issues with the casing in variables such as <pkg-name>_INCLUDE_DIR and this will be another point of improvement in generators.

Hope that it makes sense and sorry for the long discussion here 😄

@danimtb danimtb merged commit efda5f0 into conan-io:develop Jul 29, 2020
2 checks passed
@maikelvdh
Copy link

@maikelvdh maikelvdh commented Jul 31, 2020

@ericLemanissier or @danimtb Is there already a new issue created for the variable naming by any chance? I am actually facing the exact same need right now myself in context of Freetype.

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 31, 2020

I did not create it yet. feel free to do it !

@TimSimpson
Copy link
Contributor Author

@TimSimpson TimSimpson commented Jul 31, 2020

@danimtb Awesome! Sorry I got unresponsive, work this last week was busy. Thanks for picking up the rest of it, I've very happy to see this merged.

kralicky added a commit to kralicky/conan that referenced this issue Aug 4, 2020
* Feature: add settings for clang-cl (clang on Windows) (conan-io#5705)

* - add settings for clang-cl (clang on Windows)

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add new VS toolsets for Clang

* fix migration test

* Feature/toolchain without dump (conan-io#7435)

* toolchain without dump()

* working

* fixing tests

* relax pluginbase requirements (conan-io#7441)

* New lockfiles iteration. Including version-only lockfiles. (conan-io#7243)

* try to reproduce error in lockfile

* iterate build_order

* package in requires and build_requires

* rename test file

* removing constraint multiple packages

* minor improvements

* first lock-recipe version working

* working

* working

* working 75%, but revisions not managed

* working

* working

* working...

* allow no user/channel match in --build

* build order

* working

* working

* working

* not working...

* working

* tests passing without revisions

* working

* review and patterns for rrevs

* new tests

* some fixes

* more fixes

* skip build-info

* fix revision test msg

* removed global activation of revisions

* multiple br test

* cli changes

* working

* new cli syntax

* fixing tests

* partial lockfiles

* CI partial lock working

* make sure partial lockfiles are used completely and from its root

* version ranges and py_requires working

* partial CI with python_requires

* comment

* tests passing

* working

* working in build_requires

* CI for build-requires test

* recovering conan_build_info tests

* broken py27

* clean-modified, stricter locks

* partial arranged

* minor style fixes

* compacting lockfile search for nodes

* remove unnecessary --build=missing

* processing --update

* testing the --update necessary for revisions non matching

* make the lockfile-out compulsory

* fixed conan_build_info

* forcing --lockfile-out

* workspace change

* Update conans/client/command.py

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>

* make lock_node.path relative

* new tests and fixes

* remove hardcoded os.environ

* make lockfile-out required

* fix test

* fixing os.path.relpath in Win

* more tests

* fix test

* fix test

* checking --lockfile-out requires --lockfile

* working

* review

* rename get_consumer

* a partial lock can match more than 1 require

* error msg

* working

* check that profile is not passed when using lockfiles

* working

* working

* recover profile_build from lockfile

* add new test for --update

* make sure modified is propagated

* with revisions

* format

* changed build-order order of output

* update cmd line

* review

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* forbidding version ranges when using lockfiles, in command line

Co-authored-by: jgsogo <jgsogo@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Support for components in pkg_config generator (conan-io#7413)

* Support for components in pkg_config generator

* capitalize

* remove code not needed

* use real pkg-config for tests

* try without skip

* [feature] Define file name for CMake find generators (conan-io#7320)

* adds `filenames` to cpp_info

Relates to [this issue](conan-io#7254)

* use `filenames` attribute, not name, as prefix to global vars

* `filenames` should fall back to `names`, not package name

* removes debug statements

* Makes `cmake_find_package_multi` work with `filenames` attribute

* adds a test for the filename attribute

* Update find_package generators to not use `filename` in global var names

* change `name` used in cmake_find_package_test, since sharing them isn't possible now

* Set both `{name}_FOUND` and `{filename}_FOUND` vars so code will work

* reverts `filename` of `name` in some spots (danimtb feedback)

* fixes bug in cmake_fine_package_multi_test

* adds a test for changing filename in cmake_find_package_multi generator

* use filename version of _FOUND in if statements...

This allows CMake exported target namespaces to be shared.

* set upper case version of _FOUND vars

* Add Conan version to HTML output (conan-io#7443)

* conan-io#7365 Add Conan version in search table

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#7365 Add Conan version in info graph

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#7365 Add shebang for python2.7

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#7365 Use JS for current date

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#7365 Do not rename old tests

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#7365 Remove copyright symbol to allow py27

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Refactor filenames and simplify tests (conan-io#7447)

* Refactor filenames and simplify tests

* fix

* small fixes

* review fix

* rename variables

* fix linux tests

* version 1.28.0

* version 1.29.0-dev

* fix migration test

* Hotfix/lockfile errors (conan-io#7453)

* improve error msgs

* better error msgs

* minor fixes to error msgs and better checks

* Fixes bad powershell separators on Linux (conan-io#7472)

* minor improvements in components checks (conan-io#7486)

* fix message missing components (conan-io#7483)

* fix message missing components

* changed error message

Co-authored-by: SSE4 <tomskside@gmail.com>
Co-authored-by: memsharded <james@conan.io>
Co-authored-by: David Roman <davidroman96@gmail.com>
Co-authored-by: jgsogo <jgsogo@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Daniel <danimanzaneque@gmail.com>
Co-authored-by: Tim Simpson <timsimpson4@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: mjvankampen <mjvk@allseas.com>
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.

None yet

8 participants