tools.check_min_cppstd: add warning about API usage

[leha-bot]

It requires to check the cppstd setting, as sometimes this setting could absent and the api call doesn't check it.

[CLAassistant]

All committers have signed the CLA.

[danimtb]

maybe this is something to improve in the tool itself. WDYT @uilianries @SSE4 @jgsogo ??

[jgsogo]

The tool does the work as documented: https://github.com/conan-io/conan/blob/bb5f59050a947c556f75524ea56610e7ae31c0d2/conans/client/tools/settings.py#L5. We shouldn't mix here (Conan client documentation) the way we are using this tool in conan-center-index and the reason why we are adding that check before. I think this PR is somehow conditioned to the CCI context (at least, this is how I'm understanding it).

In CCI we are never providing the compiler.cppstd setting in the profiles. However, libraries that require new C++ standard (newer than the compiler default) are being built successfully. How? Why? Because the required C++ is set in the build-system. We can find lots of places where we are forcing some C++ standard (CMakeList wrappers and library sources themselves).

The reason why we need that if before running tools.check_min_cppstd in CCI is because the tool would raise if the default C++ standard for the given compiler is not high enough.... and this happens for many of the compilers when you require something newer than C++11. So, we skip the check because we know that the C++ standard is forced later in the build-system.

Nevertheless, general usage or the intention of this tools.check_min_cppstd is to check if the C++ standard is high enough from the POV of Conan, before going to the build-system. And it is the (theoretical) right approach: Conan can validate the full graph and tell the build-system the C++ standard to use, ensuring that all the graph is using the same C++ standard.

On the practical side in CCI, the right approach would require us to generate the profiles for all the C++ standards and trigger a build of the package for all of them (not feasible, it'd require running x5 configurations)... and also to patch a lot of libraries where the build-system has a C++ standard requirement hardcoded into the sources.

I think the examples here still apply and are a good reading: conan-io/examples#73

[SSE4]

if we talk not only about CCI, but about general usage, the API seems to be an easy to misuse in general, and warnings in docs isn't going to address that. I would vote to fix the tool, but have no objections to put the warning for the time being.

now about CCI experience, we even have this section in FAQ: https://github.com/conan-io/conan-center-index/pull/5146/files#diff-9a0d263d27f331b7f71ed0a2f4e39864d4e1ba6df602130a0094d84b52109612R168
also this hook proposal: https://github.com/conan-io/hooks/pull/184/files
doing a grep on CCI repo, I mostly see this pattern (more or less, with small modifications from time to time):

./recipes/ezc3d/all/conanfile.py-36-        if self.settings.compiler.cppstd:
./recipes/ezc3d/all/conanfile.py:37:            tools.check_min_cppstd(self, 11)

so it's clearly only used in conjunction with this sanity check.
if ever we define compiler.cppstd in our CCI profiles, check will be unnecessary, but harmless.
so why not put it inside the tool when?

[uilianries]

As cpp_std is still experimental, I see no problem improving/fixing it for Conan Client. I agree by don't mixing CCI with Conan client, we have FAQ and hooks there.

[jgsogo]

I'm not opposed to changing the tool implementation and change the docs accordingly. But, if that is the case, the proposal should start in conan-io/conan repository defining new behavior, and docs will follow.

[SSE4]

I'm not opposed to changing the tool implementation and change the docs accordingly. But, if that is the case, the proposal should start in conan-io/conan repository defining new behavior, and docs will follow.

not necessarily, it may start as:

• document current behavior (adding note why extra condition might be needed)
• proposal in conan
• document new behavior

[danimtb]

IMO the tool is doing what is documented and I find the behavior logical. Having the additional conan-center-index check inside the tool would "fake" the purpose of the tool and I consider it is better to have this explicit in the recipe. Unless we want to change the behavior of the tool...

[SSE4]

lemme explain my vision a bit :)
the tool was designed, and does what is documented, it's fine.
however, we already have a proof the tool itself is not sufficient for the most cases.
in many cases, it cannot be used as is, and requires an additional extra condition. (we have a proof for that from CCI).
otherwise, without the condition, it returns a misleading results.
the necessity of extra condition is not currently obvious from the current docs. (it might be obvious for developers, but not for newcomer).
for me, the fact tool requires an extra condition in 99% of cases sounds like a bad signal/red flag - the design of the tool probably wasn't right from the beginning, and defaults we have chosen for the tool are not actually defaults for the most of the real world cases.
this has nothing to do with the fact "tool behavior exactly matches the documentation" and "documentation exactly matches the tool design". I am not arguing with that.
I am just arguing with the fact common usage is (at the current moment, at least):

    if self.settings.compiler.get_safe("cppstd"):
        tools.check_min_cppstd(self, 14)

which is not well highlighted and explained in the docs.
I believe it's harmless to explain why this condition could be needed, and what kind of the problem it is trying to solve.

[SSE4]

the possible explanation for the current situation might follow the structure:

• if you're supplying compiler.cppstd setting, then do X
• if you're not supplying compiler.cppstd setting, then do Y
• other use-cases, if any

[jgsogo]

I'm ok with adding a note about that other context where it is used, but the main documentation should follow the implementation. So, something like (extra, like an appendix or footnote):

.. note::

   In those scenarios where `compiler.cppstd` is not explicitly defined in the profiles and 
   the C++ standard requirement is managed in the build system, it could be useful to 
   skip this check so the recipe works as expected for users that provide that `compiler.cppstd` 
   setting and users that don't:
   
   ---
   if self.settings.get_safe('compiler.cppstd'):
       tools.check...
   ---

[prince-chrismc]

I am going to close this as:

1. the CCI convention is/was based on changing client support and based on the specifics of c3i
2. This will no longer be true with 2.0 since the cpp_std will always be present and the compatibility default should cover the majority of cases

We've has lots of conversations over the last two years so hopefully this is satisfactory for everyone!