-
Notifications
You must be signed in to change notification settings - Fork 414
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
Potential fix for pylauncher with python version #1186
Conversation
TLDR; in my opinion, this change is good Testing on Linux, this change causes expected behaviour more often and prints a helpful warning message. The change also means that if a version of Python is not installed, all three ways of specifying that version (e.g. (1) This testing also illustrates another part of #1150 ; without the python launcher installed and using either the main branch or this one the results include five different trace backs. It can be difficult to see how these relate to the command that triggered them. I think my expectation was that these would print a message from which I could understand I need to install the launcher. I appreciate that would be a separate PR to this one. Testing the main branch for a baseline understanding:
Summary
Testing the
The summary is as above. Testing the
Summary:
Expected output
Trace back 1
Trace back 2
Trace back 3
Trace back 4
Trace back 5
Trace back 6
|
I'll convert this to draft for now as it only fixes a subset of the mentioned problems. |
I've added some changes to unify how the python interpreter is evaluated based on the --python flag. Still need to clean up the now redundant checks and add some tests, but looks promising IMO. In my testing I am unable to run into any tracebacks, except for the one mentioned in #1195. Testing the pylauncher_fix_version branch: No Pylauncher, 3.11 installed, 3.10 not installed pipx run pycowsay moo # expected output Pylauncher, 3.11 installed, 3.10 not installed pipx run pycowsay moo # expected output warning1: warning2: warning3: warning4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rebase and add changelog.
docs/changelog.md
Summary of changes
When pylauncher is detected, check the provided python version to see if it was provided including
python
(as per our examples). If so, log a warning and remove python from the version, leaving only the semver, as expected by pylauncher.This might fix #1150
Test plan
Not tested yet, as I'm not on windows and I don't have pylauncher. I've added an automated test that might verify the new behavior.
Maybe someone using pylauncher can assist me here with a manual validation.