Skip to content

Conversation

Kelledin
Copy link
Contributor

test_typing_compiles_with_opt() is prone to failure if building and testing with an interpreter that doesn't match the default python in $PATH (for example, if running python3 setup.py ... when the default system-wide interpreter is Python 2.x).

Using sys.executable (vs hardcoded unversioned "python") seems to be the most sensible solution.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that this will break weirdly when sys.executable has a space in it.

Also, please fill out the CLA before we can proceed.

@Kelledin
Copy link
Contributor Author

Kelledin commented Aug 1, 2019

So...simple quoting appears to address spaces in the interpreter path. Then I thought about it little more and decided we might as well account for spaces in file_path as well.

If we want to handle any more complex corner cases (like quote characters embedded in file names), we'd probably be better off breaking the command into an array and leaving shell=False. Unless there's any particular reason we set shell=True?

@gvanrossum
Copy link
Member

I don't see any deep reason why shell=True was used here except convenience. So please go ahead!

@Kelledin Kelledin force-pushed the test_use_sys.executable branch from ba2cc47 to 9f8bc95 Compare August 2, 2019 20:03
@ilevkivskyi
Copy link
Member

@Kelledin Please don't force-push because GitHub doesn't send a notification in this case.

@gvanrossum It looks like your last comment is now addressed. So I think this can be merged now.

@ilevkivskyi ilevkivskyi merged commit 6f7daad into python:master Aug 7, 2019
@gvanrossum
Copy link
Member

gvanrossum commented Aug 7, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants