Skip to content
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

Suppress "not on PATH" warning when --prefix is given #9931

Merged
merged 1 commit into from
Jul 12, 2021
Merged

Suppress "not on PATH" warning when --prefix is given #9931

merged 1 commit into from
Jul 12, 2021

Conversation

nchepanov
Copy link
Contributor

@nchepanov nchepanov commented Apr 30, 2021

Closes: #9821

Similar to how it works with --target, avoid printing the
warning since it's clear from the context that the final
destionation of the executables is unlikely to be in the PATH.

@uranusjr
Copy link
Member

uranusjr commented May 1, 2021

Looks to me, tests need to be fixed.

@nchepanov
Copy link
Contributor Author

nchepanov commented May 3, 2021

tests need to be fixed.

I'm not sure how, please advice:

The test_installing_scripts_outside_path_prints_warning does exactly this, it uses --prefix and expects --no-warn-script-location in stderr. Since this is the behavior we are changing, it would make sense to somehow trigger this warning by other means. But there are no other command line flags I see other than --target and --prefix that would get the warning printed. Thoughts?

The failing test is followed by succeeding but now redundant test:

  • test_installing_scripts_outside_path_can_suppress_warning - the suppression no longer needed in case of --prefix Should it be removed? Changed?

@pradyunsg looks like you were the one to write these tests 4 years ago, thoughts?

@pradyunsg
Copy link
Member

changed?

I think that test should be changed -- it's intended to check that when the scripts go somewhere that's not on PATH, the warning is printed. --prefix was being used to change where the scripts go, the test should probably be updated to instead change what PATH is (so that where the scripts go is not on PATH).

@nchepanov nchepanov requested review from pradyunsg and uranusjr May 5, 2021 18:59
@pradyunsg
Copy link
Member

Linters aren't happy FYI.

Similar to how it works with `--target`, avoid printing the
warning since it's clear from the context that the final
destionation of the executables is unlikely to be in the PATH.
@uranusjr uranusjr added this to the 21.2 milestone May 6, 2021
@uranusjr uranusjr merged commit f2ce774 into pypa:main Jul 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress "not on PATH" warning when --prefix is given
3 participants