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

Command line for notice update not copy-pasteable for venv #13156

Open
1 task done
roubert opened this issue Jan 12, 2025 · 30 comments
Open
1 task done

Command line for notice update not copy-pasteable for venv #13156

roubert opened this issue Jan 12, 2025 · 30 comments
Labels
type: bug A confirmed bug or unintended behavior

Comments

@roubert
Copy link

roubert commented Jan 12, 2025

Description

When using venv and pip like this:

$ python3 -m venv test
$ test/bin/pip install […]
[notice] A new release of pip is available: 24.3 -> 24.3.1
[notice] To update, run: python3 -m pip install --upgrade pip

Then that command line isn't directly copy-pasteable, as it doesn't include the required path to the venv.

Expected behavior

It'd be more helpful if the command line was directly copy-pasteable, including the venv path (when needed).

pip version

24.3.0

Python version

3.12.7

OS

Linux

How to Reproduce

$ python3 -m venv test
$ test/bin/pip --version
pip 24.3 from /tmp/test/lib/python3.12/site-packages/pip (python 3.12)
$ test/bin/pip install pip-install-test
Collecting pip-install-test
  Using cached pip_install_test-0.5-py3-none-any.whl.metadata (979 bytes)
Using cached pip_install_test-0.5-py3-none-any.whl (1.7 kB)
Installing collected packages: pip-install-test
Successfully installed pip-install-test-0.5

[notice] A new release of pip is available: 24.3 -> 24.3.1
[notice] To update, run: python3 -m pip install --upgrade pip

Output

No response

Code of Conduct

@roubert roubert added S: needs triage Issues/PRs that need to be triaged type: bug A confirmed bug or unintended behavior labels Jan 12, 2025
@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2025

The command isn't intended to be copy-pastable. There are too many edge cases (such as spaces or special characters in path names) which would require shell-specific quoting, and rather than have something that looks copy-pastable, but often isn't, we prefer to explain to the user what they need to do in more general terms.

@pfmoore pfmoore added type: docs Documentation related resolution: no action When the resolution is to not do anything and removed type: bug A confirmed bug or unintended behavior S: needs triage Issues/PRs that need to be triaged labels Jan 12, 2025
@roubert
Copy link
Author

roubert commented Jan 12, 2025

The problem is that the current message looks copy-pasteable and in the vast majority of cases when copy-pasted it'll run and perform an update, just not updating the right thing.

I filed this after having had to help out explaining why the same message kept reappearing despite running the command.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2025

If you have any suggestions for better wording, that would be fine. But something that can be copy-pasted is explicitly not a goal here. Consider if you had a virtual environment /home/André/my envs/env$1 - how would you make a message that was "directly copy-pastable"? Remember that you need to consider users of the CMD shell (where '...' quotes don't work) and Powershell (where any quoted path needs to be preceded by &) as well as any number of Unix shells.

@roubert
Copy link
Author

roubert commented Jan 12, 2025

I actually believe that such a command line, which resulted in an error message when copy-pasted if quoting was required, really would be a little bit more helpful than the current one which appears to work and update something, but updates the wrong thing.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2025

We've had user reports saying the exact opposite in the past, unfortunately 🙁 This is one of those cases where you can't please everyone, no matter how hard you try, unfortunately.

@roubert
Copy link
Author

roubert commented Jan 12, 2025

Maybe one improvement could then be to make sure that the message doesn't include any command line that can be copy-pasted and executed directly, which would eliminate the problem of mistakenly updating the wrong thing.

@roubert
Copy link
Author

roubert commented Jan 12, 2025

I just came to realize one thing, this message already includes unquoted spaces (and also the full path into the venv), if the Python interpreter is invoked through some nonstandard name, so if I were to do a ln -s python3 'python 3' inside of my venv (and also give the venv a name with a space in it too, just for completeness), I'd get this:

$ 'te st/bin/python 3' -m pip install pip-install-test 
Collecting pip-install-test
  Using cached pip_install_test-0.5-py3-none-any.whl.metadata (979 bytes)
Using cached pip_install_test-0.5-py3-none-any.whl (1.7 kB)
Installing collected packages: pip-install-test
Successfully installed pip-install-test-0.5

[notice] A new release of pip is available: 24.3 -> 24.3.1
[notice] To update, run: /tmp/te st/bin/python 3 -m pip install --upgrade pip

So it seems that the risk of outputting an unquoted space hasn't been a concern for the current implementation and that a really straightforward improvement could be to treat an invocation from inside of a venv the same as using a nonstandard interpreter name.

@d97jro
Copy link

d97jro commented Jan 12, 2025

Second that.

@notatallshaw
Copy link
Member

notatallshaw commented Jan 12, 2025

See related discussion: #12411, #12412, #10987, #10734

So it seems that the risk of outputting an unquoted space hasn't been a concern for the current implementation and that a really straightforward improvement could be to treat an invocation from inside of a venv the same as using a nonstandard interpreter name.

Can you give a concrete example of your suggestion, what it looks like now, what it would look like after.

I'm struggling to follow your reasoning here, as currently it is accepted to have an unquoted space because it is not a goal to copy and paste, but meant to be illustrative for the user. I'd need to see real examples to see if what you're suggesting improves the illustrativeness for users.

@roubert
Copy link
Author

roubert commented Jan 12, 2025

Can you give a concrete example of your suggestion, what it looks like now, what it would look like after.

Sure. Currently the message looks like this:

[notice] To update, run: python3 -m pip install --upgrade pip

The problem with that is that even if it isn't intended to be copy-pasted it very much looks as if it is and when someone does copy-paste it, it (in pretty much every normal enviroment) is a valid command line that performs an upgrade and looks as if it did the right thing, even when it didn't upgrade the installation inside of the venv, but another one.

My two suggestions for possible ways to eliminate that problem are:

  1. Make sure that the command line can't be copy-pasted and executed, eg. something like this:
[notice] To update, run: [PYTHON3] -m pip install --upgrade pip
  1. Handle an invocation from inside of a venv the same as an invocation through a nonstandard interpreter name is already handled, ie. by including the path to the Python interpreter like this:
[notice] To update, run: /tmp/test/bin/python3 -m pip install --upgrade pip

If you want to stress that this command line isn't supposed to be copy-pasted, you'd probably prefer 1, but anyone who finds it convenient to be able to copy-paste the command line will probably prefer 2.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2025

Make sure that the command line can't be copy-pasted and executed

I'm not keen on this - it feels like unnecessary obfuscation for what is to be honest a problem that most people don't seem to have.

Handle an invocation from inside of a venv the same as an invocation through a nonstandard interpreter name is already handled

I'm curious how you expect to detect "inside of a venv". It would probably be best if you created a PR demonstrating the precise change you're suggesting that we make. That doesn't imply a commitment to accept such a PR, it's just that without actual code it's hard to be sure everyone means the same thing.

@roubert
Copy link
Author

roubert commented Jan 12, 2025

I'm curious how you expect to detect "inside of a venv". It would probably be best if you created a PR demonstrating the precise change you're suggesting that we make.

I'm not at all familiar with this code base, but my possibly naïve assumption is that this can be done as simply as PR #13161 (commit ed0b04b). That change achieves my proposed alternative 2 while all already existing tests still pass.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2025

Ah, OK. I see what you mean now, thanks. Although if that's the code you're thinking of, I don't see why you're getting the behaviour you described in the first place:

$ python3 -m venv test
$ test/bin/pip install […]
[notice] A new release of pip is available: 24.3 -> 24.3.1
[notice] To update, run: python3 -m pip install --upgrade pip

The code says:

    # Try to use the basename, if it's the first executable.
    found_executable = shutil.which(exe_name)
    if found_executable and os.path.samefile(found_executable, exe):
        return exe_name

    # Use the full executable name, because we couldn't find something simpler.
    return exe

But surely, found_executable will be the system Python (as you didn't activate the environment), meaning that the samefile check will fail, and the code will fall back to printing the full executable name anyway.

So it seems like there's something else going on here, and we should fix that rather than just hiding the problem (which is what your PR does, in effect).

@notatallshaw
Copy link
Member

notatallshaw commented Jan 12, 2025

Yeah, I'm confused here why @roubert isn't already getting the behavior they expect.

I think it would be helpful to know:

  1. Is venv activated?
  2. What is the exact command you are running and what is it's full output?
  3. What is the output of which python3?
  4. What is the output of python3 -c 'import shutil; print(shutil.which("python3"))'?

@roubert
Copy link
Author

roubert commented Jan 13, 2025

  1. Is venv activated?

No.

  1. What is the exact command you are running and what is it's full output?

See "How to Reproduce" above.

  1. What is the output of which python3?
/usr/bin/python3
  1. What is the output of python3 -c 'import shutil; print(shutil.which("python3"))'?
/usr/bin/python3

@roubert
Copy link
Author

roubert commented Jan 13, 2025

So it seems like there's something else going on here,

So it seems like the "resolution: no action" label should be removed from this issue.

@notatallshaw
Copy link
Member

Is pip installing into the venv or into the environment that /usr/bin/python3 belongs to?

@roubert
Copy link
Author

roubert commented Jan 13, 2025

Is pip installing into the venv […]?

Yes, I verified this just now.

@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

Can you run the test again but patch test/bin/pip to print sys.executable in get_best_invocation_for_this_pip?

@roubert
Copy link
Author

roubert commented Jan 13, 2025

Can you run the test again but patch test/bin/pip to print sys.executable in get_best_invocation_for_this_pip?

Sure, the result of that is:

sys.executable='/tmp/test/bin/python3'

Do you get something different there?

@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

So what I don’t understand is why the os.path.samefile(found_executable, exe) check isn’t returning False

@roubert
Copy link
Author

roubert commented Jan 13, 2025

So what I don’t understand is why the os.path.samefile(found_executable, exe) check isn’t returning False

I would hope that someone familiar with this code base would be able to answer that, but I could debug it if you need me to.

@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

Well, exe is /tmp/test/bin/python3 as you've shown, and found_executable is /usr/bin/python3 (again as you've shown). So the question is why does os.path.samefile think they are the same file? That's the problem here, and as far as I can tell, it's a quirk of your system, rather than anything to do with pip's code. So really, it's down to you to explain why it's happening...

@roubert
Copy link
Author

roubert commented Jan 13, 2025

So the question is why does os.path.samefile think they are the same file?

As far as I can read the documentation, that is exactly the expected behaviour, the function returns true because one of those paths is a symlink to the other (which is exactly what's expected to be found in a venv):

https://docs.python.org/3/library/os.path.html#os.path.samefile

@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

Symlinks should not have the same device and inode. Only hard links should have that, and venvs don't (as far as I'm aware) use hard links.

@roubert
Copy link
Author

roubert commented Jan 13, 2025

Well, then you and I understand that documentation differently, but it's easy enough to check what the actual behaviour is:

#!/usr/bin/python3
import os
os.mknod('testfile')
os.symlink('testfile', 'testlink')
print(f'{os.path.samefile("testfile", "testlink")=}')

When I run that on my (normal Debian testing) Linux machine, I get:

os.path.samefile("testfile", "testlink")=True

Do you get a different result?

@pfmoore pfmoore added type: bug A confirmed bug or unintended behavior and removed type: docs Documentation related resolution: no action When the resolution is to not do anything labels Jan 13, 2025
@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

OK. It looks like os.path.samefile follows symlinks before checking device/inode, and it looks like venv on Unix uses symlinks for the interpreter executable.

So I'm left wondering how that code in get_best_invocation_for_this_pip has ever worked on Unix. I don't use Unix myself, so this is about as far as I can go in debugging. But it looks like there may be a bug here, but it's not something we should fix by checking for a virtual environment. Rather, we should fix the "is this the same executable as you find with a path search" check.

@pfmoore
Copy link
Member

pfmoore commented Jan 13, 2025

Thanks for sticking with this. I think your revised PR seems like a plausible fix, although I'd want someone who knows Unix better than me to verify it. Also, I'm still concerned over why no-one has reported this before - has something changed, or is this simply too trivial for anyone to have noticed it previously?

@roubert
Copy link
Author

roubert commented Jan 13, 2025

It seems plausible to me that this might never have worked, for if I myself hadn't had to play tech support and explain that the copy-pasted command line that reported the update to be successful didn't seem to have any actual effect because it hadn't updated the correct installation in the venv, then it seems unlikely that I would ever have felt motivated to report it.

In case the root cause really is just a misunderstanding of os.path.samefile() treating a symlink and its target as the same, the solution might be as simple as just not dereferencing symlinks, ie. using lstat() instead of stat(), so I've now updated PR #13161 to do just that.

This results in printing the full path when the Python interpreter is a symlink in a venv (as proposed in my "alternative 2" above) and all tests still pass.

But someone who's familiar both with this code base and preferrably also with Unix should take a closer look at it.

@eli-schwartz
Copy link
Contributor

Thanks for sticking with this. I think your revised PR seems like a plausible fix, although I'd want someone who knows Unix better than me to verify it.

The fix looks correct IMO.

Also, I'm still concerned over why no-one has reported this before - has something changed, or is this simply too trivial for anyone to have noticed it previously?

I suspect most people mentally edit out the message to read it as

[notice] To update, hit the up arrow in your shell, backspace a few times, and
[notice] rerun pip with the following args instead: install --upgrade pip

A subset of the remainder are terribly confused and can't get it to work, but give up without asking anyone. At some point they either decide they don't care "because the old stuff works fine anyway", upgrade Ubuntu and get a new pip version without knowing it, or delete and recreate a virtual environment with a brand new pip.

(The people who care most about having up to date pip are the ones who know a lot about python.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants