-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
conda_install
and install
args are now automatically double-quoted when needed.
#312
Conversation
…d when they contain a `<` or a `>`. Fixes wntrblm#311
…dblquote_pkg_install_arg
Note: although the tests pass, it seems that this fix is only needed for Invalid requirement: '"setuptools"'
Traceback (most recent call last):
File "C:\...\.nox\tests-2-7\lib\site-packages\pip\req\req_install.py", line 82, in __init__
req = Requirement(req)
File "C:\...\.nox\tests-2-7\lib\site-packages\pip\_vendor\packaging\requirements.py", line 96, in __init__
requirement_string[e.loc:e.loc + 8]))
InvalidRequirement: Invalid requirement, parse error at "'"setupto'" |
… Apparently, `pip` does not need it.
This looks fine, but can you show me some invocations that cause this behavior? |
For example this:
On windows, it runs with success ! Indeed the And here is the content of the file
As can be seen above, the version constraint was not even seen by conda. |
Note that there is a workaround today: the user has to quote the dependency, e.g. |
nox/sessions.py
Outdated
@@ -361,6 +395,9 @@ def install(self, *args: str, **kwargs: Any) -> None: | |||
if not args: | |||
raise ValueError("At least one argument required to install().") | |||
|
|||
# Escape args that should be: not needed and would make pip raise an InvalidRequirement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this instead of leaving it commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. This was for maintainability - but instead of writing this here, I edited the comment in the conda part so that future readers are not tempted to apply the same escaping procedure to the pip section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I'd want one of our maintainers that actually uses conda to review. Maybe @tswast?
I'm a bit confused as to why this is necessary. When I run |
It appears that this is a windows-only issue (https://docs.python.org/3/library/subprocess.html#subprocess.Popen)
|
LGTM, since conda is able to handle the extra quotes on all platforms. |
Great, thanks for the review @tswast ! I updated the branch, it should be ready for merging |
Thanks all! |
Changelog (let me know if I should commit it somewhere):
conda_install
andargs are now automatically double-quoted when they contain ainstall
<
or a>
. Fixesconda_install
should automatically escape args when they contain version constraints #311