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

Handle unknown statement while parsing profile. Fixes #6931 #7577

Merged
merged 5 commits into from Aug 24, 2020

Conversation

iblancasa
Copy link
Contributor

@iblancasa iblancasa commented Aug 22, 2020

Changelog: Bugfix: Provide a more descriptive error when an unknown statement is added to a profile
Docs: omit

Closes: #6931

Tested with the example provided in the examples repository

I replaced the first line:

include(default)

By:

includes(default)

When running conan before this change, the command output is:

✗ ./build.sh
+ conan remove 'conan-hello-emscripten/*' -f
WARN: No package recipe matches 'conan-hello-emscripten/*'
+ conan create . conan/testing -pr emscripten.profile --build missing
ERROR: Error reading 'emscripten.profile' profile: Error parsing the profile text file: not enough values to unpack (expected 2, got 1)

After this change:

✗ ./build.sh
+ conan remove 'conan-hello-emscripten/*' -f
WARN: No package recipe matches 'conan-hello-emscripten/*'
+ conan create . conan/testing -pr emscripten.profile --build missing
ERROR: Error reading 'emscripten.profile' profile: Error while parsing line 0: 'includes(default)'

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2020

CLA assistant check
All committers have signed the CLA.

@memsharded memsharded self-assigned this Aug 23, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good. Please check the comment about the exception.

It would be great to include a test that covers this case. Do you think you could manage to add it? profile_loader_test.py seems a good place. Ask for help if need it, we could contribute the test if you prefer.

try:
name, value = line.split("=", 1)
except ValueError as error:
raise ConanParsingError("Error while parsing line %i: '%s'" % (counter, line))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's throw a generic ConanException. New exceptions will be introduced from now on, only if it is going to be captured and processed somewhere else. Let's keep it simple.

@memsharded
Copy link
Member

I hadn't check the CI, please check: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-7577/1/pipeline

One of the test was producing a different output, the test can be changed (and also adding a new one that valides the includes() error too)

@memsharded memsharded added this to the 1.29 milestone Aug 23, 2020
@iblancasa
Copy link
Contributor Author

Ok, I added the requested changes except the tests. I'm trying to figure out what test I broke because, when I run tox, I get:

FAILED (SKIP=250, errors=30, failures=2)

Even if I run those tests from develop. I'm running the tests in a Fedroa 31 machine. Can be related? Do I need a specific environment? If yes, do you think it can make sense to create a Dockerfile to help developers to run tests?

@memsharded
Copy link
Member

Even if I run those tests from develop. I'm running the tests in a Fedroa 31 machine. Can be related? Do I need a specific environment? If yes, do you think it can make sense to create a Dockerfile to help developers to run tests?

Yes, setting up the whole environment might be a bit complicated, and a docker image will only partially help, because there might be easily tests failing in other platforms, other python versions, etc. Typically having a look at our CI and running those failing tests manually (with nosetests conans.test.functional...) is good enough for development, this is what I do many times, when not in my main OS.

In your case: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-7577/2/pipeline, so probably running nosetests conans.test.functional.command.install.install_test.InstallTest is good enough.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

@memsharded memsharded merged commit e038eb4 into conan-io:develop Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Confusing error message when typo in profile
3 participants