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

jgsogo · 2020-12-10T08:29:35Z

Specify library name and version: entt/all

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

conan-center-bot · 2020-12-10T08:48:11Z

Some configurations of 'entt/3.2.2' failed in build 1 (a3fb476a0fd7fb7fd3c0865672830d82f580cb87):

Linux x86_64, Release, gcc 4.9, libstdc++
Access to all the logs

SpaceIm · 2020-12-10T08:48:27Z

It does not check C++ standard support of compilers (which can be complex, because a partial support of C++ standard for some library might be sufficient, while not for another library using others features).

SSE4 · 2020-12-10T08:55:07Z

yes, that might be harder than it initially appears. many compilers declare support for particular standards, but either does not fully support them or have some bugs in the implementation. it would be too much for conan to validate the support (as it will require thousands of test cases).

SpaceIm · 2020-12-10T08:56:59Z

For this particular recipe (in fact in all recipes with this pattern), Visual Studio min version should be set to 15 instead of 15.9, since settings.yml doesn't provide such granularity, to avoid excuding Visual Studio 15. Even for others compilers, it's not uncommon to not use minor version in its profile.
Or maybe we should modify this pattern to consider that a self.settings.compiler.version with major only is greater or equal than major[.minor][.patch]

jgsogo · 2020-12-10T09:55:56Z

The underlying issue is c3i itself, we are not iterating any profile with C++17 enabled and we need to write these workarounds in the recipes. After adding the new configurations for new gcc, clang and apple-clang versions that we are using to learn how to add packages for new configurations while merging other PRs, we need to add some profiles with C++XX support.

jgsogo · 2020-12-10T09:56:33Z

Using VisualStudio 15 should be enough to "fix" the issue reported here: skypjack/entt#606

SpaceIm · 2020-12-10T10:08:28Z

Here is a simple function which could allow to check compiler version against a min version:

def compare_compiler_version(version1, version2):
    v1, *v1_res = version1.split(".", 1)
    v2, *v2_res = version2.split(".", 1)
    if v1 == v2 and v1_res and v2_res:
        return compare_compiler_version(v1_res[0], v2_res[0])
    return v1 >= v2

checks = [
    ["15", "14"],
    ["15", "15"],
    ["15", "16"],
    ["15", "15.1"],
    ["14", "15.1"],
    ["16", "15.1"],
    ["16", "15.8.9"],
    ["14.1.5", "15.1"],
    ["15.1.7", "15.1"],
    ["15.1.7", "15.1.8"],
    ["15.1.9", "15.1.8"]
]

for check in checks:
    print("version {0} >= version {1}: {2}".format(check[0], check[1], compare_compiler_version(check[0], check[1])))

output:

version 15 >= version 14: True
version 15 >= version 15: True
version 15 >= version 16: False
version 15 >= version 15.1: True
version 14 >= version 15.1: False
version 16 >= version 15.1: True
version 16 >= version 15.8.9: True
version 14.1 >= version 14.1.5: True
version 14.1 >= version 15.1.5: False
version 14.1.5 >= version 15.1: False
version 15.1.7 >= version 15.1: True
version 15.1.7 >= version 15.1.8: False
version 15.1.9 >= version 15.1.8: True

This way, we can keep the detailed information about min compiler version, while allowing to have a less accurate version in profile.

conan-center-bot · 2020-12-10T10:15:06Z

All green in build 2 (4c369fede26e76a9833bfa06100c95678d7d6bb0)! 😊

entt/3.2.2: Generated 1 packages. All logs here
entt/3.3.0: Generated 1 packages. All logs here
entt/3.3.1: Generated 1 packages. All logs here
entt/3.3.2: Generated 1 packages. All logs here
entt/3.4.0: Generated 1 packages. All logs here
entt/3.5.0: Generated 1 packages. All logs here
entt/3.5.1: Generated 1 packages. All logs here
entt/3.5.2: Generated 1 packages. All logs here

jgsogo · 2020-12-10T10:43:34Z

@SpaceIm , I see the intention, but having a look at Conan sources I'm afraid using compiler.version=15.9 won't behave as expected (sometimes we look for an exact match and we expect to find strictly "15"). In Conan v2.0 we will try to move to a new msvc compiler that should model the version of the compiler itself, so probably we are not going to fix these issues with compiler=Visual Studio unless it is very needed.

Nevertheless, Conan behavior doesn't justify a wrong implementation here. I can add your smart version-checker to the recipe.

SpaceIm · 2020-12-10T10:51:19Z

Yes my intention was to add this function for these specific checks in the recipe, not in conan library itself.

This issue is not specific to Visual Studio. Loot at #3320, cpp-sort asks for gcc 5.5 as a minimum in configure(), but gcc-6 package was generated by CI, I'm pretty sure that gcc-5 in profile raises an error.

jgsogo · 2020-12-10T11:01:44Z

Let's see if we can allocate some time and prioritize the profiles using other C++ standards, I think it is totally needed and now we are at the point where we can start to implement these things more easily. But more profiles are more resources, more time and more chances to fail unexpectedly... more fun! 😅

conan-center-bot · 2020-12-10T11:13:24Z

An unexpected error happened and has been reported. Help is on its way! 🏇

conan-center-bot · 2020-12-10T11:45:33Z

All green in build 4 (7ee396e2ad5534984349e02d82cce182003f08ea)! 😊

entt/3.2.2: Generated 1 packages. All logs here
entt/3.3.0: Generated 1 packages. All logs here
entt/3.3.1: Generated 1 packages. All logs here
entt/3.3.2: Generated 1 packages. All logs here
entt/3.4.0: Generated 1 packages. All logs here
entt/3.5.0: Generated 1 packages. All logs here
entt/3.5.1: Generated 1 packages. All logs here
entt/3.5.2: Generated 1 packages. All logs here

SSE4 · 2020-12-10T11:48:46Z

recipes/entt/3.x.x/conanfile.py

-        if version < minimal_version[compiler]:
+        # Compare versions asuming minor satisfies if not explicitly set
+        def gte_compiler_version(version1, version2):
+            v1, *v1_res = version1.split(".", 1)


I don't like this implementation as it may lead into infinite loops. there is a Version class for that.

What kind of input could lead to infinite loop (empty string maybe)? Can Version handle the logic implemented in this function?

The Version class will consider minor equals to 0 if it is not set, which is exactly the opposite of what we are doing here. The expression tools.Version(version1) >= tools.Version(version2) doesn't work the same.

About infinite recursion... it would require infinite . in both versions

SpaceIm · 2020-12-10T15:03:57Z

recipes/entt/3.x.x/conanfile.py

+        # 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.


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?

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.

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.

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()

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

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

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

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.

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!

Ok thanks, I'll take a look ;)

recipes/entt/3.x.x/conanfile.py

Co-authored-by: SpaceIm <[email protected]>

conan-center-bot · 2020-12-11T08:08:14Z

All green in build 5 (f618a70ef5033ae294936e2feb90b425f664fa80)! 😊

entt/3.2.2: Generated 1 packages. All logs here
entt/3.3.0: Generated 1 packages. All logs here
entt/3.3.1: Generated 1 packages. All logs here
entt/3.3.2: Generated 1 packages. All logs here
entt/3.4.0: Generated 1 packages. All logs here
entt/3.5.0: Generated 1 packages. All logs here
entt/3.5.1: Generated 1 packages. All logs here
entt/3.5.2: Generated 1 packages. All logs here

recipes/entt/3.x.x/conanfile.py

conan-center-bot · 2020-12-12T17:53:35Z

All green in build 6 (761799ecb46ca64707571d5b97d8d29d21811aa1)! 😊

entt/3.2.2: Generated 1 packages. All logs here
entt/3.3.0: Generated 1 packages. All logs here
entt/3.3.1: Generated 1 packages. All logs here
entt/3.3.2: Generated 1 packages. All logs here
entt/3.4.0: Generated 1 packages. All logs here
entt/3.5.0: Generated 1 packages. All logs here
entt/3.5.1: Generated 1 packages. All logs here
entt/3.5.2: Generated 1 packages. All logs here

uilianries

Good enough for this situation.

malachib · 2020-12-23T16:32:35Z

👍 thank you all for your hard work on this. Since I still am experiencing:

ERROR: entt/3.5.2: Invalid configuration: entt requires a compiler that supports at least C++17

and/or:

ERROR: Invalid setting '15.9' is not a valid 'settings.compiler.version' value.
Possible values are ['10', '11', '12', '14', '15', '16', '8', '9']

I deduce that either:

I'm doin' this totally wrong
Some conan upgrades/merges are still needing to happen

Please advise if it's 1, 2 or... something else :)

uilianries · 2020-12-23T17:10:31Z

+1 thank you all for your hard work on this. Since I still am experiencing:

ERROR: entt/3.5.2: Invalid configuration: entt requires a compiler that supports at least C++17

and/or:
ERROR: Invalid setting '15.9' is not a valid 'settings.compiler.version' value.
Possible values are ['10', '11', '12', '14', '15', '16', '8', '9']
I deduce that either:
1. I'm doin' this totally wrong

2. Some conan upgrades/merges are still needing to happen
Please advise if it's 1, 2 or... something else :)

@malachib Take a look on this answer: https://stackoverflow.com/questions/64694269/getting-uwebsockets-for-visual-2017-via-conan It looks pretty similar. If it doesn't work, please open an issue.

jgsogo · 2020-12-23T17:11:11Z

Hi, @malachib

You are facing a problem not related to this recipe. Conan has a file listing all the settings and their values, have a look at your cache folder (conan config home), you will find a file like <cache-folder>/settings.yml. These settings are distributed together with Conan, they are default, but they should fit most of the configurations out there.

You can modify them to fit the needs of your company, you can add 15.9 to the list of versions for Visual Studio, but then Conan will generate a different package ID for 15 and 15.9. It allows you fine-grained control, but maybe this is not what you are looking for, typically 15 is enough as all those binaries will be ABI compatible.

So, use a profile like this one (notice compiler.version):

[settings]
os=Windows
arch=x86_64
compiler=Visual Studio
compiler.version=15
compiler.runtime=MT
build_type=Release

conan install entt/3.5.2@ -r conan-center --profile=that-profile --update

malachib · 2020-12-23T18:29:16Z

The magic for me appears to be your suggested --update portion. In my profile I've also got specified cppstd=17.

Subsequent unadorned conan install work well

I now have EnTT running on VS2017! I also never knew settings.yml could be tweaked that way, so thank you for that insight as well

simplify min cppstd check

a3fb476

conan-center-bot added the Failed label Dec 10, 2020

use VisualStudio 15 as base reference

4c369fe

conan-center-bot removed the Failed label Dec 10, 2020

assume minor version satisfies

7ee396e

conan-center-bot added the Unexpected Error label Dec 10, 2020

conan-center-bot removed the Unexpected Error label Dec 10, 2020

SSE4 reviewed Dec 10, 2020

View reviewed changes

jgsogo requested a review from czoido December 10, 2020 13:45

jgsogo marked this pull request as ready for review December 10, 2020 13:45

jgsogo requested review from SSE4 and uilianries December 10, 2020 13:45

czoido previously approved these changes Dec 10, 2020

View reviewed changes

SpaceIm reviewed Dec 10, 2020

View reviewed changes

recipes/entt/3.x.x/conanfile.py Outdated Show resolved Hide resolved

Update recipes/entt/3.x.x/conanfile.py

f618a70

Co-authored-by: SpaceIm <[email protected]>

jgsogo dismissed czoido’s stale review via f618a70 December 11, 2020 07:48

jgsogo requested a review from czoido December 11, 2020 08:16

czoido previously approved these changes Dec 11, 2020

View reviewed changes

SpaceIm previously approved these changes Dec 11, 2020

View reviewed changes

SpaceIm reviewed Dec 12, 2020

View reviewed changes

recipes/entt/3.x.x/conanfile.py Outdated Show resolved Hide resolved

remove opinionated fixme, cast to int

761799e

jgsogo dismissed stale reviews from SpaceIm and czoido via 761799e December 12, 2020 17:33

SpaceIm approved these changes Dec 12, 2020

View reviewed changes

jgsogo requested a review from czoido December 14, 2020 15:54

uilianries approved these changes Dec 16, 2020

View reviewed changes

SSE4 approved these changes Dec 16, 2020

View reviewed changes

conan-center-bot merged commit a2e4d4c into conan-io:master Dec 16, 2020

jgsogo deleted the fix/entt branch December 23, 2020 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jgsogo commented Dec 10, 2020 •

edited

Loading

conan-center-bot commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 •

edited

Loading

SSE4 commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 •

edited

Loading

jgsogo commented Dec 10, 2020

jgsogo commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 •

edited

Loading

conan-center-bot commented Dec 10, 2020

jgsogo commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 •

edited

Loading

jgsogo commented Dec 10, 2020

conan-center-bot commented Dec 10, 2020

conan-center-bot commented Dec 10, 2020

SSE4 Dec 10, 2020

SpaceIm Dec 10, 2020 •

edited

Loading

jgsogo Dec 10, 2020

SpaceIm Dec 10, 2020 •

edited

Loading

jgsogo Dec 10, 2020

jgsogo Dec 10, 2020

SpaceIm Dec 10, 2020 •

edited

Loading

jgsogo Dec 10, 2020

SpaceIm Dec 10, 2020 •

edited

Loading

jgsogo Dec 11, 2020

SpaceIm Dec 11, 2020 •

edited

Loading

jgsogo Dec 12, 2020

SpaceIm Dec 12, 2020

conan-center-bot commented Dec 11, 2020

conan-center-bot commented Dec 12, 2020

uilianries left a comment

malachib commented Dec 23, 2020

uilianries commented Dec 23, 2020 •

edited

Loading

jgsogo commented Dec 23, 2020

malachib commented Dec 23, 2020

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

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

Conversation

jgsogo commented Dec 10, 2020 • edited Loading

conan-center-bot commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 • edited Loading

SSE4 commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 • edited Loading

jgsogo commented Dec 10, 2020

jgsogo commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 • edited Loading

conan-center-bot commented Dec 10, 2020

jgsogo commented Dec 10, 2020

SpaceIm commented Dec 10, 2020 • edited Loading

jgsogo commented Dec 10, 2020

conan-center-bot commented Dec 10, 2020

conan-center-bot commented Dec 10, 2020

Choose a reason for hiding this comment

SpaceIm Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 11, 2020

conan-center-bot commented Dec 12, 2020

uilianries left a comment

Choose a reason for hiding this comment

malachib commented Dec 23, 2020

uilianries commented Dec 23, 2020 • edited Loading

jgsogo commented Dec 23, 2020

malachib commented Dec 23, 2020

jgsogo commented Dec 10, 2020 •

edited

Loading

SpaceIm commented Dec 10, 2020 •

edited

Loading

SpaceIm commented Dec 10, 2020 •

edited

Loading

SpaceIm commented Dec 10, 2020 •

edited

Loading

SpaceIm commented Dec 10, 2020 •

edited

Loading

SpaceIm Dec 10, 2020 •

edited

Loading

SpaceIm Dec 10, 2020 •

edited

Loading

SpaceIm Dec 10, 2020 •

edited

Loading

SpaceIm Dec 10, 2020 •

edited

Loading

SpaceIm Dec 11, 2020 •

edited

Loading

uilianries commented Dec 23, 2020 •

edited

Loading