Prevent the use of PIP_NO_USE_PEP517 to head off user confusion

[cjerdonek]

This PR is a follow-up to PR #5743 (the PEP 517 PR). I'm hoping it can be merged in the same release that PEP 517 goes out since for backwards compatibility reasons we might not be able to merge it afterwards.

This PR addresses an issue I raised with that PR and discussed with @pfmoore in that issue. He and I agreed I would take care of it: #5743 (comment)

Basically, the change is to prevent from happening with the new --no-use-pep517 option a confusion that happened in the past with certain other "no" options (like --no-cache-dir): #5735
Namely, without this PR, someone can pass PIP_NO_USE_PEP517=true, and it will result in options.use_pep517 being True instead of False, which is what one would logically expect.

This PR prevents this by not allowing the environment variable PIP_NO_USE_PEP517 to be used. Instead, it instructs the user to use an appropriate value for PIP_USE_PEP517. Here is what the output looks like:

$ PIP_NO_USE_PEP517=true pip download xxx

Usage:   
  pip download [options] <requirement specifier> [package-index-options] ...
  pip download [options] -r <requirements file> [package-index-options] ...
  pip download [options] <vcs project url> ...
  pip download [options] <local project path> ...
  pip download [options] <archive url/path> ...

--no-use-pep517 error: A value was passed for --no-use-pep517,
probably using either the PIP_NO_USE_PEP517 environment variable or
the "no-use-pep517" config file option. Use an appropriate value of
the PIP_USE_PEP517 environment variable or the "use-pep517" config
file option instead.

The PR also improves handling of the corresponding --no-cache-dir scenario by raising a parser error instead of a ValueError, since I noticed that while implementing this PR. This results in a better error message display. Instead of a ValueError stack trace, the user will get this:

$ PIP_NO_CACHE_DIR=invalid pip list

Usage:   
  pip <command> [options]

--no-cache-dir error: invalid truth value 'invalid'

After this PR, it shouldn't be too hard also to address @pradyunsg's suggestions that he made here:
#5743 (comment)
since this PR sets up the option callback and tests, etc, needed for his suggestion.

[pradyunsg]

Could you add a release blocker label to this? (on my mobile)

[cjerdonek]

Done!

[cjerdonek]

By the way, the unit tests I added reveal that use_pep517 will be 0 or 1 rather than True or False if the option is invoked via an environment variable or config file option rather than the command-line option. This is because the strtobool() function that the parser uses returns 0 or 1 rather than True or False. However, the main part of the code uses is rather than == when deciding whether use_pep517 has been specified, e.g. here:

pip/src/pip/_internal/pyproject.py

Line 63 in 47d7f2b

if use_pep517 is False:

And here:

pip/src/pip/_internal/pyproject.py

Line 70 in 47d7f2b

if use_pep517 is False:

I don't know if there are other places that the code distinguishes between 0 and False, etc.

[cjerdonek]

If the if use_pep517 is False: lines are changed to if use_pep517 is not None and not use_pep517:, it looks like that might be sufficient to address the issue. It seems like that change would be good in any case. That way use_pep517=0 and use_pep517=False will result in the same behavior.

[pradyunsg]

Pinging @pfmoore for comments on this. :)

[pfmoore]

I'll try to get some time to review, but I'm super busy at the moment, so it may be a while. Please don't hold the release on the basis of waiting for me to have free time. Sorry! (I agree with @cjerdonek that it would be nice for this to go in alongside the PEP 517 change, though).

[pradyunsg]

Please don't hold the release on the basis of waiting for me to have free time.

Roger that! I'll try to do a review on this myself, later this week.

[pradyunsg]

LGTM.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.