~kaniini/pkgconf#18: 
Cflags and Libs values escaping is broken on Windows

The recent 'fix missing backslashes in paths on Windows' commit (42b355310f) cancels escaping on Windows for all options specified in Cflags and Libs values and over sudden makes existing .pc files-generating programs and the already installed .pc files incompatible with the libpkgconf bugfix release 1.7.3.

In particular, even option values that have nothing to do with paths (-D, etc) cannot contain escaped characters anymore.

So, for example, the value

Cflags: -IC:\\Program\ Files\\Foo -DFOO=b\ ar

was previously parsed by the libpkgconf library into the 2 fragments (<type> <data>):

I C:\Program Files\Foo
D FOO=b ar

which are the perfectly valid Windows path and macro definition. Starting 1.7.3 this value is parsed into the 4 unusable fragments:

I C:\\Program\
  Files\\Foo
D FOO=b\
  ar

Note that trying to use the more realistic macro example -DFOO=\"b\ ar\" seems to somehow end up with no fragments at all on Windows now.

I actually wonder which issue the commit tries to address. Why the unparsed option values representing paths (-I, etc) must be valid Windows paths? The .pc file example in the commit message doesn't give any clues since it doesn't contain a single backslash. Was it an intention to make the paths usable "on Windows" as is, without unescaping? Note however, that making them usable in the Windows Command Prompt you make them unusable in the MSYS shell since it expects, for example, C:\\Windows path rather than C:\Windows. So if I'm right about intentions, the fix doesn't achieve the goal but breaks all around.

Isn't it better to revert the commit, keep the escaping rules uniform across all the platforms, and implement the desired functionality somehow differently. For example, inventing some option that tells pkgconf to print the option values in the form suitable for use in the specific command prompt (POSIX shell, Windows CMD, etc)? There can probably be some autodetection implemented.

Status
REPORTED
Submitter
~karen_arutyunov
Assigned to
No-one
Submitted
2 years ago
Updated
1 year, 4 months ago
Labels
No labels applied.

~vtorri 2 years ago

can you try this patch, please ? :

https://lists.sr.ht/~kaniini/pkgconf/%3CCAMq1ado72%3DqTQ%2BcNsO9jB3m8v0E%3DPYCiO6L5OBa14osLoN8m0g%40mail.gmail.com%3E

if you can join the IRC channel, i would like to discuss with you and see how to fix the bugs

thank you

~karen_arutyunov 2 years ago

The patch doesn't help. I would be surprised if it would, since the code parsing the Cflags value doesn't call the amended pkgconf_path_relocate() function.

Also I think it's valuable to have a record of the discussion in the public and so suggest to keep it in this thread.

To reiterate, my point is that the ability to backslash-escape characters like ' ', '"', etc in .pc files on Windows (same as on POSIX systems), canceled by the 'fix missing backslashes in paths on Windows' commit, should be restored by reverting this commit. It should be consern of the .pc file-generating programs to properly escape these characters (and the backslash itself), producing lines like

Cflags: -IC:\\Program\ Files\\Foo

rather than

Cflags: -IC:\Program Files\Foo

Alternatively, such programs may prefer value quoting.

For the library client after the pkgconf_pkg_find() and pkgconf_pkg_cflags() function calls the former line should be represented by the fragment list with a single fragment

I C:\Program Files\Foo

but not 2 unusable fragments

I C:\\Program\
  Files\\Foo

as it works now, after the mentioned commit.

It should also be consern of programs that parse .pc files using the libpkgconf library and then print the parsed data (for example pkgconf) to properly escape or quote it.

~vtorri 2 years ago

  1. this patch will replace \ with / in all the paths, this will work for both linux and windows (which also supports / as path separator). It works for me at least

  2. i told you that we can talk on IRC, which is more convenient

~kaniini 2 years ago

I'm not sure what the solution is here, because different people have different needs from pkgconf on Windows. Hopefully we can find some consensus on how to move forward.

~vtorri 2 years ago

i am wondering your usage of pc files on Windows. I would like to know it, to see what can be done.

My usage of pc files on Windows, and for most people in that case, it is to have flags passed to the preprocessor, compiler and linker, usually gcc, using MSYS2 terminal (there is also cross compilation, but that's another story). But one of the advice of gcc developpers on Windows is that path passed to compiler / linker must not have space. So using "Program Files" is not a good choice for installation of libraries and gcc itself. I have always followed that rule since 2007, without any problem since then.

~kaniini 2 years ago

AFAIK, they use libpkgconf directly, not the pkgconf CLI. The problem is that internally (at libpkgconf level), the fragments are no longer parsed correctly.

~karen_arutyunov 2 years ago

Yes, we only use libpkgconf library in our build2 toolchain project.

The build2 driver uses it to parse the installed .pc files during build2-based C/C++ project builds.

It also creates .pc files during C/C++ library installation. In particular, it escapes special characters (spaces, backslashes, etc) in the compiler and linker options. And yes, is is allowed for the users to use spaces in paths or macro values on any platform, if they wish to.

For more usage details you can see the libpkgconf-using source code.

I believe there are no regression tests for the libpkgconf library specifically and we've already been beaten several times due to backward incompatible changes related to quoting/escaping. Thus, while packaging the libpkgconf library for use in build2-based projects we have added some regression tests, which were blown up with the mentioned commit. You can probably guess the build2 expectations from the libpkgconf API, looking at these tests. Note that in these tests '$*' resolves to the test driver program (not pkgconf program). If something in the tests is still unclear, there is the testscript documentation available.

And as a general note, the quoting/escaping rules for the .pc file keyword and variable values needs to be documented by the pkgconf projects.

~vtorri 2 years ago

ok, i will try to fix this. I'll revert the patch in my local tree and see what to do, next week. I'll keep you in touch here

as I said, don't hesitate to go to IRC

regards

~vtorri 1 year, 11 months ago

in 42b355310f what is the problem : argvsplit.c modification, or fragment.c one ?

~karen_arutyunov 1 year, 11 months ago

The change in argvsplit.c affects us directly.

The change in fragment.c doesn't affect us, since build2 doesn't use libpkgconf rendering functions. However, I think that rolling back change in argvsplit.c but not in fragment.c would be wrong, because parsing and rendering should generally be symmetric. So, for example, the {parsing, rendering, parsing} sequence should end up with the same fragments as just parsing.

~vtorri 1 year, 11 months ago

without the moditications in argvsplit.c, i get this :

$ /opt/ewpi_64/bin/pkgconf.exe --cflags --libs eina -IE:Documentsprogrammes_x64msys2optefl_64/include/eina-1 -IE:Documentsprogrammes_x64msys2optefl_64/include/eina-1/eina -pthread -DWINICONV_CONST= -IE:Documentsprogrammes_x64msys2optewpi_64/include -LE:Documentsprogrammes_x64msys2optefl_64/lib -leina -pthread -lm -levil

that is, no path separator at all

eina.pc is this : prefix=E:/Documents/programmes_x64/msys2/opt/efl_64 libdir=${prefix}/lib includedir=${prefix}/include

Name: eina Description: efl: eina Version: 1.24.99 Requires.private: iconv Libs: -L${libdir} -leina -pthread -lm -levil Libs.private: -lpsapi -lole32 -lws2_32 -lsecur32 -luuid -lregex Cflags:-I${includedir}/eina-1 -I${includedir}/eina-1/eina -pthread

~vtorri 1 year, 11 months ago

well, i guess that you understand that eina.pc is formatted on a single line...

~vtorri 1 year, 11 months ago

prefix=E:/Documents/programmes_x64/msys2/opt/efl_64

libdir=${prefix}/lib

includedir=${prefix}/include

Name: eina

Description: efl: eina

Version: 1.24.99

Requires.private: iconv

Libs: -L${libdir} -leina -pthread -lm -levil

Libs.private: -lpsapi -lole32 -lws2_32 -lsecur32 -luuid -lregex

Cflags:-I${includedir}/eina-1 -I${includedir}/eina-1/eina -pthread

~karen_arutyunov 1 year, 4 months ago

Hi, do you have plans to fix this before 1.7.4 is released.

~kaniini 1 year, 4 months ago

1.7.4 was already released. We moved to using / as directory separator on Windows.

~karen_arutyunov 1 year, 4 months ago

Hm, don't see the version tag when clone from https://git.sr.ht/~kaniini/pkgconf but see it for https://github.com/pkgconf/pkgconf.git.

Is the latter is now the official repository (again)?

Also, does the change that you've mention in particular means that the libpkgconf library will parse the value

Cflags: -IC:\\Program\ Files\\Foo -DFOO=b\ ar

into the 2 fragments (<type> <data>) on Windows:

I C:\Program Files\Foo
D FOO=b ar

as it did before the 'fix missing backslashes in paths on Windows' commit (42b355310f) that has broken this behaviour, as explained in the issue description?

Register here or Log in to comment, or comment via email.