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.

Sign up for GitHub

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

Jump to bottom

[entt] Tool check_min_cppstd already validates default 'cppstd' values #3840

Merged
merged 5 commits into from
Dec 16, 2020
Merged

[entt] Tool check_min_cppstd already validates default 'cppstd' values #3840

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3fb476
simplify min cppstd check
jgsogo Dec 10, 2020
4c369fe
use VisualStudio 15 as base reference
jgsogo Dec 10, 2020
7ee396e
assume minor version satisfies
jgsogo Dec 10, 2020
f618a70
Update recipes/entt/3.x.x/conanfile.py
jgsogo Dec 11, 2020
761799e
remove opinionated fixme, cast to int
jgsogo Dec 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions recipes/entt/3.x.x/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def _source_subfolder(self):
return "source_subfolder"

def configure(self):
# FIXME: Here we are implementing a workaround because ConanCenter is not generating any configuration with
# C++17 support (either supported by default by a compiler or using 'compiler.cppstd=17' in the Conan profile)
# and ConanCenter requires that at least one package is generated. Once ConanCenter uses a profile
# with C++17 support, only a raw call to `tools.check_mis_cppstd(self, "17")` will be required.
Copy link
Contributor

@SpaceIm SpaceIm Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why this comment by the way? All CCI recipes requiring at least C++14 have this kind of pattern.
Do you mean that in future conan version, a profile without compiler.cppstd=17 (or higher) won't be able to consume or build a library requiring at least C++17, so we won't be able to mix different standard in the dependency graph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The C++ standard should be known by Conan in order to select the proper binary. Once we start to generate here binaries for stable C++ standards, recipes might look like the following:

  • A recipe that needs C++17 to build, but the API is C++11 compatible:
class Recipe:

def validate(self):
    tools.check_min_cppstd(self, "17")  # This is needed to build

def package_id(self):
    # If the API is compatible for >=C++11, we can fallback to any binary available
    for cppstd in ("11", "14", "17", "20"):
        compatible_pkg = self.info.clone()
        compatible_pkg.settings.compiler.cppstd = cppstd
        self.compatible_packages.append(compatible_pkg)

So, a profile without compiler.cppstd will be suitable to consume this library if the default for the compiler is at least C++11, but only a profile with explicit compiler.cppstd=17 or compiler.cppstd=20 will be able to generate this library.

Behind the scenes, Conan keeps the exception raises by the check_min_cppstd and only raise it if the command is trying to build the library, otherwise this package remains in the graph with package_id = INVALID (that would be the output for conan info command), and it will fall back to the compatible package if we need a binary to install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this implementation, you can mix binaries from different C++ standards because they are declared compatible not just because a hidden CXX_STANDARD 17 in the CMakeLists.txt was able to generate the binary. I think it is preferred to make it explicit.

Copy link
Contributor

@SpaceIm SpaceIm Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

And if default standard of a compiler version is lower than the minimum standard required by the library, but still supports enough features of this standard for this library? Will it work if standard is not explicitly set in profile?

Moreover, AFAIK tools.check_min_cppstd(self, "17") raises if standard is not set in profile, so it requires modifications in this function.

What happen if a consumer tries to build from source with an unsupported compiler? Currently, it gracefully fails, while with this implementation it will fail at some point during build()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if default standard of a compiler version is lower than the minimum standard required by the library, but still supports enough features of this standard for this library? Will it work if standard is not explicitly set in profile?

We cannot handle this situation, if the recipe says tools.check_min_cppstd(self, "17") it will fail if the compiler doesn`t implement that standard (if it is declared stable). If it only needs some features that are available in previous versions, the recipe will need some handcrafted checks... but I would try to avoid them as much as possible (in my 🦄 world).

Moreover, AFAIK tools.check_min_cppstd(self, "17") raises if standard is not set in profile, so it requires modifications in this function.

No, it doesn't (https://github.com/conan-io/conan/blob/develop/conans/client/tools/settings.py#L5). You saying that is a symptom of the workaround we are using in the recipe to bypass the fact that ConanCenter is not trying any profile with C++14/17/20 support.

What happen if a consumer tries to build from source with an unsupported compiler? Currently, it gracefully fails, while with this implementation it will fail at some point during build()

It will fail gracefully, have a look at the docs about validate() function: https://docs.conan.io/en/latest/reference/conanfile/methods.html?highlight=validate#validate

Copy link
Contributor

@SpaceIm SpaceIm Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, but if compiler.cppstd is not set in profile, tools.check_min_cppstd raises if default standard -as estimated by conan- is not sufficient.
https://github.com/conan-io/conan/blob/61d75d945a0e16a56a320926789cff2d297ac76a/conans/client/tools/settings.py#L50
https://github.com/conan-io/conan/blob/61d75d945a0e16a56a320926789cff2d297ac76a/conans/client/build/cppstd_flags.py#L50

Currently, tools.check_min_cppstd(self, 17) raises with all versions of Visual Studio -even Visual Studio 2019- if compiler.cppstd is not set.
https://github.com/conan-io/conan/blob/61d75d945a0e16a56a320926789cff2d297ac76a/conans/client/build/cppstd_flags.py#L78

I believe that, if your first answer in the previous message was implemented as is in CCI recipes, it would break a lot of them (a lot of C++11 recipes work with gcc 4.9/5, while C++11 is not the default standard of those versions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If compiler.cppstd is not set in the profile it will fail to build, yes. And it is right because the recipe says C++17 is required and those compilers don't enable C++17 support by default. Activating the standard in the CMakeLists.txt file is something to avoid, it is hiding changes in the ABI from the package manager.

But, by adding the compatible_packages feature, Conan will be able to retrieve a compatible binary that was compiled with an explicit compiler.cppstd value when using a profile without it.

I believe that, if your first answer in the previous message was implemented as is in CCI recipes, it would break a lot of them (a lot of C++11 recipes work with gcc 4.9/5, while C++11 is not the default standard of those versions).

As they are now, those recipes are wrong (because of c3i profiles), they are activating C++11 inside the CMakeLists.txt, and some of the header-only ones are activating it inside the test_package/CMakeLists.txt (if you add them to your graph, Conan retrieves a package and then your consumer fails to compile without further notice). If you modify those recipes as the draft in the first commit they will fail to build if you do not provide a compatible configuration in the profile (nothing hidden) but you still can consume them with a lower C++ standard if the API is compatible and the binary is available (it was compiled with a higher C++ standard).

Maybe this requires a written example

Copy link
Contributor

@SpaceIm SpaceIm Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If compiler.cppstd is not set in the profile it will fail to build, yes. And it is right because the recipe says C++17 is required and those compilers don't enable C++17 support by default. Activating the standard in the CMakeLists.txt file is something to avoid, it is hiding changes in the ABI from the package manager.

I strongly disagree.

It's not wrong to activate C++11/14/17 in CMakeLists.txt, it's what all C++11/14/17 libraries in the world do. I don't understand your point. It fails in test_package if C++ standard is not set, because conan doesn't provide a way (or if it does, nobody use it in CCI recipes) to easily propage this information in generators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've written an example with a (opinionated) dramatization here: conan-io/examples#73. I know every library activates the C++ standard in the CMakeLists.txt but IMHO it is not the best place to do it. We can move this conversation there (or to another repository) with an example, it is easier for me to show what I want to show.

I will remove the comment in this recipe as this is something we cannot fix right now, but I really think user experience and ConanCenter would benefit from that approach. Please have a look at the recipes in that PR. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks, I'll take a look ;)

minimal_cpp_standard = "17"
if self.settings.compiler.cppstd:
tools.check_min_cppstd(self, minimal_cpp_standard)
Expand All @@ -38,8 +42,14 @@ def configure(self):
"%s requires a compiler that supports at least C++%s" % (self.name, minimal_cpp_standard))
return

version = tools.Version(self.settings.compiler.version)
if version < minimal_version[compiler]:
# Compare versions asuming minor satisfies if not explicitly set
def lazy_lt_semver(v1, v2):
lv1 = v1.split(".")
lv2 = v2.split(".")
jgsogo marked this conversation as resolved.
Show resolved Hide resolved
min_length = min(len(lv1), len(lv2))
return lv1[:min_length] < lv2[:min_length]

if lazy_lt_semver(str(self.settings.compiler.version), minimal_version[compiler]):
raise ConanInvalidConfiguration(
"%s requires a compiler that supports at least C++%s" % (self.name, minimal_cpp_standard))

Expand Down