--includes - escape paths with spaces in include list

[MarkusBauer]

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:

[rwgk]

The shlex.quote documentation has this big red warning:

https://docs.python.org/3/library/shlex.html#shlex.quote

Warning The shlex module is only designed for Unix shells.

Did you already think about this? What is the behavior under Windows? From really rusty memory, I think Windows may only understand " but not ', is that true?

[MarkusBauer]

Just tested the behavior on Windows:

• everything works fine with Powershell
• old CMD does not understand the new quoting
• backslashes trigger the escaping, so any path with backslashes will get invalid on Windows

I see three possible solutions, both have their drawbacks:

• pull in an additional dependency (oslex or mslex)
• do the quoting manually on Windows (which might or might not be a complete implementation - at least we're not talking about untrusted data here)
• disable the quoting on Windows (and accept the bug on one platform)

I can update the implementation if you chose your preferred way.

[rwgk]

oof :-)

I'm really out of touch with the Windows world.

pull in an additional dependency (oslex or mslex)

That sounds best to me.

But not sure what makes sense:

• try-except import oslex or mslex
• if available: all good
• if not available: check for obvious failure conditions (e.g. spaces, maybe backslashes?) and raise an exception
• otherwise just skip the quoting and hope for the best

[MarkusBauer]

What about such a construct? Pick oslex if available, shlex as linux fallback and stupid windows fallback otherwise? The Windows fallback catches at least the most common case (spaces).

try:
    from oslex import quote
except:
    import os
    if os.name != 'nt':
        from shlex import quote
    else:
        def quote(s):
            return '"' + s + '"' if ' ' in s else s

[rwgk]

Looks good to me!

Nits:

except ImportError:

if os.name != "nt":

(I think black will insist on that)

Maybe also add a one-line comment for the fallback quote() implementation, something like

# Minimal attempt, handling only the most common case.

[MarkusBauer]

I added oslex and the proposed windows fallback.

However, it looks to me like pybind11 has zero python runtime dependencies so far, and therefore does not have anything like requirements.txt. Did I miss something?
If this is true, I suggest that we don't introduce that and keep oslex purely optional.

[rwgk]

The current version of this PR looks good to me.

We have requirements.txt only under tests/.

If this is true, I suggest that we don't introduce that and keep oslex purely optional.

"that" == requirements.txt?

Sounds good to me: this PR only makes things better, without introducing hard new dependencies.

[MarkusBauer]

"that" == requirements.txt? - yes. Not necessary to introduce additional dependency management here.

[henryiii]

Little worried about adding a dependency with two stars on GitHub. :) Also just assuming it might be present, rather than making it, for example, conditional. I think this is a bit better than the status quo, though, so I'd be fine with it.

[rwgk]

Thanks!

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