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

[feature] check_min_cppstd should specifically report on an unknown compiler #6547

Closed
Minimonium opened this issue Feb 13, 2020 · 7 comments · Fixed by #6548
Closed

[feature] check_min_cppstd should specifically report on an unknown compiler #6547

Minimonium opened this issue Feb 13, 2020 · 7 comments · Fixed by #6548
Assignees
Milestone

Comments

@Minimonium
Copy link
Contributor

The check_min_cppstd works, or is intended to work, with default compilers. But it should handle unknown compilers that could be added by users.

Right now it doesn't handle the None value returned by the deduced_cppstd and proceeds to compare it. Should probably throw a specialized error?
https://github.com/conan-io/conan/blob/develop/conans/client/tools/settings.py#L51

@uilianries
Copy link
Member

Yes, indeed. When a compiler is not specified, it should throw an error.

About supporting different compiler, I would prefer adding them directly to Conan code base, so we can re-use for other cases.

@Minimonium
Copy link
Contributor Author

I meant that we should let users an ability to handle that use case since it's not necessarily a hard error.
Right now it doesn't do it. Handle here means throwing an appropriate error which is not an InvalidConfiguration. 🙂

uilianries added a commit to uilianries/conan that referenced this issue Feb 14, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member

That's why we have valid_min_cppstd, which does not raise an exception. The user is able to decide what to do.

@Minimonium
Copy link
Contributor Author

Minimonium commented Feb 14, 2020

You misunderstood. The situation when the tool itself doesn't know about a compiler is a different outcome from a situation if the cppstd is not valid.

@Minimonium
Copy link
Contributor Author

Forgot to mention, this specified exception is required to correctly handle the standard version as described here: conan-io/conan-center-index#54 (comment)

@czoido czoido self-assigned this Feb 14, 2020
uilianries added a commit to uilianries/conan that referenced this issue Feb 14, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
uilianries added a commit to uilianries/conan that referenced this issue Mar 10, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
memsharded pushed a commit that referenced this issue Mar 10, 2020
* #6547 Raise exception for unknown compiler

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* #6547 Show message with specific compiler-version

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* #6547 Update exception to ConanInvalidConfiguration

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@memsharded memsharded added this to the 1.24 milestone Mar 10, 2020
@Minimonium
Copy link
Contributor Author

Oh, snap. Sorry folks, but I think I've missed a very important moment:
https://github.com/uilianries/conan/blob/3fe411a8908c8c62241230061c9c439ed79b95fc/conans/client/tools/settings.py#L52
https://github.com/uilianries/conan/blob/3fe411a8908c8c62241230061c9c439ed79b95fc/conans/client/tools/settings.py#L61

These two places raise the same exception. Meaning that there is no way to distinguish these two situations. It effectively blocks all new compiler versions until they and their logic are added into the Conan repository.

The use case: https://github.com/conan-io/conan-center-index/blob/master/recipes/abseil/all/conanfile.py#L51

@Minimonium
Copy link
Contributor Author

I've made a slight extension of cppstd operations capability in #6713 , to solve my concern in this issue and in related ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants