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

Provide _INCLUDE_DIR in cmake_find_package generator #6017

Merged
merged 4 commits into from Dec 3, 2019

Conversation

theirix
Copy link
Contributor

@theirix theirix commented Nov 1, 2019

Changelog: Feature: Provide _INCLUDE_DIR variables in the cmake_find_package generator
Docs: omit

This affects at least ZLIB (a lot of package check ZLIB_INCLUDE_DIR), openjpeg.

Closes: #6011

  • 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.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

Copy link
Member

@memsharded memsharded left a comment

Looks reasonable, but would like review from @lasote

@@ -58,8 +58,13 @@ def _find_for_dep(self, name, cpp_info):
# Here we are generating FindXXX, so find_modules=True
public_deps_names = [self.deps_build_info[dep].name for dep in cpp_info.public_deps]
lines = find_dependency_lines(name, public_deps_names, find_modules=True)
# get {{name}}_INCLUDE_DIR as a first include path if exists
if cpp_info.include_paths:
include_dir = cpp_info.include_paths[0]
Copy link
Member

@memsharded memsharded Nov 1, 2019

Choose a reason for hiding this comment

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

It seems a bit arbitrary that just the first path is used. Can't INCLUDE_DIR variable contain more than one path?

Copy link
Contributor Author

@theirix theirix Nov 2, 2019

Choose a reason for hiding this comment

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

Good point, it can have more than one path. That's because cmake encourage a user to use INCLUDE_DIRS. Usually, it will work but multiple paths can be found by complex modules such as FindJava and FindFreetype.
I think we can provide a list as a string joined by semicolons (as recommended in forums and some modules):

set(FREETYPE_INCLUDE_DIRS "${FREETYPE_INCLUDE_DIR_ft2build};${FREETYPE_INCLUDE_DIR_freetype2}")

Copy link
Contributor Author

@theirix theirix Nov 2, 2019

Choose a reason for hiding this comment

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

Added a commit that builds a string from all include paths. It is very common with INCLUDE_DIRS but semicolon-separated because I think consuming packages can be surprised by a newline-separated list in INCLUDE_DIR.

Copy link
Contributor

@lasote lasote Nov 4, 2019

Choose a reason for hiding this comment

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

I don't like it much, I think it is a bit complex the new variable and the replacements. What if we generate INCLUDE_DIR when there is only one include dir only? @memsharded Rarely the packages declare more than one, and in these cases, they are managing INCLUDE_DIRS anyway...

Copy link
Contributor Author

@theirix theirix Nov 4, 2019

Choose a reason for hiding this comment

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

The original variant of PR generated a variable with the first include directory only and it covered 99% cases. But I thought it would be better to support remaining cases. If the former variant is more clear, I can revert the second commit.

Unfortunately, the problem is not in a package that produces variables (it will provide INCLUDE_DIRS for multiple directories of course) but in a bad-written package that consumes those variables. Many real-world packages use old-style _INCLUDE_DIR instead of INCLUDE_DIRS, even if there is only one directory.

Just for information, in case of a single include directory the generated cmake file will contain two same definitions:

set(LIBFOO_INCLUDE_DIRS "/path/to/included/dir")
set(LIBFOO_INCLUDE_DIR "/path/to/included/dir")

For two directories:

set(LIBFOO_INCLUDE_DIRS "/path/to/included/dir"
    "/path/to/included/dir2")
set(LIBFOO_INCLUDE_DIR "/path/to/included/dir;/path/to/included/dir2")

if cpp_info.include_paths:
include_dir = cpp_info.include_paths[0]
else:
include_dir = '""'
Copy link
Member

@memsharded memsharded Nov 1, 2019

Choose a reason for hiding this comment

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

What if include_dir is just empty instead of ""? The final var:

set(ZLIB_INCLUDE_DIR)  # Good
#no need of 
set(ZLIB_INCLUDE_DIR "") 

Copy link
Contributor Author

@theirix theirix Nov 2, 2019

Choose a reason for hiding this comment

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

This may be not actual with a new commit that uses all include paths

@memsharded memsharded requested a review from lasote Nov 1, 2019
Variable _INCLUDE_DIR may be expected to be a pure string (without newlines).
So build it as semicolon-separated string that is essentially a CMake list.
@lasote lasote added this to the 1.21 milestone Nov 4, 2019
@lasote lasote self-assigned this Nov 4, 2019
@lasote
Copy link
Contributor

@lasote lasote commented Dec 3, 2019

Hey @theirix. I've pushed a commit extracting the "path-merge-trick" to the model we are using for the CMake generators. Now it is a bit cleaner. I finally agree with you that we can cover 100% of cases. Thanks for your help!

lasote
lasote approved these changes Dec 3, 2019
@lasote lasote merged commit 608ccb0 into conan-io:develop Dec 3, 2019
2 checks passed
@theirix
Copy link
Contributor Author

@theirix theirix commented Dec 3, 2019

@lasote thanks, that looks really better! I doubted about introducing new attributes in DepsCppCmake. Should we document this new field in https://docs.conan.io/en/latest/reference/conanfile/attributes.html#deps-cpp-info ?

@lasote
Copy link
Contributor

@lasote lasote commented Dec 4, 2019

No, it is not the same model. The CMake one is just a wrapper for preparing the cppinfo data to something more chewed for the cmake generators.

@theirix
Copy link
Contributor Author

@theirix theirix commented Dec 4, 2019

ah, yes, confused by the similar name

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