Should warn about unquoted spaces in variable values #153

New Issue

ariadne · 2017-11-02T07:47:14Z

nirbheek commented

2017-11-02 07:47:14 +00:00

(Migrated from github.com)

With a .pc file like this:

prefix=/usr/with spaces
libdir=${prefix}/lib
...

Libs: -L${libdir} -lfoo
Cflags: -I${includedir}/foo-2.0

The output looks like this:

$ pkg-config --libs foo
-L/usr/with spaces/lib -lfoo
$ pkg-config --libs-only-L foo
-L/usr/with

It seems that the tokenization is broken. In addition, the paths outputted are not escaped or quoted at all. The correct output would be:

$ pkg-config --libs foo
'-L/usr/with spaces/lib' -lfoo
$ pkg-config --libs-only-L foo
'-L/usr/with spaces/lib'

--cflags and --cflags-only-I are broken in the same way.

With a `.pc` file like this: ```ini prefix=/usr/with spaces libdir=${prefix}/lib ... Libs: -L${libdir} -lfoo Cflags: -I${includedir}/foo-2.0 ``` The output looks like this: ```console $ pkg-config --libs foo -L/usr/with spaces/lib -lfoo $ pkg-config --libs-only-L foo -L/usr/with ``` It seems that the tokenization is broken. In addition, the paths outputted are not escaped or quoted at all. The correct output would be: ```console $ pkg-config --libs foo '-L/usr/with spaces/lib' -lfoo $ pkg-config --libs-only-L foo '-L/usr/with spaces/lib' ``` `--cflags` and `--cflags-only-I` are broken in the same way.

nirbheek commented

2017-11-02 07:54:05 +00:00

(Migrated from github.com)

Note that pkg-config is broken in the same way and more, so projects that use paths with spaces are currently totally broken. This change will only improve the current situation and make pkg-config actually usable on Windows where paths with spaces are common.

It would be great to fix this because pkgconf is probably the best option for getting pkg-config on Windows and I've been looking into including it with our meson MSI installers. 😄

Note that pkg-config is broken [in the same way and more](https://bugs.freedesktop.org/show_bug.cgi?id=103203), so projects that use paths with spaces are currently totally broken. This change will only improve the current situation and make pkg-config actually usable on Windows where paths with spaces are common. It would be great to fix this because pkgconf is probably the best option for getting pkg-config on Windows and I've been looking into including it with our meson MSI installers. :smile:

ariadne commented

2017-11-03 05:53:49 +00:00

This is basically a duplicate of #139.

The solution in my opinion is to properly quote paths with spaces, e.g. prefix="/usr/with spaces".

The quoting should be double-quote to allow POSIX shell compatibility with fragments such as -I"/usr/with spaces"/include, as I understand it, single-quote is not compatible in that way.

This is basically a duplicate of #139. The solution in my opinion is to properly quote paths with spaces, e.g. `prefix="/usr/with spaces"`. The quoting should be double-quote to allow POSIX shell compatibility with fragments such as `-I"/usr/with spaces"/include`, as I understand it, single-quote is not compatible in that way.

nirbheek commented

2017-11-03 07:07:37 +00:00

(Migrated from github.com)

The solution in my opinion is to properly quote paths with spaces, e.g. prefix="/usr/with spaces".

That does work, but IMO pkgconf should output a warning when it finds unquoted space in variables. Every single pkg-config generator (via autoconf or whatever else) does not shell-quote the values of variables in .pc files, so they're all broken. It would also reduce the chances of someone else filing a duplicate bug. ;)

The quoting should be double-quote to allow POSIX shell compatibility with fragments

Single quotes work just fine for that, and they are superior because shell expansion cannot happen inside them— $foo, etc. $ is common in filenames on Windows, so I think it's worth preferring ' over ".

> The solution in my opinion is to properly quote paths with spaces, e.g. prefix="/usr/with spaces". That does work, but IMO pkgconf should output a warning when it finds unquoted space in variables. Every single pkg-config generator (via autoconf or whatever else) does not shell-quote the values of variables in .pc files, so they're all broken. It would also reduce the chances of someone else filing a duplicate bug. ;) > The quoting should be double-quote to allow POSIX shell compatibility with fragments Single quotes work just fine for that, and they are superior because shell expansion cannot happen inside them— `$foo`, etc. `$` is common in filenames on Windows, so I think it's worth preferring `'` over `"`.

nirbheek commented

2017-11-03 07:08:38 +00:00

(Migrated from github.com)

One more thing: some generators quote spaces with \ which is totally broken on Windows, so perhaps pkgconf should also warn about that.

One more thing: some generators quote spaces with `\` which is totally broken on Windows, so perhaps pkgconf should also warn about that.

nirbheek commented

2017-11-03 07:47:59 +00:00

(Migrated from github.com)

Hmm, quoting with ' or " does not ~~interact well with the usage of ${prefix} in variable values~~ work at all.

prefix="/usr/with spaces"
libdir="/usr/with spaces/lib"
...

Libs: -L${libdir} -lfoo
Cflags: -I${includedir}/foo-2.0

$ pkg-config --libs foo
-L/usr/with spaces/lib -lfoo

Escaping with \ does work, but as I said earlier, that does not work well on Windows because \ is a path separator.

Hmm, quoting with `'` or `"` does not ~~interact well with the usage of `${prefix}` in variable values~~ work at all. ```ini prefix="/usr/with spaces" libdir="/usr/with spaces/lib" ... Libs: -L${libdir} -lfoo Cflags: -I${includedir}/foo-2.0 ``` ```console $ pkg-config --libs foo -L/usr/with spaces/lib -lfoo ``` Escaping with `\` does work, but as I said earlier, that does not work well on Windows because `\` is a path separator.

ariadne commented

2017-11-10 04:51:47 +00:00

Yes it’s quite a mess unfortunately.

Spaces in paths have not gotten much consideration with pkg-config in general.

I am still thinking on how to best attack this.

Yes it’s quite a mess unfortunately. Spaces in paths have not gotten much consideration with pkg-config in general. I am still thinking on how to best attack this.

ariadne commented

2017-12-10 07:17:41 +00:00

So we are going to redesign the way we quote things in pkgconf 1.4 to follow POSIX instead of trying to do what the freedesktop implementation does (which I think we can all agree is not very sane).

I had previously wanted to go in this direction, but decided not to because I hadn't strongly familiarized myself with the POSIX standard in this regard, but after doing so, I believe that leveraging POSIX single-quotes as described in this bug is the right way forward.

This also makes it possible for us to do the right thing when running under cmd.exe (using double-quotes there) and other DCL derivatives (such as on OpenVMS).

So we are going to redesign the way we quote things in pkgconf 1.4 to follow POSIX instead of trying to do what the freedesktop implementation does (which I think we can all agree is not very sane). I had previously wanted to go in this direction, but decided not to because I hadn't strongly familiarized myself with the POSIX standard in this regard, but after doing so, I believe that leveraging POSIX single-quotes as described in this bug is the right way forward. This also makes it possible for us to do the right thing when running under cmd.exe (using double-quotes there) and other DCL derivatives (such as on OpenVMS).

🎉 1

nirbheek commented

2017-12-10 07:53:58 +00:00

(Migrated from github.com)

Great news, thanks for working on this. pkgconf is quickly becoming the recommended pkgconfig implementation for me on Windows. :)

It would be ideal if this implementation was somewhat compatible with the fdo implementation (it's a superset after all). Specifically, quoting spaces with \ inside .pc files would keep working, and would output arguments quoted with ':

prefix=/usr/with\ spaces
libdir=${prefix}/lib
...
Libs: -L${libdir} -lfoo

$ pkgconf --libs foo-2.0
'-L/usr/with spaces' -lfoo

Meson (and other projects) have to output pkgconfig files like that for compatibility with the fdo pkg-config implementation. Eventually we can get rid of this and use POSIX quoting inside the .pc file.

Meson already uses shlex.split() for splitting the output, and Autotools uses shell, so overall things would work just fine with POSIX quoting in the output.

Great news, thanks for working on this. pkgconf is quickly becoming the recommended pkgconfig implementation for me on Windows. :) It would be ideal if this implementation was somewhat compatible with the fdo implementation (it's a superset after all). Specifically, quoting spaces with `\` inside .pc files would keep working, and would output arguments quoted with `'`: ```ini prefix=/usr/with\ spaces libdir=${prefix}/lib ... Libs: -L${libdir} -lfoo ``` ```console $ pkgconf --libs foo-2.0 '-L/usr/with spaces' -lfoo ``` Meson (and other projects) have to output pkgconfig files like that for compatibility with the fdo pkg-config implementation. Eventually we can get rid of this and use POSIX quoting inside the .pc file. Meson already uses [`shlex.split()`](https://docs.python.org/3/library/shlex.html) for splitting the output, and Autotools uses shell, so overall things would work just fine with POSIX quoting in the output.

ariadne commented

2017-12-11 03:33:58 +00:00

That's the lexing side which is what #139 is about.

ariadne commented

2017-12-11 23:28:13 +00:00

Specifically an example of #139 deficiency:

> ./pkgconf --libs foo.pc
'-L/test/with\ spaces/lib' -lfoo

Specifically an example of #139 deficiency: ``` > ./pkgconf --libs foo.pc '-L/test/with\ spaces/lib' -lfoo ```

nh2 commented

2021-11-08 19:02:43 +00:00

(Migrated from github.com)

Is there user-facing documentation somewhere that says how I should generate my .pc files?

Something that describes (e.g. in the man page) how quoting should be done, such as making clear which of

Cflags: "-I/path"
Cflags: -I"/path"

should be used?

Is there user-facing documentation somewhere that says how I should generate my `.pc` files? Something that describes (e.g. in the `man` page) how quoting should be done, such as making clear which of ``` Cflags: "-I/path" Cflags: -I"/path" ``` should be used?

ariadne commented

2021-11-08 23:07:16 +00:00

No, that specific item hasn't been addressed, but patches are welcome.

Sign in to join this conversation.