Bug 75858 - use unexpanded paths in .pc files so pkg-config's special handling of ${prefix} on Windows can work
Summary: use unexpanded paths in .pc files so pkg-config's special handling of ${prefi...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks: 84966
  Show dependency treegraph
 
Reported: 2014-03-07 00:12 UTC by LRN
Modified: 2014-10-13 12:52 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Hack pkgconfig files to be more MinGW-friendly (3.12 KB, patch)
2014-03-07 00:13 UTC, LRN
Details | Splinter Review
Process and install pkg-config files for dbus (1.51 KB, patch)
2014-03-07 00:14 UTC, LRN
Details | Splinter Review
Hack pkgconfig files to be more MinGW-friendly (v2, no autotools) (2.07 KB, patch)
2014-03-08 07:36 UTC, LRN
Details | Splinter Review
Process and install pkg-config files for dbus (v2) (1.50 KB, patch)
2014-03-08 07:36 UTC, LRN
Details | Splinter Review
Hack pkgconfig files to be more MinGW-friendly (v2, autotools part) (1.04 KB, patch)
2014-03-08 09:11 UTC, LRN
Details | Splinter Review
Hack pkgconfig files to be more MinGW-friendly (v3, no autotools) (2.12 KB, patch)
2014-03-17 15:11 UTC, LRN
Details | Splinter Review
Make sure bindir is in .pc file and daemondir there is unexpanded (1.19 KB, patch)
2014-04-28 17:12 UTC, LRN
Details | Splinter Review
Hack pkgconfig files to be more MinGW-friendly (v4, combined) (2.80 KB, patch)
2014-04-30 17:52 UTC, LRN
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description LRN 2014-03-07 00:12:14 UTC
DBus pkg-config files explicitly use expanded versions of various paths in .pc
files. I assume this is necessary on *nix.

But on Windows it leads to poorly-compatible .pc files being created.
Observe this:
sysconfdir=/mingw/etc
This is expanded, and it goes into .pc file this way.
Now, if you run pkg-config as part of building something in MSYS, everything's
fine, /mingw/etc is probably the right path to have.
But if you run it as part of building something with, say, CMake, or anything
that is not MSYS-related, suddenly /mingw/etc stops making any sense
whatsoever.

This is why pkg-config historically had variable expansion. You put
sysconfdir=${prefix}/etc
into a .pc file, pkg-config will figure out what ${prefix} is at runtime, and
it will spew correct W32 path whenever you ask for sysconfdir.

When the same .pc file is used in MSYS, you usually just pass the
--dont-define-prefix
option to it, which will make it use prefix=... from the .pc file itself, and
that prefix is the right one for MSYS (if, of course, you build packages
correctly, with --prefix=/mingw).

The patches that i have are hacks, not committable as-is. Just using them
to start a discussion.
Comment 1 LRN 2014-03-07 00:13:30 UTC
Created attachment 95264 [details] [review]
Hack pkgconfig files to be more MinGW-friendly
Comment 2 LRN 2014-03-07 00:14:11 UTC
Created attachment 95265 [details] [review]
Process and install pkg-config files for dbus

This is tied to the other patch, the one that makes pkg-config files
MinGW-friendly.
Comment 3 Simon McVittie 2014-03-07 12:37:44 UTC
Comment on attachment 95264 [details] [review]
Hack pkgconfig files to be more MinGW-friendly

Review of attachment 95264 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +1451,3 @@
>  #### find the actual value for $prefix that we'll end up with
>  ##   (I know this is broken and should be done in the Makefile, but
>  ##    that's a major pain and almost nobody actually seems to care)

You are running into precisely the brokenness described by this comment. Your use-case is why it is considered broken.

@@ +1453,5 @@
>  ##    that's a major pain and almost nobody actually seems to care)
> +UNEXPANDED_PREFIX="$prefix"
> +UNEXPANDED_LOCALSTATEDIR="$localstatedir"
> +UNEXPANDED_SYSCONFDIR="$sysconfdir"
> +UNEXPANDED_BINDIR="$bindir"

That's a sh(1) variable reference. What you've done here is not "unexpanded", it's "expanded, but not recursively" - for instance, in a typical Unix build with default settings, that's equivalent to

    UNEXPANDED_BINDIR='${exec_prefix}/bin'

At best that's misleading naming.

prefix, localstatedir, sysconfdir, bindir etc. are all AC_SUBST'd anyway, so you should just be able to use

prefix=@prefix@
exec_prefix=@exec_prefix@
bindir=@bindir@

and so on.

::: dbus-1.pc.in
@@ +3,4 @@
>  libdir=@libdir@
>  includedir=@includedir@
>  system_bus_default_address=@DBUS_SYSTEM_BUS_DEFAULT_ADDRESS@
> +sysconfdir=@sysconfdir@

s/@EXPANDED_SYSCONFDIR@/@sysconfdir@/ is an improvement, I'd take that patch (assuming the CMake stuff either substitutes @sysconfdir@, or can be made to do so).

@@ +3,5 @@
>  libdir=@libdir@
>  includedir=@includedir@
>  system_bus_default_address=@DBUS_SYSTEM_BUS_DEFAULT_ADDRESS@
> +sysconfdir=@sysconfdir@
> +session_bus_services_dir=@UNEXPANDED_DATADIR@/dbus-1/services

The right way to do this is something like:

datarootdir=@datarootdir@
datadir=@datadir@
session_bus_services_dir=${datadir}/dbus-1-services

or whatever the hierarchy of expansions is for the default ${datadir}.

@@ +6,5 @@
> +sysconfdir=@sysconfdir@
> +session_bus_services_dir=@UNEXPANDED_DATADIR@/dbus-1/services
> +system_bus_services_dir=@UNEXPANDED_DATADIR@/dbus-1/system-services
> +interfaces_dir=@UNEXPANDED_DATADIR@/dbus-1/interfaces
> +daemondir=${exec_prefix}/bin

No, daemondir really does need to be its own variable - otherwise, this will break the functionality of --with-dbus-daemondir in configure.ac.

You could have a variable that is unexpanded and defaults to '${bindir}' - I would suggest calling it dbus_daemondir - and a separate expanded version from

    AS_AC_EXPAND([DBUS_DAEMONDIR], ["$dbus_daemondir"])

for hard-coding into binaries and bus/org.freedesktop.dbus-session.plist (only used on Unix and Mac OS, respectively).

The fact that we manually expand *anything* is arguably a bug, but it's one we're stuck with for backwards compatibility in D-Bus-as-an-OS-feature, which is its most important use case.
Comment 4 Simon McVittie 2014-03-07 12:38:32 UTC
Comment on attachment 95265 [details] [review]
Process and install pkg-config files for dbus

Review of attachment 95265 [details] [review]:
-----------------------------------------------------------------

::: cmake/CMakeLists.txt
@@ +75,5 @@
>  set(DBUS_DAEMONDIR           ${EXPANDED_BINDIR})
> +set(libdir \${prefix}/lib)
> +set(includedir \${prefix}/include)
> +set(sysconfdir \${prefix}/etc)
> +set(UNEXPANDED_DATADIR \${prefix}/share)

set(datadir ...), set(datarootdir ...)
Comment 5 Simon McVittie 2014-03-07 12:41:32 UTC
(In reply to comment #0)
> DBus pkg-config files explicitly use expanded versions of various paths in
> .pc
> files. I assume this is necessary on *nix.

It isn't, if you substitute all the layers of variables like

    prefix=@prefix@
    datarootdir=@datarootdir@
    datadir=@datadir@
    session_bus_services_dir=${datadir}/dbus-1-services

which typically expands to

    prefix=/usr
    datarootdir=${prefix}/share
    datadir=${datarootdir}
    session_bus_services_dir=${datadir}/dbus-1-services

which pkg-config evaluates at runtime to

    session_bus_services_dir=/usr/share/dbus-1-services

as desired (but if you used pkg-config --define-variable=prefix=opt it would expand it to /opt/share/dbus-1-services, and so on).
Comment 6 LRN 2014-03-08 07:36:03 UTC
Created attachment 95341 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v2, no autotools)

Doesn't include configure.ac changes yet
Comment 7 LRN 2014-03-08 07:36:33 UTC
Created attachment 95342 [details] [review]
Process and install pkg-config files for dbus (v2)
Comment 8 LRN 2014-03-08 09:11:58 UTC
Created attachment 95349 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v2, autotools part)

Works as intended, if i configure with:
--prefix=/mingw --with-dbus-daemondir=\${prefix}/bin --datarootdir=\${prefix}/share --docdir=\${prefix}/\${docdir} --sbindir=\${prefix}/sbin --libexecdir=\${prefix}/libexec --sysconfdir=\${prefix}/etc --localstatedir=\${prefix}/var

CMake version works as intended, if configured with:
    -DCMAKE_INSTALL_PREFIX=/${prefix} \
    -DCMAKE_SYSTEM_PREFIX_PATH=${prefix} \
    -Ddbus_daemondir=\${prefix}/bin
Comment 9 LRN 2014-03-13 12:14:44 UTC
Totally forgot to add datarootdir=@datarootdir@ to .pc files
Comment 10 LRN 2014-03-17 15:11:42 UTC
Created attachment 95944 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v3, no autotools)

Fixed version of the .pc changes, with datarootdir added. Other two patches are unaffected.
Comment 11 Simon McVittie 2014-03-17 17:45:09 UTC
Comment on attachment 95342 [details] [review]
Process and install pkg-config files for dbus (v2)

Review of attachment 95342 [details] [review]:
-----------------------------------------------------------------

::: cmake/CMakeLists.txt
@@ +82,5 @@
> +endif()
> +set(libdir \${prefix}/lib)
> +set(includedir \${prefix}/include)
> +set(sysconfdir \${prefix}/etc)
> +set(datadir \${prefix}/share)

There should probably be a datarootdir here too, for completeness?
Comment 12 Simon McVittie 2014-03-17 17:49:27 UTC
Comment on attachment 95349 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v2, autotools part)

Review of attachment 95349 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +1588,3 @@
>  else
>      DBUS_DAEMONDIR=$with_dbus_daemondir
> +    dbus_daemondir=$with_dbus_daemondir

This still performs one level of expansion. Is that really what you want?

I would have expected that you'd want

if test -z "$with_dbus_daemondir"; then
    DBUS_DAEMONDIR=$EXPANDED_BINDIR
    dbus_daemondir='$bindir'
else
    DBUS_DAEMONDIR=$with_dbus_daemondir
    dbus_daemondir=$with_dbus_daemondir
fi

so that in a normal Unix installation, dbus-1.pc would say

prefix=/usr
exec_prefix=${prefix}
dbus_daemondir=${exec_prefix}/bin

and on msys it would be something like

prefix=/c/mingw
exec_prefix=${prefix}
dbus_daemondir=${exec_prefix}/bin

(With your patch it would be ${prefix}/bin - one level of expansion applied to the default value of ${bindir} - but the most correct thing would be to have zero levels of expansion.)
Comment 13 Simon McVittie 2014-03-17 17:52:48 UTC
(In reply to comment #12)
> I would have expected that you'd want
> 
> if test -z "$with_dbus_daemondir"; then
>     DBUS_DAEMONDIR=$EXPANDED_BINDIR
>     dbus_daemondir='$bindir'
> else
>     DBUS_DAEMONDIR=$with_dbus_daemondir
>     dbus_daemondir=$with_dbus_daemondir
> fi

Er, that should have said '${bindir}' not '$bindir'.

To be consistent with the name of the variable in the .pc file, maybe daemondir would be a better name for the unexpanded version.

I think this would be fine as one big commit - I don't see any reason why it would make sense to apply some, but not all, of it.


> so that in a normal Unix installation, dbus-1.pc would say
> 
> prefix=/usr
> exec_prefix=${prefix}
> dbus_daemondir=${exec_prefix}/bin

That should have said:

prefix=/usr
exec_prefix=${prefix}
bindir=${exec_prefix}/bin
daemondir=${bindir}

Getting that result might mean you need to add bindir=@bindir@ to the .pc files.
Comment 14 Simon McVittie 2014-03-17 18:01:07 UTC
(In reply to comment #8)
> Works as intended, if i configure with:
> --prefix=/mingw --with-dbus-daemondir=\${prefix}/bin
> --datarootdir=\${prefix}/share --docdir=\${prefix}/\${docdir}
> --sbindir=\${prefix}/sbin --libexecdir=\${prefix}/libexec
> --sysconfdir=\${prefix}/etc --localstatedir=\${prefix}/var

You shouldn't have to do all that. If you've got it right, "--prefix=/mingw" should be enough.

> --docdir=\${prefix}/\${docdir}

So docdir='${prefix}/${docdir}', with infinite recursion when evaluated? That's not going to work very well.
Comment 15 LRN 2014-03-17 21:37:41 UTC
(In reply to comment #11)
> ::: cmake/CMakeLists.txt
> @@ +82,5 @@
> > +endif()
> > +set(libdir \${prefix}/lib)
> > +set(includedir \${prefix}/include)
> > +set(sysconfdir \${prefix}/etc)
> > +set(datadir \${prefix}/share)
> 
> There should probably be a datarootdir here too, for completeness?

Yes, but it's kind of optional. Datarootdir is needed there for autotools first and foremost, because datadir=${datarootdir} in that case. In cmake datadir is explicitly set as ${prefix}/share, so datarootdir is not really needed. But it's OK to have it too.(In reply to comment #12)

> Review of attachment 95349 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.ac
> @@ +1588,3 @@
> >  else
> >      DBUS_DAEMONDIR=$with_dbus_daemondir
> > +    dbus_daemondir=$with_dbus_daemondir
> 
> This still performs one level of expansion. Is that really what you want?
> 
> I would have expected that you'd want
> 
> if test -z "$with_dbus_daemondir"; then
>     DBUS_DAEMONDIR=$EXPANDED_BINDIR
>     dbus_daemondir='$bindir'
> else
>     DBUS_DAEMONDIR=$with_dbus_daemondir
>     dbus_daemondir=$with_dbus_daemondir
> fi
> 
> so that in a normal Unix installation, dbus-1.pc would say
> 
> prefix=/usr
> exec_prefix=${prefix}
> dbus_daemondir=${exec_prefix}/bin
> 
> and on msys it would be something like
> 
> prefix=/c/mingw
> exec_prefix=${prefix}
> dbus_daemondir=${exec_prefix}/bin
> 
> (With your patch it would be ${prefix}/bin - one level of expansion applied
> to the default value of ${bindir} - but the most correct thing would be to
> have zero levels of expansion.)

I get dbus_daemondir=${exec_prefix}/bin with my patch, when building with cmake. What am i doing wrong?
Shell expansion always baffled me, to be honest.
Comment 16 LRN 2014-03-17 21:42:07 UTC
(In reply to comment #14)
> (In reply to comment #8)
> > Works as intended, if i configure with:
> > --prefix=/mingw --with-dbus-daemondir=\${prefix}/bin
> > --datarootdir=\${prefix}/share --docdir=\${prefix}/\${docdir}
> > --sbindir=\${prefix}/sbin --libexecdir=\${prefix}/libexec
> > --sysconfdir=\${prefix}/etc --localstatedir=\${prefix}/var
> 
> You shouldn't have to do all that. If you've got it right, "--prefix=/mingw"
> should be enough.

I like to not to rely on autotools' defaults, they are wrong sometimes (i build lots of things, this (minus the --with-dbus-daemondir) is basically a template i use everywhere).
> 
> > --docdir=\${prefix}/\${docdir}
> 
> So docdir='${prefix}/${docdir}', with infinite recursion when evaluated?
> That's not going to work very well.

Ah, that. No, i guess i wasn't making things clear enough.
This is what i do:

prefix=/mingw
docdir=share/doc/${name}/${ver}

...

if test "x${install_dirs}" == "x"; then install_dirs="--datarootdir=\\\${prefix}/share --docdir=\\\${prefix}/\${docdir} --sbindir=\\\${prefix}/sbin --libexecdir=\\\${prefix}/libexec --sysconfdir=\\\${prefix}/etc --localstatedir=\\\${prefix}/var"; fi

...

eval "install_dirs=\"$install_dirs\""

...

  ./configure \
    --prefix=${prefix} \
    --enable-silent-rules \
    --enable-verbose-mode \
    --enable-checks \
    --enable-asserts \
    --enable-xml-docs \
    --enable-embedded-tests \
    --enable-modular-tests \
    --enable-installed-tests \
    --with-dbus-daemondir=\${prefix}/bin \
    ${install_dirs} \
...
Comment 17 Simon McVittie 2014-04-28 15:57:52 UTC
(In reply to comment #16)
> This is what i do:
> 
> prefix=/mingw
> docdir=share/doc/${name}/${ver}

I recommend using a less confusing name for your intermediate variable :-)

> if test "x${install_dirs}" == "x"; then
> install_dirs="--datarootdir=\\\${prefix}/share
> --docdir=\\\${prefix}/\${docdir} --sbindir=\\\${prefix}/sbin
> --libexecdir=\\\${prefix}/libexec --sysconfdir=\\\${prefix}/etc
> --localstatedir=\\\${prefix}/var"; fi
> ...
> eval "install_dirs=\"$install_dirs\""

If you find yourself using eval, it's usually a sign that something has gone wrong.

(In reply to comment #15)
> > (With your patch it would be ${prefix}/bin - one level of expansion applied
> > to the default value of ${bindir} - but the most correct thing would be to
> > have zero levels of expansion.)
> 
> I get dbus_daemondir=${exec_prefix}/bin with my patch, when building with
> cmake. What am i doing wrong?
> Shell expansion always baffled me, to be honest.

More specifically, with no special configure command-line options, the right thing would be for the resulting .pc file to contain a line «dbus_daemondir=${exec_prefix}/bin» and a line «exec_prefix=${prefix}» and a line «prefix=/usr/local», so that pkg-config can do all the levels of expansion.

(Deliberately using non-standard quotes to avoid confusion with sh(1) syntax :-)

If that's the case already with your patchset, then I'll need to look at what's going on in configure and why it works correctly when I expected that it wouldn't.

(In reply to comment #16)
> (In reply to comment #14)
> > You shouldn't have to do all that. If you've got it right, "--prefix=/mingw"
> > should be enough.
> 
> I like to not to rely on autotools' defaults, they are wrong sometimes (i
> build lots of things, this (minus the --with-dbus-daemondir) is basically a
> template i use everywhere).

That's fine for your personal meta-build-system, but by making all these command-line options explicit, you're hiding bugs in configure scripts. When you're testing patches to be submitted, please also test them with that simpler situation (no command-line options, or --prefix=/mingw and nothing else) and check that they are working as intended, and you aren't ignoring (or creating!) configure-script bugs.

dbus needs to work in multiple configurations, not just your personal configuration - indeed, the most important configuration for it to work in is «--prefix=/usr --sysconfdir=/etc --localstatedir=/var», because that's what Linux distributions will be using.
Comment 18 Simon McVittie 2014-04-28 16:00:12 UTC
(In reply to comment #17)
> More specifically, with no special configure command-line options, the right
> thing would be for the resulting .pc file to contain a line
> «dbus_daemondir=${exec_prefix}/bin» and a line «exec_prefix=${prefix}» and a
> line «prefix=/usr/local», so that pkg-config can do all the levels of
> expansion.

Ugh, now I'm confusing myself. It should ideally have «dbus_daemondir=${bindir}» and «bindir=${exec_prefix}/bin» and «exec_prefix=${prefix}» and «prefix=/usr/local» - in other words, pkg-config should be doing *all* of the levels of expansion.
Comment 19 LRN 2014-04-28 17:12:56 UTC
Created attachment 98131 [details] [review]
Make sure bindir is in .pc file and daemondir there is unexpanded

With attachment 95349 [details] [review] and attachment 95944 [details] [review] and with invoking configure like this:
../dbus-1.8.0/configure --prefix=/mingw --enable-silent-rules --enable-verbose-mode --enable-checks --enable-asserts --enable-xml-docs --enable-embedded-tests --enable-modular-tests --enable-installed-tests --build=i686-w64-mingw32 STRIP=true CFLAGS="-g -O2 -D__USE_MINGW_ANSI_STDIO=1"

i get this in the .pc file:

> prefix=/mingw
> exec_prefix=${prefix}
> libdir=${exec_prefix}/lib
> includedir=${prefix}/include
> system_bus_default_address=unix:path=/mingw/var/run/dbus/system_bus_socket
> datarootdir=${prefix}/share
> datadir=${datarootdir}
> sysconfdir=${prefix}/etc
> session_bus_services_dir=${datadir}/dbus-1/services
> system_bus_services_dir=${datadir}/dbus-1/system-services
> interfaces_dir=${datadir}/dbus-1/interfaces
> daemondir=/mingw/bin

and documentation goes into /mingw/share/doc/dbus.

If i add --sysconfdir=/etc --localstatedir=/var, i get this in the .pc file:

> prefix=/mingw
> exec_prefix=${prefix}
> libdir=${exec_prefix}/lib
> includedir=${prefix}/include
> system_bus_default_address=unix:path=/var/run/dbus/system_bus_socket
> datarootdir=${prefix}/share
> datadir=${datarootdir}
> sysconfdir=/etc
> session_bus_services_dir=${datadir}/dbus-1/services
> system_bus_services_dir=${datadir}/dbus-1/system-services
> interfaces_dir=${datadir}/dbus-1/interfaces
> daemondir=/mingw/bin


with obvious consequences (conf files in <instdir>/etc instead of <instdir><prefix>/etc , same with /var).

Without --prefix (but with --sysconfdir=/etc --localstatedir=/var) i get this:

> prefix=/usr/local
> exec_prefix=${prefix}
> libdir=${exec_prefix}/lib
> includedir=${prefix}/include
> system_bus_default_address=unix:path=/var/run/dbus/system_bus_socket
> datarootdir=${prefix}/share
> datadir=${datarootdir}
> sysconfdir=/etc
> session_bus_services_dir=${datadir}/dbus-1/services
> system_bus_services_dir=${datadir}/dbus-1/system-services
> interfaces_dir=${datadir}/dbus-1/interfaces
> daemondir=/usr/local/bin

and some things get installed into <instdir>/usr/local

> More specifically, with no special configure command-line options, the right thing would be for the resulting .pc file to contain a line «dbus_daemondir=${exec_prefix}/bin» and a line «exec_prefix=${prefix}» and a line «prefix=/usr/local», so that pkg-config can do all the levels of expansion.
> Ugh, now I'm confusing myself. It should ideally have «dbus_daemondir=${bindir}» and «bindir=${exec_prefix}/bin» and «exec_prefix=${prefix}» and «prefix=/usr/local» - in other words, pkg-config should be doing *all* of the levels of expansion.

[ ] daemondir (by the way, is it "daemondir" or "dbus_daemondir"?)
[ ] bindir
[x] exec_prefix
[x] prefix

So, we need to:
add bindir=${exec_prefix}/bin (config.status says «S["bindir"]="${exec_prefix}/bin"», so just setting it to $bindir in configure will probably work)
fix daemondir to not to be expanded (i.e. use $bindir instead of $EXPANDED_BINDIR)

With the extra patch that i'm attaching, i get (configured with --prefix=/mingw and --enable..., just as the first config presented in this comment):

> prefix=/mingw
> exec_prefix=${prefix}
> bindir=${exec_prefix}/bin
> libdir=${exec_prefix}/lib
> includedir=${prefix}/include
> system_bus_default_address=unix:path=/mingw/var/run/dbus/system_bus_socket
> datarootdir=${prefix}/share
> datadir=${datarootdir}
> sysconfdir=${prefix}/etc
> session_bus_services_dir=${datadir}/dbus-1/services
> system_bus_services_dir=${datadir}/dbus-1/system-services
> interfaces_dir=${datadir}/dbus-1/interfaces
> daemondir=${exec_prefix}/bin

Which should satisfy your conditions.
Comment 20 Simon McVittie 2014-04-30 17:35:49 UTC
Comment on attachment 98131 [details] [review]
Make sure bindir is in .pc file and daemondir there is unexpanded

Review of attachment 98131 [details] [review]:
-----------------------------------------------------------------

> With the extra patch that i'm attaching, i get (configured with --prefix=/mingw and --enable..., just as the first config presented in this comment):
> 
> > prefix=/mingw
> > exec_prefix=${prefix}
> > bindir=${exec_prefix}/bin
> > libdir=${exec_prefix}/lib
> > includedir=${prefix}/include
> > system_bus_default_address=unix:path=/mingw/var/run/dbus/system_bus_socket
> > datarootdir=${prefix}/share
> > datadir=${datarootdir}
> > sysconfdir=${prefix}/etc
> > session_bus_services_dir=${datadir}/dbus-1/services
> > system_bus_services_dir=${datadir}/dbus-1/system-services
> > interfaces_dir=${datadir}/dbus-1/interfaces
> > daemondir=${exec_prefix}/bin
>
> Which should satisfy your conditions.

This all looks good except that daemondir has been expanded once: its value is a copy of the value of ${bindir}, not the unexpanded string «${bindir}».

Would you mind combining all the patches you want applied for this bug into one, or perhaps two if you want to keep Attachment #95342 [details] separate? I don't think the intermediate states with some, but not all, of your three most recent patches make sense; it'll be clearer to just get it right in one go.

::: configure.ac
@@ +1584,4 @@
>  #### Directory to install dbus-daemon
>  if test -z "$with_dbus_daemondir" ; then
>      DBUS_DAEMONDIR=$EXPANDED_BINDIR
> +    dbus_daemondir=$bindir

I think this should probably be

    dbus_daemondir='${bindir}'

with the quotes.

The fact that configure is a shell script means that you're applying one level of shell expansion by leaving it unquoted: after executing this line, the value of the dbus_daemondir variable will be something like «${exec_prefix}/bin», whereas for the maximum amount of scope for redefining things when invoking pkg-config, it would be better for it to be «${bindir}».
Comment 21 LRN 2014-04-30 17:52:02 UTC
Created attachment 98256 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v4, combined)

I haven't touched cmake in a while, and i'm currently not very interested in doing so. To that end i'd rather see this to be committed right now, as-is, without checking that cmake (with or without the attachment 95342 [details] [review]) counterpart works, and *then* someone can see to making cmake compatible with the new .pc.in files.
Comment 22 Simon McVittie 2014-10-13 12:14:45 UTC
Comment on attachment 98256 [details] [review]
Hack pkgconfig files to be more MinGW-friendly (v4, combined)

Review of attachment 98256 [details] [review]:
-----------------------------------------------------------------

::: dbus-1-uninstalled.pc.in
@@ +6,2 @@
>  system_bus_default_address=@DBUS_SYSTEM_BUS_DEFAULT_ADDRESS@
> +datadir=@datadir@

The default is datadir='${datarootdir}', so as far as I can see, you will need

datarootdir=@datarootdir@

before this line to make it work for anyone not setting an explicit datadir.

::: dbus-1.pc.in
@@ +4,4 @@
>  libdir=@libdir@
>  includedir=@includedir@
>  system_bus_default_address=@DBUS_SYSTEM_BUS_DEFAULT_ADDRESS@
> +datadir=@datadir@

Likewise
Comment 23 Simon McVittie 2014-10-13 12:23:03 UTC
(In reply to LRN from comment #21)
> To that end i'd rather see this to be committed right now, as-is,
> without checking that cmake (with or without the attachment 95342 [details] [review])
> counterpart works, and *then* someone can see to making cmake
> compatible with the new .pc.in files.

Sorry, no, I am not willing to commit this with known regressions. The cmake side of things can be pretty simple - only the things that now work need to still work, and in particular it doesn't necessarily need to leave strings unexpanded like you're doing with the Autotools side - but it does need to exist.

As far as I can see, that means, at a minimum,

set(bindir ${EXPANDED_BINDIR})
set(datadir ${EXPANDED_DATADIR})
set(sysconfdir ${EXPANDED_SYSCONFDIR})
set(dbus_daemondir ${DBUS_DAEMONDIR})
Comment 24 Simon McVittie 2014-10-13 12:28:05 UTC
(In reply to Simon McVittie from comment #22)
> The default is datadir='${datarootdir}', so as far as I can see, you will
> need
> 
> datarootdir=@datarootdir@
> 
> before this line to make it work for anyone not setting an explicit datadir.

Actually, this would work, for now, but only because configure contains explicit compatibility code:

# If the template does not know about datarootdir, expand it.
# FIXME: This hack should be removed a few years after 2.60.

This doesn't seem like something that will continue to work forever, and provokes warnings:

config.status: creating dbus-1.pc
config.status: WARNING:  '/home/smcv/src/fdo/dbus/dbus-1.pc.in' seems to ignore the --datarootdir setting
config.status: creating dbus-1-uninstalled.pc
config.status: WARNING:  '/home/smcv/src/fdo/dbus/dbus-1-uninstalled.pc.in' seems to ignore the --datarootdir setting
Comment 25 Simon McVittie 2014-10-13 12:41:14 UTC
(In reply to Simon McVittie from comment #23)
> The cmake
> side of things can be pretty simple - only the things that now work need to
> still work, and in particular it doesn't necessarily need to leave strings
> unexpanded like you're doing with the Autotools side - but it does need to
> exist.

Actually, no, I take that back. The CMake build system doesn't currently install .pc files at all, which is a bug, but means it isn't a regression if the CMake build system doesn't substitute everything that needs to be substituted.
Comment 26 Simon McVittie 2014-10-13 12:50:05 UTC
(In reply to Simon McVittie from comment #25)
> The CMake build system doesn't currently
> install .pc files at all, which is a bug

Bug #84966
Comment 27 Simon McVittie 2014-10-13 12:52:24 UTC
(In reply to Simon McVittie from comment #22)
> you will need
> 
> datarootdir=@datarootdir@

Fixed that and committed the result. Fixed in git for 1.9.2, thanks!


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.