Skip to content
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

fix: escape paths with spaces in pybind11-config #4874

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

MarkusBauer
Copy link
Contributor

@MarkusBauer MarkusBauer commented Oct 5, 2023

Description

tl;dr: pybind11 --includes returns invalid arguments when the project's directory contains spaces.

When you build an extension manually, the docs suggest this command: c++ ... $(python3 -m pybind11 --includes) ....
However, this command fails when the project's directory contains a space in its path.
The command would then expand to c++ ... -I/home/user/a b/c -I/python ..., where /home/user/a is a in invalid include directory, and b/c is an invalid compiler argument.

Even if no manual build is desired this bug might affect users: for example, we're currently building clang-tidy in our project, which requires all include paths.

To solve this issue, this PR escapes include paths of pybind11 --includes before printing. Paths without spaces/special characters are printed as-is, while paths with spaces are wrapped in ''. Thus, the final command line of custom builds will be valid even if spaces occur: c++ ... '-I/home/user/a b/c' -I/python ...

Suggested changelog entry:

* Fix paths with spaces, including on Windows

pybind11/__main__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks!

Waiting for @henryiii to help with a 2nd set of eyes.

pybind11/__main__.py Outdated Show resolved Hide resolved
pybind11/__main__.py Outdated Show resolved Hide resolved
@henryiii
Copy link
Collaborator

henryiii commented Aug 14, 2024

I'd forgotten about this. Unless mslex modernizes a bit (it currently uses setup.py with deprecated arguments, like test_requires), I think I'd like to just have the custom form (like meson). I think we can go a bit simpler, though, since double quotes are not valid in a Windows path, only single quotes are. I'll update this.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
pybind11/__main__.py Outdated Show resolved Hide resolved
@henryiii henryiii changed the title --includes - escape paths with spaces in include list fix: escape paths with spaces in pybind11-config Aug 14, 2024
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

... alright ...

I cannot honestly write "looks good to me" here, but it seem beyond anyone's capacity (definitely my own) to do something actually good here :-)

I'm holding out hope that some future Python version will come with a platform-aware rock-solid quote() implementation out of the box.

@henryiii henryiii merged commit d893f97 into pybind:master Aug 14, 2024
78 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 14, 2024
@QuLogic
Copy link
Contributor

QuLogic commented Aug 14, 2024

FWIW, I just tried this on Windows, and everything is quoted enough to work properly, and cl no longer warns about non-existent paths.

henryiii added a commit that referenced this pull request Aug 15, 2024
* fix: Escape paths with spaces in include list from --includes

* fix: --includes should not use shlex on Windows platforms

* Apply suggestions from code review

* fix: use custom impl

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

* Support trailing backslashes

Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: Markus Bauer <markus.bauer@cispa.saarland>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@henryiii
Copy link
Collaborator

Released.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 22, 2024
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.

4 participants