-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Do not call python directly but use sys.executable. Fixes #5872 #5873
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
Conversation
Thanks! Just for reference: it is best to use branches for PRs. Normally not really a problem, when it could be squash-merged, but this is disabled here (#4361). |
Use sys.executable to detect which python we should actually be testing.
fp.write('COMP_WORDBREAKS="$COMP_WORDBREAKS" python -m pytest 8>&1 9>&2') | ||
fp.write( | ||
'COMP_WORDBREAKS="$COMP_WORDBREAKS" {} -m pytest 8>&1 9>&2'.format( | ||
sys.executable |
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.
should ~probably be shlex.quote(sys.executable)
since we're sticking it into a shell script
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.
Oh that's true. Could you open a PR with that change Anthony?
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.
yep!
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.
{!r}
might be enough though.
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.
repr is not a substitute for proper shell quoting, that's asking for shell injection ;)
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.
Sure.. but this is something we have control over here more or less.
It's good to use the proper way though, of course.. :)
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.
we don't control this at all though, it's a user's directory -- they could do all sorts of wild things with the directory they clone/install pytest into
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.
I've meant the user / test runner - who is setting/using sys.executable
.
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.
of course, that doesn't stop people from having spaces in their development dirs, or non-ascii usernames, or having quotes in their names
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.
Spaces would be handled by the repr()
then.
Anyway, your fix is better.
Fixes #5872.