Bug 93284 - Unqoting values of requested variables in pkg-config might break builds
Summary: Unqoting values of requested variables in pkg-config might break builds
Status: RESOLVED FIXED
Alias: None
Product: pkg-config
Classification: Unclassified
Component: src (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Dan Nicholson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-07 13:54 UTC by Marco Trevisan (Treviño)
Modified: 2016-02-26 17:03 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Revert "Unquote values of requested variables" (2.16 KB, patch)
2016-01-29 22:12 UTC, Dan Nicholson
Details | Splinter Review
Only unquote --variable when it appears quoted (3.27 KB, patch)
2016-01-29 22:12 UTC, Dan Nicholson
Details | Splinter Review
check: More thoroughly test variable usage (2.81 KB, patch)
2016-01-29 22:12 UTC, Dan Nicholson
Details | Splinter Review
Revert "Quote pc_path virtual variable" (1.20 KB, patch)
2016-01-29 22:12 UTC, Dan Nicholson
Details | Splinter Review

Description Marco Trevisan (Treviño) 2015-12-07 13:54:37 UTC
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.
Comment 1 Dan Nicholson 2016-01-29 22:12:06 UTC
Created attachment 121398 [details] [review]
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.
Comment 2 Dan Nicholson 2016-01-29 22:12:08 UTC
Created attachment 121399 [details] [review]
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.
Comment 3 Dan Nicholson 2016-01-29 22:12:10 UTC
Created attachment 121400 [details] [review]
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.
Comment 4 Dan Nicholson 2016-01-29 22:12:11 UTC
Created attachment 121401 [details] [review]
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.
Comment 5 Dan Nicholson 2016-01-29 22:13:23 UTC
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 ".
Comment 6 Dan Nicholson 2016-01-29 22:40:24 UTC
BTW, this issue originally came from bug67904. Adding Marek to CC to make sure this fix doesn't break his case.
Comment 7 Marek Kasik 2016-02-01 16:17:49 UTC
(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
Comment 8 Dan Nicholson 2016-02-01 17:33:10 UTC
Thanks, Marek. Marco, do you think you could test this or point me to the issue you're facing?
Comment 9 Marco Trevisan (Treviño) 2016-02-02 14:30:20 UTC
(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.
Comment 10 Dan Nicholson 2016-02-26 16:50:20 UTC
(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).
Comment 11 Dan Nicholson 2016-02-26 17:03:49 UTC
Applied in 5164b9dbabdca00fbd9d6bb962c4ac9b252448a2.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.