Building xorg-gtest fails with new pkg-config

Bug #1523508 reported by Marco Trevisan (Treviño)
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
pkg-config
Fix Released
Medium
pkg-config (Ubuntu)
Fix Released
High
Unassigned

Bug Description

As per the pkg-config change of unquoting variables, building xorg-gtest (in compiz for example) fails.

See more details in the linked upstream bug.

Revision history for this message
In , Marco Trevisan (Treviño) (3v1n0) wrote :

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.

tags: added: rls-x-incoming
Changed in pkg-config:
importance: Unknown → Medium
status: Unknown → Confirmed
Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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.

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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.

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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.

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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.

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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 ".

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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

Revision history for this message
In , Marek Kašík (mkasik) wrote :

(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

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

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

Revision history for this message
In , Marco Trevisan (Treviño) (3v1n0) wrote :

(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.

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

(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).

Revision history for this message
In , Dan Nicholson (danbnicholson) wrote :

Applied in 5164b9dbabdca00fbd9d6bb962c4ac9b252448a2.

Changed in pkg-config:
status: Confirmed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package pkg-config - 0.29.1-0ubuntu1

---------------
pkg-config (0.29.1-0ubuntu1) xenial; urgency=medium

  * Merge with Debian; remaining changes:
    - On Ubuntu, in pkg-config-crosswrapper add /usr/lib/pkgconfig to
    PKG_CONFIG_LIBDIR. As a lot of packages that are cross-build on
    regular basis rely on cross-building with non-multiarched libraries.
    - add dpkg-dev dependency, as dpkg-architecture binary is called from
    wrapper.
  * New upstream version 0.29.1.
    - Fix regression quoting variables. LP: #1523508.
  * Fix build failure with recent glib2.0.

pkg-config (0.29-3) unstable; urgency=medium

  * Store pkg-config architecture in /usr/lib/pkg-config.multiarch and
    pick that up in the crosswrapper. Closes: #807289, #807946

 -- Matthias Klose <email address hidden> Wed, 06 Apr 2016 15:22:03 +0200

Changed in pkg-config (Ubuntu):
status: Triaged → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.