Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

ankith26 · 2022-04-21T15:47:21Z

The pip warning that shows up when pip is running on an older version, gives users a command that invokes python via sys.executable. While this is less error-prone, it can potentially give a rather verbose command, and does not handle the case where there could be spaces in sys.executable (which I've seen, tends to confuse a lot of newbies new to pip and working with shells, when the command recommended in the warning does not work for them due to having spaces)

This PR tries to make this warning use python/python3/pythonX.Y if available on path, falling back to trying to use a py launcher version of the command (with some sanity checking) and finally falling back to old behaviour of using sys.executable if the above methods did not work.

This PR is not very robust either... since it involves running an arbitrary command py, which could potentially point to some random executable on the users setup (could this be a security risk?) although I imagine this would be super rare. Either way, I'd be happy to hear if there's another way of doing what I want to implement.

uranusjr · 2022-04-22T07:16:02Z

Not sure this entire logic (and much more importantly the maintenance and bug reports from edge cases this inevitably can’t catch 100%) is worth it.

pfmoore · 2022-04-22T08:27:57Z

Haven't we already had this discussion a few times? I'm pretty sure the conclusion was that while the existing suggestion isn't perfect, we can never actually make it perfect, and most changes to improve one case make a different case worse. So we should keep it simple, and accept that people have to understand it's a suggestion, not "here, copy/paste this command".

pradyunsg · 2022-04-22T08:35:56Z

There's also #10959, which takes a lighter weight approach and makes a UI improvement as well.

ankith26 · 2022-04-22T10:38:44Z

Well, I don't see any edge cases atleast in the first bit I am proposing, which is fairly simple: use shutil.which to tell if a commonly used python* command exactly matches sys.executable. And this should make things less verbose for like 80% of the pip users

The pylauncher bit is trickier tho, I see it's hacky and could be a potential minor maintenance burden down the line. But since this warning is only a suggestion (like pointed above by pfmoore), it probably cannot introduce critical release blocker issues. I did keep in mind to try to make it much easier to fallback to sys.executable rather than giving wrong pylauncher command suggestions in the warning.

I took a look at the linked PR, and that PR is looking good too, which handles pip*, and python* like this PR, but this PR also tries to do pylauncher

notatallshaw · 2022-04-22T14:51:59Z

@pfmoore This PR is different than previous discussions because it is only trying to suggest to use python (or some variant) rather than attempting to escape spaces correctly.

I've spent a bit of time looking over this and trying to think of possible edge cases (e.g. in working directory x which has a python executable, but running a python from directory y, but there is also a python in directory z which is on the PATH) and I think it's a pretty good approach.

I'm personally a bit iffy on it's usage of subprocess to call py and then parse the outputs. Though I've gone through all the code and it does seems to capture all edge cases I can think of. Pip does use subprocess in other areas, so the Python implementation must support subprocess to use Pip already.

pfmoore · 2022-04-22T15:07:57Z

src/pip/_internal/self_outdated_check.py

+        # docs (python 3.10) says that sys.executable can be can be None or an
+        # empty string if this value cannot be determined, although this is
+        # very rare. In this case, there is nothing much we can do
+        return "python3"


This would be wrong on Windows, where there's never a python3 executable. I'm not sure giving the wrong answer is better than explicitly failing to give any answer.

I'm inclined to fail in this case.

I was inclined to spit an error too, but I did not want the error to be too disruptive. But I see, giving a wrong suggestion does not help much either, this can be changed to put another warning instead (in place of the upgrade suggestion warning)

pfmoore · 2022-04-22T15:11:46Z

src/pip/_internal/self_outdated_check.py

+            # resolve() removes symlinks, normalises paths and makes them
+            # absolute
+            if Path(which).resolve() == sys_executable_path.resolve():
+                return py


What if the user has a shell alias that overrides what shutil.which returns? Then what we suggest won't work as we expect it to.

Also on a case-insensitive filesystem, this == needs to be a case insensitive comparison (although this will simply miss some possible abbreviations, not return something wrong).

In both these cases won't this check return False then and correctly use the sys.executable fallback?

What if the user has a shell alias that overrides what shutil.which returns? Then what we suggest won't work as we expect it to.

Hmmm that's a good point... I can think one workaround, looking into os.environ for aliases, but this would only work on Windows and cmd shell (yeah, would not even work with something like powershell for instance). Trying to workaround this for all other platforms/configurations seems impractical and error-prone, definitely high-maintenance for a relatively minor thing.

I believe #10959 too would also suffer from the same issue

Also on a case-insensitive filesystem, this == needs to be a case insensitive comparison (although this will simply miss some possible abbreviations, not return something wrong).

Well... We are comparing Path objects here so this is already handled internally by pathlib. Under windows it uses WindowsPath which does case insensitive comparison.

pfmoore · 2022-04-22T15:13:25Z

src/pip/_internal/self_outdated_check.py

+    # executable.
+    try:
+        proc = subprocess.run(
+            ["py", "--list-paths"],


Not all versions of the launcher support --list-paths - how can we be sure the user doesn't have an old version? I guess it just fails to find anything, so that's sort of fine.

Isn't that why it's an a try/except block and if that's the case correctly use the sys.executable fallback?

FYI it appears to be part of the Windows CPython 3.7 release onward and the documented flag to use is -0p not --list-paths: https://github.com/python/cpython/blob/3.7/Doc/whatsnew/3.7.rst#windows-only-changes

Also, upgrading the launcher is optional, so people could end up using a later Python but an old launcher (one reason being if the launcher is installed globally, needing admin rights).

Isn't that why it's an a try/except block and if that's the case correctly use the sys.executable fallback?

Yeah, that's why I said "I guess it just fails to find anything, so that's sort of fine"...

Yeah, it would fallback in this case (EDIT: github was being weird, when I sent this message it appeared that it was the first reply on this review chain, after a browser reload this comment seems to have come a few comments below)

pfmoore · 2022-04-22T15:16:31Z

src/pip/_internal/self_outdated_check.py

+            if platform.system() == "Windows":
+                invalid_chars += '<>:"\\|?*'
+
+            line_path = line_path.strip(invalid_chars)


strip() only removes invalid characters from the start and end. Why would you want to do that? Also, why are you doing this anyway? If the returned value is invalid, what are you trying to achieve by making it valid?

pfmoore · 2022-04-22T15:19:36Z

This PR is different than previous discussions because it is only trying to suggest to use python (or some variant) rather than attempting to escape spaces correctly.

OK. I still don't think it's worth it but I won't object if people disagree.

I'm personally a bit iffy on it's usage of subprocess to call py and then parse the outputs.

Yes, I don't really like that at all.

Also, I've added some review comments noting places I think there could be issues. However, these are really just for reference - even if they are "fixed" I still won't be in favour of making this change.

@ankith26 Thanks for taking the time to contribute, and I'm sorry my feedback is negative - but unfortunately there's history here which you weren't aware of which makes this a lot less straightforward than I imagine you hoped 🙁

pradyunsg · 2022-04-22T15:28:56Z

I took a look at the linked PR, and that PR is looking good too, which handles pip*, and python* like this PR, but this PR also tries to do pylauncher

I think that this PR duplicates a couple of things from #10959. That PR does many more things in addition to providing a better presentation for pip and python (when they're first on the PATH) -- it refactors the entirety of the self-check logic, reworks the presentation of this message and makes changes to the logging architecture to support more generic "rich" output messages.

As it stands, the changes here are functionally scoped to be an improvement in the get_best_invocation_for_this_python introduced in the other PR (IIUC, that'd be py launcher support, handling missing sys.executable etc). I'm not too familiar with the details of py launchers on Windows/Unix so... I'm not the best person to comment on that. :)

ankith26 · 2022-04-22T16:35:09Z

Yes... I'm not happy with the hacky py --list-paths parsing myself. It would be very handy if we had something like py --list-json to give a JSON dict of version-path values, but that's something for upstream cpython. It could also benefit other tools trying to interface with the py launcher, but it's probably a pretty niche usecase and wouldn't be a game changer in any way.

Yeah this PR did end up being less straightforward than I initially thought, but it's fine; thanks for the quick responses and reviews!

I think this PR could be a draft for a while until a better solution comes up hopefully (I might look into it more to try and find something), this PR could be closed too if there's no progress on this

pradyunsg · 2022-09-30T13:20:06Z

Going to go ahead and close this, since it has merge conflicts that haven't been resolved in a while. Please feel welcome to file a new PR or to reopen and update this one!

I'll note that we've made significant improvements to the outdated check in #10959.

Thanks for filing this PR @ankith26!

ankith26 force-pushed the pylauncher-cmd branch from d2c9ae5 to fbb385d Compare April 21, 2022 15:49

ankith26 added 2 commits April 22, 2022 09:45

Suggest python/pylauncher command in pip warning

7d69302

Create 11057.feature.rst

b22e735

ankith26 force-pushed the pylauncher-cmd branch from fbb385d to b22e735 Compare April 22, 2022 04:15

pfmoore reviewed Apr 22, 2022

View reviewed changes

github-actions bot added the needs rebase or merge PR has conflicts with current master label Apr 28, 2022

pradyunsg closed this Sep 30, 2022

ankith26 deleted the pylauncher-cmd branch September 30, 2022 14:02

github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

ankith26 commented Apr 21, 2022

uranusjr commented Apr 22, 2022

pfmoore commented Apr 22, 2022

pradyunsg commented Apr 22, 2022

ankith26 commented Apr 22, 2022

notatallshaw commented Apr 22, 2022 •

edited

Loading

pfmoore Apr 22, 2022

pradyunsg Apr 22, 2022

ankith26 Apr 22, 2022

pfmoore Apr 22, 2022

notatallshaw Apr 22, 2022

ankith26 Apr 22, 2022

pfmoore Apr 22, 2022

notatallshaw Apr 22, 2022

notatallshaw Apr 22, 2022

pfmoore Apr 22, 2022 •

edited

Loading

ankith26 Apr 22, 2022 •

edited

Loading

pfmoore Apr 22, 2022

pfmoore commented Apr 22, 2022

pradyunsg commented Apr 22, 2022 •

edited

Loading

ankith26 commented Apr 22, 2022

pradyunsg commented Sep 30, 2022

Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

Try to suggest python/python3/pythonX.Y/pylauncher command in pip warning #11057

Conversation

ankith26 commented Apr 21, 2022

uranusjr commented Apr 22, 2022

pfmoore commented Apr 22, 2022

pradyunsg commented Apr 22, 2022

ankith26 commented Apr 22, 2022

notatallshaw commented Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

ankith26 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore commented Apr 22, 2022

pradyunsg commented Apr 22, 2022 • edited Loading

ankith26 commented Apr 22, 2022

pradyunsg commented Sep 30, 2022

notatallshaw commented Apr 22, 2022 •

edited

Loading

pfmoore Apr 22, 2022 •

edited

Loading

ankith26 Apr 22, 2022 •

edited

Loading

pradyunsg commented Apr 22, 2022 •

edited

Loading