Building xorg-gtest fails with new pkg-config

I'm Marco, from the Ubuntu desktop team; and we're facing an issue with
recent pkg-config when using it in cmake.

As per commit 50c2867f4a6981e085c721d936c96f174f11f415 in pkg-config,
variables are unquoted.

This is fine for most of scenarios, but when there are variables such as

 CPPflags=-I${includedir} -I${includedir}/xorg -I${sourcedir} \
 -DDUMMY_CONF_PATH=\"${datarootdir}/xorg/gtest/dummy.conf\" \
 -DLOGFILE_DIR=\"/tmp\"

Then when using the new pkg-config "pkg-config --variable=CPPflags"
returns something like:

 -I/usr/include -I/usr/include/xorg -I/usr/src/xorg-gtest \
 -DDUMMY_CONF_PATH="/usr/share/xorg/gtest/dummy.conf" \
 -DLOGFILE_DIR="/tmp"

Which is wrong since the quotes aren't escaped.

Changing the variable so that it uses something like
  -DLOGFILE_DIR=\\\"/tmp\\\"

Makes it return the proper thing, but not in a subshell, but it doesn't
seem the case.

In fact pkg-config seems to return the proper thing when called in a
subshell, but not when used directly.

I.e.:

$ pkg-config --variable=CPPflags xorg-gtest
+ pkg-config --variable=CPPflags xorg-gtest
-I/usr/include -I/usr/include/xorg -I/usr/src/xorg-gtest
-DDUMMY_CONF_PATH="/usr/share/xorg/gtest/dummy.conf" -DLOGFILE_DIR="/tmp"

$ gcc-5 $(pkg-config --variable=CPPflags xorg-gtest) foo.c
++ pkg-config --variable=CPPflags xorg-gtest
+ gcc-5 -I/usr/include -I/usr/include/xorg -I/usr/src/xorg-gtest
'-DDUMMY_CONF_PATH="/usr/share/xorg/gtest/dummy.conf"'
'-DLOGFILE_DIR="/tmp"' foo.c

As you can see when called in a subshell, the variables are properly
single-quoted, making the thing work. But when calling this directly
there are no single quotes.

So I'm wondering what's the best way to fix this case.

Created attachment 121398
Revert "Unquote values of requested variables"

This reverts commit 50c2867f4a6981e085c721d936c96f174f11f415.
g_shell_quote doesn't match the quoting behavior of pkg-config when the
same variables are output via --cflags and similar.

Created attachment 121399
Only unquote --variable when it appears quoted

The change to unquote values in the --variable output broke users that
had shell special characters in the variable. Instead, only unquote if
the value starts with " or '. A larger fix to do a full unquote, split
and escaping like --cflags/--libs is possible, but that might break the
old semantics even further.

Add a new function, parse_package_variable(), to handle that logic.

Created attachment 121400
check: More thoroughly test variable usage

Add some more tests for handling unusual variables such as those that
are quoted or that contain shell characters. This should help make the
--variable output more reliable in the future.

Created attachment 121401
Revert "Quote pc_path virtual variable"

This reverts commit a6e8749ada5af1737b27f1eca1babe83e82af38c. With the
--variable output only being unquoted when it appears needed, this can
return to being a normally defined value.

Argh, sorry. I should have realized that unconditionally calling unquote would break things.

Can you try the attached patches? I changed it to only unquote if the value starts with ' or ".

BTW, this issue originally came from bug67904. Adding Marek to CC to make sure this fix doesn't break his case.

(In reply to Dan Nicholson from comment #6)
> BTW, this issue originally came from bug67904. Adding Marek to CC to make
> sure this fix doesn't break his case.

Hi,

I confirm that the changes also fixes the problem I was fixing by the patch. Btw, it was this one: https://bugzilla.redhat.com/show_bug.cgi?id=961855.

Regards

Thanks, Marek. Marco, do you think you could test this or point me to the issue you're facing?

(In reply to Dan Nicholson from comment #8)
> Thanks, Marek. Marco, do you think you could test this or point me to the
> issue you're facing?

This was the issue we were getting (explained on comment #0):
  - https://bugs.launchpad.net/compiz/+bug/1521366

We've currently "fixed" it by using a workaround at CMakeFile level, but I guess reverting the commit and adding a --unquoted-variable cmd line would be the best choice to do to avoid breakage.

(In reply to Marco Trevisan (Treviño) from comment #9)
>
> We've currently "fixed" it by using a workaround at CMakeFile level, but I
> guess reverting the commit and adding a --unquoted-variable cmd line would
> be the best choice to do to avoid breakage.

What I'd be doing here would be reverting so that --variable had the behavior it's had since it was introduced years ago except in the case that the value appears to be quoted.

For your downstream case, you could revert that change and depend on pkg-config-0.29.1 (or build-conflict with pkg-config-0.29). But I really think this is a regression and the proper fix is to change back to the original behavior as much as possible. Unfortunately, that makes this particular case more difficult, but people using such complex values in variables is almost certainly a rarity.

I'm going to apply this and release 0.29.1 with it. Please reopen if you need something else to handle this correctly (e.g., a special --unquoted-variable or such).

Applied in 5164b9dbabdca00fbd9d6bb962c4ac9b252448a2.