Support --use-pep517 and --no-use-pep517 inside requirements files #6447
Conversation
cjerdonek
added
C: PEP 517 impact
There was a problem hiding this comment.
We need to update the list of available options under the Requirements File Format to include the new ones.
Also using the original issue's requirements.txt verbatim results in no output:
$ echo "--no-use-pep517 docopt" > requirements.txt
$ pip install -r requirements.txt
$
Using docopt --no-use-pep517 instead works:
$ echo "docopt --no-use-pep517" > requirements.txt
$ pip install -r requirements.txt
Collecting docopt (from -r requirements.txt (line 1))
Using cached https://files.pythonhosted.org/packages/a2/55/8f8cab2afd404cf578136ef2cc5dfb50baa1761b68c9da1fb1e4eed343c9/docopt-0.6.2.tar.gz
Installing collected packages: docopt
Running setup.py install for docopt ... done
Successfully installed docopt-0.6.2
I'm not sure how the former is being interpreted. As a global option?
| @pytest.mark.parametrize('use_pep517, line, expected', [ | ||
| # Test passing use_pep517=None. | ||
| (None, '-e git+https://url#egg=SomeProject', None), | ||
| (None, '--use-pep517 -e git+https://url#egg=SomeProject', True), |
There was a problem hiding this comment.
The docs have: <requirement specifier> [; markers] [[--option]...], we should test for the same.
| @pytest.mark.parametrize('use_pep517, line, expected', [ | ||
| # Test passing use_pep517=None. | ||
| (None, '-e git+https://url#egg=SomeProject', None), | ||
| (None, '--use-pep517 -e git+https://url#egg=SomeProject', True), |
There was a problem hiding this comment.
Can we test with a non-editable, non-vcs requirements specifier as mentioned in the original issue?
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
|
ping @cjerdonek ;-) |
| @@ -172,6 +174,10 @@ def process_line( | |||
| opts, _ = parser.parse_args( | |||
| shlex.split(options_str), defaults) # type: ignore | |||
|
|
|||
| if opts.use_pep517 is not None: | |||
There was a problem hiding this comment.
I rebased this for my own needs, and I only needed to change this to if options and options.use_pep517 ... and replace opts with options on the following line.
|
@jayvdb if you're willing to carry this forward please feel free to open a new PR! |
|
I'm gonna go ahead and close this PR -- it has merge conflicts, there's an not-responded-to review on the PR and there's an open tracking issue where we can have a discussion even after this PR is closed. |
lock
bot
added
the
auto-locked
Fixes #6438.