Skip to content

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

Merged
henryiii merged 5 commits into
pybind:masterfrom
MarkusBauer:fix-paths-with-spaces
Aug 14, 2024
Merged

fix: escape paths with spaces in pybind11-config#4874
henryiii merged 5 commits into
pybind:masterfrom
MarkusBauer:fix-paths-with-spaces

Conversation

@MarkusBauer

@MarkusBauerMarkusBauer commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

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

Comment threadpybind11/__main__.py Outdated
rwgk
rwgk approved these changes Oct 5, 2023

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment threadpybind11/__main__.py Outdated
Comment threadpybind11/__main__.py Outdated
@henryiii

henryiii commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator

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.

@henryiii
henryiiiforce-pushed the fix-paths-with-spaces branch from 4099e56 to 95b1bedCompareAugust 14, 2024 16:37
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Comment threadpybind11/__main__.py Outdated
@henryiiihenryiii changed the title --includes - escape paths with spaces in include listfix: escape paths with spaces in pybind11-configAug 14, 2024
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

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:masterAug 14, 2024
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Aug 14, 2024
@QuLogic

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

Released.

@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Aug 22, 2024
Sign up for freeto 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

@MarkusBauer@henryiii@QuLogic@rwgk