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

Issue #2839: Avoid dequoting markers in req files. #2841

Merged
merged 8 commits into from Jun 1, 2015

Conversation

Projects
None yet
4 participants
@rbtcollins
Contributor

rbtcollins commented May 29, 2015

In < 7 requirements files did not need escaping for quotation marks.
7.0 introduced quoting (via the use of shlex.split) when also
introducing the feature of supporting local and global options,
because paths in particular commonly contain spaces. However, markers
already contain quote marks, so this broke existing quotes needed for
marker support.

Since we've only had a week while this has been public it should be
pretty safe to unwind the quoting aspect for markers and restore
previously working requirements files to working order.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 29, 2015

Contributor
Contributor

qwcode commented May 29, 2015

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 29, 2015

Contributor

the problem here is that someone will very likely use quotes, and it breaks

a line in a requirements file:
peppercorn --install-option="--install-purelib=/tmp/yo"

Installing collected packages: peppercorn
  Running setup.py install for peppercorn
    Complete output from command /home/qwcode/.qwdev/pip2/bin/python -c "import setuptools, tokenize;__file__='/tmp/pip-build-x96YHx/peppercorn/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" install --record /tmp/pip-tW3Bzw-record/install-record.txt --single-version-externally-managed --compile --install-headers /home/qwcode/.qwdev/pip2/include/site/python2.7/peppercorn "\"--install-purelib=/tmp/yo\"":
    invalid command name '"--install-purelib=/tmp/yo"'
Contributor

qwcode commented May 29, 2015

the problem here is that someone will very likely use quotes, and it breaks

a line in a requirements file:
peppercorn --install-option="--install-purelib=/tmp/yo"

Installing collected packages: peppercorn
  Running setup.py install for peppercorn
    Complete output from command /home/qwcode/.qwdev/pip2/bin/python -c "import setuptools, tokenize;__file__='/tmp/pip-build-x96YHx/peppercorn/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" install --record /tmp/pip-tW3Bzw-record/install-record.txt --single-version-externally-managed --compile --install-headers /home/qwcode/.qwdev/pip2/include/site/python2.7/peppercorn "\"--install-purelib=/tmp/yo\"":
    invalid command name '"--install-purelib=/tmp/yo"'

@rbtcollins rbtcollins changed the title from Issue #2839: Revert syntax of requirement files. to Issue #2839: Avoid dequoting markers in req files. May 29, 2015

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins May 29, 2015

Contributor

I believe I've addressed the quoting needs for options in this revised patch.

Contributor

rbtcollins commented May 29, 2015

I believe I've addressed the quoting needs for options in this revised patch.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 29, 2015

Contributor

aw man, the purity is gone.. : )

since quoting is required in the shell for markers, I personally felt ok with requiring it in the requirements file, vs having to do this... I understand compatibility is a concern, but I was hoping the user base for markers was still pretty small enough to make this jump now.

Contributor

qwcode commented May 29, 2015

aw man, the purity is gone.. : )

since quoting is required in the shell for markers, I personally felt ok with requiring it in the requirements file, vs having to do this... I understand compatibility is a concern, but I was hoping the user base for markers was still pretty small enough to make this jump now.

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins May 29, 2015

Contributor

Well, its a lot harder to read

"python_version=='2.6'"

than

python_version=='2.6'

  • I think that that is a significant win over the purity of our internal code argument.
Contributor

rbtcollins commented May 29, 2015

Well, its a lot harder to read

"python_version=='2.6'"

than

python_version=='2.6'

  • I think that that is a significant win over the purity of our internal code argument.
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 29, 2015

Contributor

just to be clear, the quotes include the requirement too 'peppercorn ; python_version=="2.6"'

Contributor

qwcode commented May 29, 2015

just to be clear, the quotes include the requirement too 'peppercorn ; python_version=="2.6"'

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 29, 2015

Member

The documentation needs to explain the syntax more clearly. Are markers allowed in combination with --install-option? I assume not, as I can't see how the parsing would work (and the comment "as the last thing on the line" indicates that is the case).

The original syntax is regular, but somewhat annoying for the user. This PR makes the syntax somewhat DWIM, and as a consequence somewhat harder to reason about.

It's not clear to me which way the trade-off should go. As I don't typically use markers or --install-option I have no real-world experience to base a judgement on. I'm mildly inclined to the current, more regular, syntax as it'll be easier for me to work out when I do need it. Heavy users may feel otherwise, though.

Member

pfmoore commented May 29, 2015

The documentation needs to explain the syntax more clearly. Are markers allowed in combination with --install-option? I assume not, as I can't see how the parsing would work (and the comment "as the last thing on the line" indicates that is the case).

The original syntax is regular, but somewhat annoying for the user. This PR makes the syntax somewhat DWIM, and as a consequence somewhat harder to reason about.

It's not clear to me which way the trade-off should go. As I don't typically use markers or --install-option I have no real-world experience to base a judgement on. I'm mildly inclined to the current, more regular, syntax as it'll be easier for me to work out when I do need it. Heavy users may feel otherwise, though.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 30, 2015

Contributor

Are markers allowed in combination with --install-option

they are in the released version, like this:

"peppercorn ; python_version <= '3.0'" --install-option='--user'

they would not be with this PR, unless they were oddly tacked on after the options?

peppercorn  --install-option='--user'  ; python_version <= '3.0'"
Contributor

qwcode commented May 30, 2015

Are markers allowed in combination with --install-option

they are in the released version, like this:

"peppercorn ; python_version <= '3.0'" --install-option='--user'

they would not be with this PR, unless they were oddly tacked on after the options?

peppercorn  --install-option='--user'  ; python_version <= '3.0'"
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 30, 2015

Contributor

I think a better approach than this PR would be to find the breakpoint between the args and the options (assuming the options always follow args), and only shlex the options portion, and not the args.

Contributor

qwcode commented May 30, 2015

I think a better approach than this PR would be to find the breakpoint between the args and the options (assuming the options always follow args), and only shlex the options portion, and not the args.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 30, 2015

Contributor

and to find that breakpoint, I think it involves using the result of shlex.split and looking for long or short option strings using the list of supported options.

Contributor

qwcode commented May 30, 2015

and to find that breakpoint, I think it involves using the result of shlex.split and looking for long or short option strings using the list of supported options.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode May 30, 2015

Contributor

see #2844

Contributor

qwcode commented May 30, 2015

see #2844

qwcode and others added some commits May 30, 2015

Break up the line into an args and options string.
We only want to shlex (and then optparse) the options, not the args.
args can contain markers which are corrupted by shlex
- we won't be shlex parsing the args later,
  so the split here should just be simple string split on ' '
- token parsing logic fixes
- text fixes (tuples, not lists)
Robert Collins
Tweak requirement file documentation.
We're actually pinning down a fairly specific grammar now, so lets
make it official. All options are at the end, and only options get
quoting. I've also tweaked some of the existing examples to make the
existing grammar features (that I know people use) clearer - like
spaces between requirements and version specifiers.
@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Jun 1, 2015

Contributor

I've replaced the content of this with a rebased 2844 + doc tweaks making the interactions with 7.0.0 and 7.0.1, plus the interactions between reqs, specifiers, markers, and options clearer (IMO).

Contributor

rbtcollins commented Jun 1, 2015

I've replaced the content of this with a rebased 2844 + doc tweaks making the interactions with 7.0.0 and 7.0.1, plus the interactions between reqs, specifiers, markers, and options clearer (IMO).

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jun 1, 2015

Member

Are you OK with this being merged @qwcode ?

Member

dstufft commented Jun 1, 2015

Are you OK with this being merged @qwcode ?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jun 1, 2015

Contributor

yea, he's doc and changelog update is ok with me.

Contributor

qwcode commented Jun 1, 2015

yea, he's doc and changelog update is ok with me.

@dstufft dstufft added this to the 7.0.2 milestone Jun 1, 2015

dstufft added a commit that referenced this pull request Jun 1, 2015

Merge pull request #2841 from rbtcollins/stable
Issue #2839: Avoid dequoting markers in req files.

@dstufft dstufft merged commit 70c200b into pypa:develop Jun 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dstufft added a commit to dstufft/pip that referenced this pull request Jun 1, 2015

Merge pull request pypa#2841 from rbtcollins/stable
Issue pypa#2839: Avoid dequoting markers in req files.
(cherry picked from commit 70c200b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment