Add ConanInvalidConfiguration exception and handle error codes. #3517
Conversation
ghost
assigned
conans/client/command.py
Outdated
| SUCCESS = 0 | ||
| ERROR_GENERAL = 1 | ||
| ERROR_INVALID_CONFIGURATION = 5 | ||
|
|
There was a problem hiding this comment.
I think that we need a single place to put this hardcoded values, as they are tightly coupled with those documented in the main method:
def main(args):
""" main entry point of the conan application, using a Command to
parse parameters
Exit codes for conan command:
0: Success (done)
1: General ConanException error (done)
2: Migration error
3: Ctrl+C
4: Ctrl+Break
5: Invalid configuration (done)
"""
There was a problem hiding this comment.
Yes, feel free to move them to global scope, I guess here, in the command.py file? Other file?
| @@ -22,6 +22,9 @@ def conanfile_exception_formatter(conanfile_name, func_name): | |||
| """ | |||
| try: | |||
| yield | |||
| except ConanInvalidConfiguration as exc: | |||
| msg = "{}: Invalid configuration: {}".format(conanfile_name, exc) # TODO: Move from here? | |||
There was a problem hiding this comment.
I don't like having this message formatted here... any ideas?
There was a problem hiding this comment.
I think it is consistent to the way exceptions from conanfile.py are managed. Check _format_conanfile_exception(), it might be worth to print the line number, etc. in the error message too.
Otherwise, if you wanted something specific to configure(), you could try-except the call to conanfile.configure() in graph_builder.py, but wouldn't be much cleaner either.
There was a problem hiding this comment.
I prefer to handle the exception message here, better not pollute the graph code.
This is not an error, is it? so I don't think that adding information about file (it is always conanfile.py), or line (always inside configure) adds any information to the caller. It contains the conan ref and the message defined in the recipe, so it should be enough.
There was a problem hiding this comment.
I was thinking about lazy raise ConanInvalidConfiguration("Unsupported platform"), and which some context, probably seeing the line above could help. But yes, fair, leave it as is, it is good.
|
Still WIP? |
|
Not if you think that all these use-cases are already covered:
|
|
I don't think so... it should be ok. Please, rename the title if it is not WIP. |
jgsogo
changed the title
|
Done, just a little bit of documentation needed; I'll do it as soon as possible. |
…n-io#3517) * add exception * test for command line * move exit codes to global scope
The aim of this PR is to implement a ConanInvalidConfiguration exception that the user can raise from the
configure()function in ConanFile to tell consumers of the recipe that a given set of settings (and options?) is known to fail (and it is ok).This exception should be propagated across Conan code up to the command caller, letting the caller know about this specific error and handle it. It will be very useful for tools like
conan-package-tools.Feedback is needed from experienced frogarians 🐸 as other use cases may be affected:
Let's talk about this
Changelog: Feature: Added
ConanInvalidConfigurationas the standard way to indicate that a specific configuration is not valid for the current package. e.g library not compatible with Windows.