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

#5958 New tool: cppstd minimum version required #5997

Merged
merged 33 commits into from
Dec 3, 2019

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Oct 29, 2019

Add a new tool: cppstd_minimum_version:

  • Receive a ConanFile instance and handle its settings to detect the current cppstd, from profile or from arguments.
  • If it is not able to detect by settings, then it will extract from current compiler (from profile) and compare if such compiler version is able to manipulate the required cppstd.
  • Any failure will result in an InvalidConfiguration error.
  • Why not only returning False and the user take care about the exception?
    Because the error can be caused by compiler or settings, so we would need to return 2 elements (result + message) and the user will (probably) forward the message as an InvalidConfiguration.

NOTE: This feature should be affected by #5440

Changelog: Feature: New tools.check_min_cppstd and tools.valid_min_cppstd to check if the cppstd version is valid for a specific package.
Docs: conan-io/docs#1467

closes #5958
closes #4943
closes #5440

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

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@lasote
Copy link
Contributor

lasote commented Nov 7, 2019

@jgsogo please take care of this PR following the latest agreement in the issue. Thanks

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

uilianries commented Nov 7, 2019

I need to update this PR, Memsharded commented sometime ago about other cppstd forms: gnu++20 and c++2x. This PR is NOT prepared to deal with exotics extensions.

UPDATE:

Conan uses alias for gnu extensions, we don't need extra settings.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@lasote
Copy link
Contributor

lasote commented Nov 11, 2019

Have you checked my last comment here? #4943

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

Yes, the implementation for valid_min_cppstd is here

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I'm asking a few changes to match the interface requested in the issue.

Also, I would like to suggest a different approach to testing: I really appreciate all these exhaustive testing, but I'd write them like unittests of the functions valid_min_cppstd and check_min_cppstd (mocking cppstd_from_settings and cppstd_flag), otherwise we are checking many other things that haven't been touched in this PR, that are already tested and we are just consuming time.

After the exhaustive ones, we should write one/two tests using an actual ConanFile loaded from disk with the TestClient to check that the full workflow is not broken in the future.

Thanks!

conans/client/tools/settings.py Outdated Show resolved Hide resolved
conans/client/tools/settings.py Outdated Show resolved Hide resolved
conans/tools.py Show resolved Hide resolved
conans/tools.py Outdated Show resolved Hide resolved
conans/tools.py Outdated Show resolved Hide resolved
conans/client/tools/settings.py Outdated Show resolved Hide resolved
@jgsogo
Copy link
Contributor

jgsogo commented Nov 11, 2019

this one should close (if merged) #4943 too.

uilianries and others added 10 commits November 11, 2019 13:55
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@jgsogo jgsogo assigned lasote and unassigned jgsogo Nov 22, 2019
conans/client/tools/settings.py Outdated Show resolved Hide resolved
conans/test/unittests/client/tools/cppstd_required_test.py Outdated Show resolved Hide resolved
conans/client/tools/settings.py Outdated Show resolved Hide resolved
- Raise ConanExcetion when some input is incorrect

Signed-off-by: Uilian Ries <uilianries@gmail.com>
- The target OS should be considered as important for cpp version

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@@ -28,8 +31,14 @@ def add_millennium(cppstd):
rhs = add_millennium(extract_cpp_version(rhs))
return lhs < rhs

def is_linux(conanfile):
os = conanfile.settings.get_safe("os_build")
Copy link
Member

Choose a reason for hiding this comment

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

Still, the C++ standard is the host one, not the build one. This is why the OSInfo() cannot be used!

@memsharded
Copy link
Member

This PR is working so far with wrong hypothesis regarding the settings model and the ABI compatibility.
Maybe that model can be reconsidered, I have created an issue for it: #6157

In the meantime, this should be either adapted to the current model, or put on hold until the underlying problem is addressed.

@jgsogo
Copy link
Contributor

jgsogo commented Nov 28, 2019

Wow! Good catch, @memsharded!

Here we have another use case where we need the settings even though the recipe has removed all of them. It looks like that the approach for header-only libraries should preserve the settings but apply the header_only() trick in the package_id...

Nevertheless, regarding this PR, if the recipe uses the cppstd_minimum_version it should have at least the compiler setting to access compiler.cppstd and, probably, if it requires the compiler, we can require the os to be there. So, I would buy an error like: "in order to check the C++ standard, the recipe must contain os and compiler settings" . WDYT?

@uilianries
Copy link
Member Author

Yes, without settings (compiler) will be impossible to use this feature.

@memsharded
Copy link
Member

Nevertheless, regarding this PR, if the recipe uses the cppstd_minimum_version it should have at least the compiler setting to access compiler.cppstd and, probably, if it requires the compiler, we can require the os to be there. So, I would buy an error like: "in order to check the C++ standard, the recipe must contain os and compiler settings" . WDYT?

Yes, regarding this feature, this would be the correct solution. But the problem is larger that this.

Absent, as already merged in Conan-center-index, is wrong: https://github.com/conan-io/conan-center-index/tree/master/recipes/absent/all.

This feature will fail if applied to that recipe, and if we force that recipe to be created with compiler.cppstd=17, then, it will not be possible to use it, because most packages in conan-center will be missing, as they were not compiled with that setting. Either we build the whole repo with compiler.cppstd=17, or we need to relax the assumptions about binary compatibility of the cppstd standards for the same compiler version.

@jgsogo
Copy link
Contributor

jgsogo commented Nov 28, 2019 via email

@lasote lasote modified the milestone: 1.21 Dec 2, 2019
@lasote
Copy link
Contributor

lasote commented Dec 2, 2019

What we want from this PR right now:

  1. If self.settings.compiler.cppstd, the tool will use self.settings.compiler.cppstd to compare
  2. It not self.settings.compiler.cppstd, the tool will use compiler to compare (reading the default from cppstd_default)
  3. If not self.settings.compiler is present (not declared in settings) will raise because it cannot compare.

This WON'T solve the issue with c3i, where we need a way to alter the input configuration, but that's a different issue.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

Done! I've followed the comment #5997 (comment)

lasote and others added 6 commits December 3, 2019 12:33
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.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
4 participants