Skip to content

Split CC to make sure we get the correct basename of the compiler #54

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

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Sep 16, 2021

See the test for an example where basename of CC value fails.

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

Hum, I think this might be a little bit trickier.

There isn't really a spec for the CC env var, though I think the de facto standard is what make defines.
https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html

The issue is that make expands the commands like a shell, which allows use quotes, eg. CC="'this path/contains spaces!/my cursed compiler'" --some-arg.

This patch does not take quoting into account. I would expect the example I presented to work.
The current implementation does not support quoting anyway, so we could possibly merge this without that. But if we are changing the handling to align with the behavior of common build systems, we probably should make it work as expected.

Also, simply splitting on spaces also does not take into account that spaces might be escaped, eg. CC=this\ path/contains\ spaces!/my\ cursed\ compiler --some-arg, so it will break things.

I think we really need to expand the text to arguments.
Here's my naive implementation: https://gist.github.com/FFY00/0029d3001b384ff974360cd6c69b39f8

@isuruf
Copy link
Contributor Author

isuruf commented Sep 16, 2021

Also, simply splitting on spaces also does not take into account that spaces might be escaped, eg. CC=this\ path/contains\ spaces!/my\ cursed\ compiler --some-arg, so it will break things.

getting the runtime library option might break, but compilers with spaces in them is already broken in distutils.
For eg: CC='"this\ path\ compiler" --some-arg' python setup.py build_ext doesn't work
neither does CC='this\ path\ compiler --some-arg' python setup.py build_ext

@FFY00
Copy link
Member

FFY00 commented Sep 19, 2021

Looks like the state is worse than what I was expecting then 😅

I am not sure if this is the right fix, though I would probably tend to yes.

@jaraco what do you think?

@jaraco
Copy link
Member

jaraco commented Sep 21, 2021

I initially thought I wasn't going to have an opinion on the PR, but then I looked at the change, and had much of the same reaction as FFY00. I'd like not to introduce broken support for parsing the CC var with respect to spaces... and in particular, I'd like not to use .split(' ') unless the space character is defined as being an exclusive delimiter. In other words, while distutils may not support space-separated paths currently, I'd like not to add behavior that further reduces compatibility for such paths.

Co-authored-by: "Jason R. Coombs" <[email protected]>
@jaraco jaraco merged commit 02e9f65 into pypa:main Sep 21, 2021
@isuruf
Copy link
Contributor Author

isuruf commented Sep 21, 2021

Thanks @FFY00 and @jaraco

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

Successfully merging this pull request may close these issues.

3 participants