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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConanInvalidConfiguration exception and handle error codes. #3517

Merged
merged 3 commits into from Sep 24, 2018

Conversation

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Sep 10, 2018

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:

  • anything related to workspaces?
  • anything related to APIs?
  • somewhere in the graph model?

Let's talk about this

Changelog: Feature: Added ConanInvalidConfiguration as the standard way to indicate that a specific configuration is not valid for the current package. e.g library not compatible with Windows.

SUCCESS = 0
ERROR_GENERAL = 1
ERROR_INVALID_CONFIGURATION = 5

Copy link
Member Author

@jgsogo jgsogo Sep 10, 2018

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

Copy link
Member

@memsharded memsharded Sep 10, 2018

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?
Copy link
Member Author

@jgsogo jgsogo Sep 10, 2018

I don't like having this message formatted here... any ideas?

Copy link
Member

@memsharded memsharded Sep 10, 2018

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.

Copy link
Member Author

@jgsogo jgsogo Sep 11, 2018

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.

Copy link
Member

@memsharded memsharded Sep 11, 2018

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.

Copy link
Member

@danimtb danimtb left a comment

Minimal comments with some suggestions. LGTM

conans/test/command/invalid_configuration_test.py Outdated Show resolved Hide resolved
conans/test/command/invalid_configuration_test.py Outdated Show resolved Hide resolved
@lasote lasote added this to the 1.8 milestone Sep 12, 2018
@lasote
Copy link
Contributor

@lasote lasote commented Sep 12, 2018

Still WIP?

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Sep 12, 2018

Not if you think that all these use-cases are already covered:

Feedback is needed from experienced frogarians 馃惛 as other use cases may be affected:

  • anything related to workspaces?
  • anything related to APIs?
  • somewhere in the graph model?

@lasote
Copy link
Contributor

@lasote lasote commented Sep 12, 2018

I don't think so... it should be ok. Please, rename the title if it is not WIP.

@jgsogo jgsogo changed the title [WIP] Add ConanInvalidConfiguration exception and handle error codes. Add ConanInvalidConfiguration exception and handle error codes. Sep 12, 2018
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Sep 12, 2018

Done, just a little bit of documentation needed; I'll do it as soon as possible.

@lasote lasote merged commit eecc16f into conan-io:develop Sep 24, 2018
2 checks passed
@ghost ghost removed the stage: review label Sep 24, 2018
@jgsogo jgsogo deleted the issue/3484 branch Sep 25, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
鈥-io#3517)

* add exception

* test for command line

* move exit codes to global scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants