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

fixing commands fail when no folder #6825

Merged
merged 3 commits into from Apr 14, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Apr 8, 2020

Changelog: Fix: Avoid failures that happen when Conan runs in a non-existing folder.
Docs: Omit

Close #6075

It also includes some minor pep8 fixes in the command.py file

@memsharded memsharded added this to the 1.25 milestone Apr 8, 2020
jgsogo
jgsogo approved these changes Apr 10, 2020
@memsharded memsharded marked this pull request as draft Apr 10, 2020
@memsharded
Copy link
Member Author

@memsharded memsharded commented Apr 10, 2020

I am moving this to draft. This responsibility is implemented already in the conan_api decorator, and in this point it wouldn't be necessary. The fix doesn't correct the error in the conan_api decorator.

@memsharded memsharded requested a review from jgsogo Apr 10, 2020
@memsharded
Copy link
Member Author

@memsharded memsharded commented Apr 10, 2020

Moved responsibility to api, removed from main(). Please @jgsogo review again

@memsharded memsharded marked this pull request as ready for review Apr 10, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Not a review, just a comment:

There are many places in Conan where we are calling os.getcwd or get_cwd (btw, why this file and comment?). This same cryptic error can be raised at any of those places.

Before this PR, Conan was failing at the very beginning, now Conan can fail with this exception at many different places. I'm not sure if it is better to raise a meaningful error than to protect/bypass at this point.

wdyt?

@memsharded
Copy link
Member Author

@memsharded memsharded commented Apr 11, 2020

Not a review, just a comment:

There are many places in Conan where we are calling os.getcwd or get_cwd (btw, why this file and comment?). This same cryptic error can be raised at any of those places.

Before this PR, Conan was failing at the very beginning, now Conan can fail with this exception at many different places. I'm not sure if it is better to raise a meaningful error than to protect/bypass at this point.

wdyt?

I have checked the code, in practice, many commands that do not require the current path (search, remove, upload), do not use getcwd at all, so I think this would be already an improvement for those commands.

For other commands, yes the error will be equally cryptic, and raised at different places. We could wrap in the get_cwd (was initially intended for unicode) for every os.getcwd() in the codebase (or at least in the first call of relevant commands), to protect against this error. Probably not worth, just a small UX improvement for a not critical thing.

jgsogo
jgsogo approved these changes Apr 11, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Ok

@danimtb danimtb merged commit c012b09 into conan-io:develop Apr 14, 2020
2 checks passed
@memsharded memsharded deleted the feature/folder_no_necessary branch Apr 20, 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.

3 participants