Warn user when installing scripts outside PATH

[pradyunsg]

Fixes #3813, Helps with #1668.

[pfmoore]

We only need this warning if the install actually creates binaries. At the moment it warns unconditionally, whether or not there is an actual issue. I appreciate it's harder to only warn if binaries are being installed (maybe so much harder that it's impractical) but I'd prefer it that way. I'm a strong -1 on "warnings" that don't actually report an issue and can be safely ignored.

To ensure the warning is correctly produced, I'd suggest adding two tests - one that tests that installing a package with binaries warns, and one that tests that installing a package with no binary doesn't warn.

[pradyunsg]

We only need this warning if the install actually creates binaries.

So, should we instead be checking if the installed path contain scripts that would be installed out of PATH?

I'd suggest adding two tests

Will do.

[pfmoore]

So, should we instead be checking if the installed path contain scripts that would be installed out of PATH?

Personally, I don't think this change is worth it in any case. But as long as the 2 tests I suggested pass, I don't really mind how it's done.

[dstufft]

I think this change is worth it, since in a future where we default to --user, we don't want pip install foo && foo to raise an error that it can't find foo. I think this also applies to Windows installs that aren't using the latest installer?

In any case, I agree with @pfmoore that we should not be warning if it's not actually going to be doing something wrong, and I think that instead of explicitly focusing on --user, we should be warning anytime we're going to install a script outside of $PATH. The one modulation to that would be we shouldn't issue that same warning if we're using --target, and possibly also inside of a virtual environment (since people might be doing bin/foo instead).

[BrownTruck]

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 master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[pradyunsg]

So, the approach I'm thinking of taking here is adding the warning conditional to a user specified flag.

I'd actually not want to add virtualenv as a special case -- the warning could include how-to-suppress message; something along the lines of:

pip config {--venv,--global} set install.warn_about_script_path False

[pradyunsg]

/request-review @pfmoore @dstufft

[xavfernandez]

I like this change also 👍

[pradyunsg]

Thanks for the review @xavfernandez!

I'll fix these when I rebase/merge.

[pfmoore]

Thanks for this @pradyunsg 😄

[pradyunsg]

You're welcome! ^>^

[asottile]

Should this only warn for --user installs? A pretty common workflow is to:

RUN virtualenv venv
RUN venv/bin/pip install ...
CMD ["dumb-init" "venv/bin/exe", ...]

(or outside of a dockerfile, similarly)

virtualenv venv
venv/bin/pip install ...
venv/bin/exe ...

(Just noticed this spamming my terminal with a lot of undesirable output after trying out the new 10.x betas)

[pfmoore]

Hmm, yes that's a very good point. We shouldn't really be warning in that situation. @pradyunsg any thoughts on this?

[pradyunsg]

You're right, this is a common use case this PR did not cover.

My current thought is: if the executable is installed in dirname(sys.executable), we can skip the warning. Basically, treat dirname(sys.executable) as being a directory on PATH.

Pinging @dstufft @ncoghlan @xavfernandez for inputs.

[pfmoore]

Chipping in with my release manager hat on, I think that we need some solution to avoid spamming the user with warnings when installing into a non-activated virtualenv (IMO, using a virtualenv without activating it is an important use case). I'll leave the interested parties to agree on a suitable solution.

I'm going to mark this as a release blocker so it doesn't get forgotten about.

[pradyunsg]

if the executable is installed in dirname(sys.executable), we can skip the warning. Basically, treat dirname(sys.executable) as being a directory on PATH.

FWIW, this is a matter of adding one line: path_env_var_parts.append(dirname(sys.executable)).

[ncoghlan]

+1 for @pradyunsg's suggested resolution - not sure if you want to convert this into a full issue before resolving it :)

[pradyunsg]

not sure if you want to convert this into a full issue before resolving it

Yeps -- #5157. :)

@pfmoore I'm removing "release blocker" label from this -- I imagine a dedicated issue would easier for you to track. :)

[pfmoore]

Thanks, that's fine.

[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.