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

Fix normalization during install #3007

Merged
merged 6 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@techalchemy
Member

techalchemy commented Oct 11, 2018

Escape command line requirements as necessary

techalchemy added some commits Oct 11, 2018

Escape command line requirements as necessary
- Fixes #2998

Signed-off-by: Dan Ryan <dan@danryan.co>
double quotes for windows escape char...
Signed-off-by: Dan Ryan <dan@danryan.co>
Upgrade pythonfinder and vistir
Signed-off-by: Dan Ryan <dan@danryan.co>
add news
Signed-off-by: Dan Ryan <dan@danryan.co>
Merge branch 'fix-normalization-during-install' of github.com:pypa/pi…
…penv into fix-normalization-during-install
@orf

This comment has been minimized.

orf commented Oct 12, 2018

Just tested and this correctly installs the pdfminer.six dependency within my lockfile 🎉

@techalchemy techalchemy merged commit aac05b8 into master Oct 12, 2018

2 checks passed

pipenv CI (Linux) #20181011.13 succeeded
Details
pipenv CI (Windows) #20181011.13 succeeded
Details

@techalchemy techalchemy deleted the fix-normalization-during-install branch Oct 12, 2018

@immerrr

This comment has been minimized.

Contributor

immerrr commented Oct 13, 2018

Holy moly! So much headache about quoting and unquoting, and all because delegator uses shell=True unconditionally and is buggy when you pass in a list of arguments

12e80ed#diff-ec74dda0078733c5b6755671cacf1de5R1424

kennethreitz/delegator.py#58

This is the culprit IMHO, after all it's the library that should handle command execution. Instead arbitrary argument quoting snippets are found here and there all over pipenv.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Oct 13, 2018

Not entirely accurate. We do proper splitting in various other libraries and are still forced to contend with this.

@immerrr

This comment has been minimized.

Contributor

immerrr commented Oct 13, 2018

@techalchemy why do you need to do extra splitting? I mean you need to do some splitting to handle requirements.txt lines, but other than that passing commands around as lists of verbatim (without any quoting) arguments should be enough, no?

@techalchemy

This comment has been minimized.

Member

techalchemy commented Oct 14, 2018

What if a path has a space in it? What if that happens on windows? What if both of those are true and the user is using nave path separators (that is, \)? If we figure out a target path for example it still needs to be quoted even if it is split. And if we are parsing user input as we do for --python we also need to be able to perform the splitting

@immerrr

This comment has been minimized.

Contributor

immerrr commented Oct 15, 2018

The short answer is that normally command line quoting should be handled by standard libraries.

What if a path has a space in it?

subprocess.check_output([mycmd, 'hello world']) will pass hello world as a single string to mycmd command, that is if it were a python script it would have sys.argv == ['echo', 'hello world']. It achieves that by using one of the exec-family of functions.

What if that happens on windows?

On windows, CreateProcess takes a string rather than a list, and subprocess uses subprocess.list2cmdline to internally properly quote a string that is passed to a subprocess.

What if both of those are true and the user is using nave path separators (that is, )?

It depends on what do you want to do with it. If you want to pass it forward, then again subprocess does all the quoting as necessary, so something like

subprocess.check_output([cmd, ..., sys.argv[1], ...])

will just work™.

If you want to handle it within pipenv, w.r.t. native/non-native path separators, it's a bit of a mess, but something along the lines of of path_components = os.path.normpath(user_path).split(os.path.sep) and then os.path.join(path_components) down the line should do the trick.

And if we are parsing user input as we do for --python

Same as above.

But like you say, apparently the problems pipenv is experiencing with quoting are related to delegator.py, because it is using shell=True kwarg for all subprocess invocations which is not only slow, but also leads to problems:

  • on POSIX with shell=True arguments passed as a list are interpreted differently (see this python bug and this delegator.py bug
  • on Windows, it is very tricky to quote shell variables reliably (the Internet is full of advice to do smth like cmd /c prog ^%PATH^%, but it is not 100% bulletproof as you can have an env-variable named PATH^ and it seems to get expanded before ^-escaping takes place)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment