Pass arbitrary args to 'pip' from pip-compile and pip-sync, with '--pip-args'

[AndydeCleyre]

Technically, pass all args after the second standalone '--' arg, whether or not they're consecutive.

Fixes #321

$ pip-sync <pip-sync arg/flag>... --pip-args '<pip install arg/flag>...'
$ pip-sync <pip-sync arg/flag>... --pip-args '<pip install arg/flag>...'

Changelog-friendly one-liners:

• Any flags or arguments following two standalone double-dashes (' -- -- ') at the end of pip-compile or pip-sync's argument list are passed on to pip
• pip-compile output headers are now more accurate when ' -- ' is used to escape filenames
• pip-compile and pip-sync now pass anything provided to the new --pip-args option on to pip

Contributor checklist

• Provided the tests for the changes.
• Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
• Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

[AndydeCleyre]

Process question: Am I supposed to check the clear-oneliner box myself, or leave that to a reviewer's judgment?

[codecov]

Codecov Report

Merging #1080 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1080   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          34       34           
  Lines        2476     2499   +23     
  Branches      307      309    +2     
=======================================
+ Hits         2462     2485   +23     
  Misses          8        8           
  Partials        6        6           

Impacted Files	Coverage Δ
piptools/scripts/compile.py	100.00% <100.00%> (ø)
piptools/scripts/sync.py	100.00% <100.00%> (ø)
piptools/utils.py	100.00% <100.00%> (ø)
tests/test_cli_compile.py	100.00% <100.00%> (ø)
tests/test_cli_sync.py	100.00% <100.00%> (ø)
tests/test_utils.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c503c9...8a2efa1. Read the comment docs.

[atugushev]

By the way, we could do the same way on pip-compile to pass additional args to pip_args.

[atugushev]

@AndydeCleyre

Process question: Am I supposed to check the clear-oneliner box myself, or leave that to a reviewer's judgment?

Both are fine.

[atugushev]

I think it's worth to mention -- -- technique in README.

[AndydeCleyre]

Now includes an example in the pip-sync section of the readme.

While I was there I noticed these sentences are included, in different spots:

If you use multiple Python versions, you can also run ``py -X.Y -m piptools sync`` on Windows and ``pythonX.Y -m piptools sync`` on other systems.

If you use multiple Python versions, you can run ``pip-sync`` as ``py -X.Y -m piptools sync ...`` on Windows and ``pythonX.Y -m piptools sync ...`` on other systems.

[atugushev]

By the way, we could do the same way on pip-compile to pass additional args to pip_args.

@AndydeCleyre what do you think?

[AndydeCleyre]

Oh, you mean in this same PR?

OK, I can get to it within 12 hours

[AndydeCleyre]

FYI I've started the pip-compile work locally, but there are still a few wrinkles to iron out before I push. I'll have to resume work a little later.

[AndydeCleyre]

I don't know if this is within scope of this PR, but these changes have exposed to me and expanded already existing problems with the header generation omitting -- from the argument list.

$ echo six > --requirements.in
$ pip-compile -- --requirements.in
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --requirements.in
#
six==1.14.0               # via -r --requirements.in
$ pip-compile -- --requirements.in -- --disable-pip-version-check
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile -- --disable-pip-version-check --requirements.in
#
six==1.14.0               # via -r --requirements.in

Those commands in the headers are not valid and will fail. The first will read --requirements.in as an illegal option, and the second will read --disable-pip-version-check as a nonexistent path.

[AndydeCleyre]

Tomorrow I will have a go at improving get_compile_command for inclusion in this PR.

Note to self:

click_ctx.params == {'src_files': ('--requirements.in', '--', '--disable-pip-version-check'),...}

[AndydeCleyre]

OK, so get_compile_command intentionally omits:

• arguments that don't change build behaviour like '--verbose'
• one-off arguments like '--upgrade'
• values that are already default

and also does expanding options short to long

What do you think of the following judgments regarding "forwarded" pip args?

1. We shouldn't bother checking if values are default or not; those details are too far outside this project.
2. Similarly, we shouldn't bother doing any expansion for them.
3. Again, we shouldn't be concerned with whether or not they change build behavior. Basically, I think we should be "hands off" as much as possible for these, not trying to be too clever.
4. The big question for me: should they all be treated as one-offs, and omitted? In favor of that:
   • simpler implementation (no extra sorting, and we can ignore 1, 2, and 3)
   • if users are regularly using that feature as standard project practice, they can always provide CUSTOM_COMPILE_COMMAND

[AndydeCleyre]

I've updated get_compile_command and a corresponding test to be more accurate when encountering one or two uses of -- in the pip-compile argument list.

Arguments that are forwarded to pip are not transformed or sorted or omitted, but left as-is as much as possible.

Please let me know what you think, and if you want these two commits squashed.

[AndydeCleyre]

Ack, this isn't quite right yet, in its handling of - to signify stdin/out. Stay tuned.

Actually I think it's quite fine. It's different, in that it adds a -- to the header in that case, but that's still correct and copy-paste-able. It's already the case that the header omits the stdin part itself, and that doesn't change.

Before:

$ pip-compile -q --output-file=- - < requirements.in | grep output
#    pip-compile --output-file=- -

After:

$ pip-compile -q --output-file=- - < requirements.in | grep output
#    pip-compile --output-file=- -- -

[AndydeCleyre]

I guess the real question regarding the last comment is if we want to change behavior for

$ pip-compile --output-file=- -- -

to treat - as an actual filename, whereas now any src_file with that exact name is taken to signify stdin rather than a file. I think it's fine to keep as-is.

[AndydeCleyre]

Looks like this will need a careful rebase now that c4dcaf7 is pushed. I see that pip-compile's install_flags is now built differently.

Would it make sense to add a kwarg to _compose_install_flags like extra_flags, passing any -- -- "forwarded" ones as a list/tuple?

Or would it be preferable to simply extend its return value after it's called, outside that function?

[AndydeCleyre]

I'm looking at / merging this c4dcaf7 . . . Is it just me, or are get_trusted_hosts and create_install_command ghosts?

I don't think that was ready for master, maybe I should hold off on merging it into this PR for now.

[atugushev]

@AndydeCleyre see #824 (comment) for details. I'll fix this shortly.

[atugushev]

@AndydeCleyre fixed

[AndydeCleyre]

Alright, after merging in those changes last night, I did a proper rebase onto master today. Please have a look. I've included all the concerns I raised above, below.

Review Pointers

tests/test_cli_sync.py:test_pip_install_flags

#824 changed some things about this test, maybe making it neater, but AFAICT also making it insufficiently flexible for the cases I needed in this PR, so those were partially reverted.

The issue is just that the flags passed to pip-sync are not identical to those which should be passed to pip install (namely, the -- instances). Someone who has a better idea than me of the intent and benefits behind those test changes, please pay attention.

piptools/scripts/sync.py:_compose_install_flags

#824 introduced _compose_install_flags. As-is, it does not accept any "extra" or "right" args to append to the more deliberately handled flags. It may or may not be preferable to add this functionality to that function. I chose to leave the function alone and instead append the new forwarded args to the list returned by that function.

stdin marker -

$ pip-compile --output-file=- -- -

What should the behavior be for this command? As is, -- signifies that the following arguments are args, not options. - is hard-coded to signify "stdin" rather than a file. It is conceivable that someone would want or expect the -- to instead signify that the following - is a literal filename. Of course no sane person would use a file with that name, and they should be able to get by with ./-, but I'd like to make sure this decision is hashed out.

piptools/utils.py:get_compile_command and pip-compile --output-file=- -

This is closely related to the previous pointer and will be affected by the corresponding consensus, whatever that turns out be.

Before this PR, a command like:

$ pip-compile --output-file=- -

would generate the following as documentation in the output header:

#    pip-compile --output-file=- -

The current state of this PR changes that part of the header to:

#    pip-compile --output-file=- -- -

With the current treatment of - as stdin (never a literal filename), this is functionally equivalent and also correct. It is of course possible to add a bit more logic to get_compile_command to strip out the excess -- . IMO, if we are to keep our current treatment of -- -, doing so is unwarranted. But we should do it if either you disagree, or we change our interpretation of -- - to handle - as a filename.

piptools/utils.py:get_compile_command and sorting, omission, & expansion

I chose to include the forwarded args and flags in the get_compile_command output, without applying any of those operations to them (sorting, omission, expansion). I view forwarded args after -- -- with a "take your stinking parsers off me, you damn dirty pip-tools" attitude, as much as possible. I think there's also a decent case for omitting them altogether, but including them is my preference.

[atugushev]

Just a thought, what if we add a new option --pip-args (same as pipx) instead of -- --. For example:

$ pip-compile --pip-args="--no-cache-dir"
$ pip-compile --pip-args="--no-cache-dir --no-deps --force-reinstall"

[tim-mitchell]

I don't mind either way. But if you go with the --pip-args option then no escaping should be done. That should be the users responsibility.
AFAICT quote escaping will be handled by the shell and
--pip-args="--some-arg=\"path with spaces\"" will pass
--pip-args=--some-arg="path with spaces" as a parameter to sys.argv allowing
--some-arg="path with spaces" to be passed to pip (on windows at least).

[AndydeCleyre]

Thanks everyone, I'll start re-implementing this as a single, quoted arg soon. Time is a little limited for now, especially during the week, but optimistically this PR will ready for another review next week.

In the meantime, you are welcome to mention here all the crazy examples you might use with this feature (with spaces and single and double quotes and whatever) to help me test properly.

[AndydeCleyre]

For posterity, the "double-dash islands" implementation now lives at my bugfix/321-doubledash-islands branch.

I've just pushed a re-implementation to this branch, using --pip-args, and relying on shlex to do proper arg splitting.

Please review and test!

[atugushev]

$ echo | pip-compile - -qo- --pip-args="--cache-dir='/tmp/with spaces'"
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file=- --pip-args='--cache-dir='"'"'/tmp/with spaces'"'"'' -
#
#

I've checked a header, it looks weird.

[AndydeCleyre]

Thanks, I'll check it out. It is a valid and working command, despite the ugly quoting.

[AndydeCleyre]

I've checked a header, it looks weird.

This may just be the price of using --pip-args='<str>' here instead of letting the shell handle it, as feared.

I think mitigating the redundant quotes would be functionally fragile and hacky.

[AndydeCleyre]

Maybe I'll dig into click tonight and see if something in there can help.

[AndydeCleyre]

EDIT: tests are failing on old python versions I didn't test locally, I'm investigating, please feel free to delay looking at this until checks pass.

Alright, I've rammed through the py2 compatibility issues, but there's probably a better way to do that. I don't like this unsquashed commit at all. Of the choices listed below, I like numbers 3 and 4 about the same, followed by 1.

---

@atugushev Please take a look at the (currently unsquashed) new commit, which special-cases pip_args within get_compile_command to just pass its value through repr instead of shlex_quote.

It's probably a fragile idea. Proper quoting can be hell, which is why I prefer the first implementation, to avoid dealing with it. But this hack is only used for the comment, so its perfection isn't as critical as it would be if it were affecting actual pip calls. It might be good enough.

So I'm thinking either:

1. you can turn this new commit's idea into something saner
2. decide this hack is good enough, and I'll squash it
3. revert to the -- -- implementation; I'd backport a couple test cases and rebase that preserved branch onto master and bulldoze this one
4. live with the ugly redundant yet reliably functional quoting exhibited without this new commit

[atugushev]

Awesome! I left a few comments below.

[AndydeCleyre]

splat

[atugushev]

Thank you, @AndydeCleyre, for the great work and patience 🎉 🎉 🎉

[tim-mitchell]

Thanks @AndydeCleyre and @atugushev