Re: [PATCH BlueZ] configure.ac: call AC_SUBST unconditionally

[Marcel Holtmann <marcel@holtmann.org>]

Hi Antonio,

> > > Call AC_SUBST unconditionally, otherwise options like
> > > --with-dbusconfdir=DIR or --with-udevdir=DIR have no effect.
> > > 
> > > Before this change, configuring with:
> > > 
> > >   $ mkdir build
> > >   $ ./configure --disable-systemd \
> > >                 --prefix=$(pwd)/build \
> > >                 --with-dbusconfdir=$(pwd)/build/etc
> > > 
> > > resulted in the option value to be ignored at "make install" time, with
> > > this error:
> > > 
> > >   /bin/mkdir: cannot create directory '/dbus-1/system.d': Permission denied
> > > 
> > > After the patch the option value is respected.
> > > ---
> > > 
> > > Hi,
> > > 
> > > the issue was highlighted by the use "--prefix=" and running "make install" as
> > > a restricted user, maybe the are still other issues with this use case.
> > > Anyone willing to take a deeper look?
> > 
> > why are you doing --prefix="" in the first place? I do not get that
> > part.
> 
> Sorry, poor communication choice from my part, in the sentence above the
> _whole_ --prefix= was enclosed in double quotes to mean "the --prefix
> parameter", but I see this could be misleading, I am actually using:
> 
>   --prefix=$(pwd)/build
> 
> > > For instance, is "--prefix=DIR" supposed to be prepended to manually specified
> > > paths too?
> > >
> > > Thanks,
> > >    Antonio
> > > 
> > >  configure.ac |   16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index 070acea..fe2893a 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -71,8 +71,8 @@ if (test -z "${path_dbusconfdir}"); then
> > >  		AC_MSG_ERROR([D-Bus configuration directory is required])
> > >  	fi
> > >  	AC_MSG_RESULT([${path_dbusconfdir}])
> > > -	AC_SUBST(DBUS_CONFDIR, [${path_dbusconfdir}])
> > >  fi
> > > +AC_SUBST(DBUS_CONFDIR, [${path_dbusconfdir}])
> > 
> > I am failing to see the bug here. you are providing the
> > --with-dbusconfdir=DIR and thus is should work. What is causing the
> > wrong mkdir actually.
> >
> 
> This is what I understand is going on in configure.ac right now:
> 
>   # define the option
>   AC_ARG_WITH([dbusconfdir] ... [path_dbusconfdir=${withval}])
> 
>   # when --with-dbusconfdir is NOT used
>   if (test -z "${path_dbusconfdir}"); then
>     ...
> 
>     # define the config dir
>     path_dbusconfdir="`$PKG_CONFIG --variable=sysconfdir dbus-1`"
>     
>     ...
>     
>     # set DBUS_CONFDIR
>     AC_SUBST(DBUS_CONFDIR, [${path_dbusconfdir}])
>   endif
> 
> when --with-dbusconfdir=SOMEDIR is used the test above fails, and the
> result is that ${path_dbusconfdir} is indeed defined, but DBUS_CONFDIR
> is not, and the latter is going to be used in Makefile.am:
> 
>   dbusdir = @DBUS_CONFDIR@/dbus-1/system.d
> 
> The wrong makedir is exposed by my use of the "prefix" option and the
> fact that I am running "make install" as a normal user, otherwise
> /dbus-1/system.d would be happily (and wrongly) created.
> 
> By always setting DBUS_CONFDIR we cover the case when
> --with-dbusconfdir=SOMEDIR is used.

I see your point here. This one is valid. Please send separate patches
for that.

> 
> > >  AC_ARG_WITH([dbussystembusdir], AC_HELP_STRING([--with-dbussystembusdir=DIR],
> > >  				[path to D-Bus system bus services directory]),
> > > @@ -84,8 +84,8 @@ if (test -z "${path_dbussystembusdir}"); then
> > >  		AC_MSG_ERROR([D-Bus system bus services directory is required])
> > >  	fi
> > >  	AC_MSG_RESULT([${path_dbussystembusdir}])
> > > -	AC_SUBST(DBUS_SYSTEMBUSDIR, [${path_dbussystembusdir}])
> > >  fi
> > > +AC_SUBST(DBUS_SYSTEMBUSDIR, [${path_dbussystembusdir}])
> > >  
> > >  AC_ARG_WITH([dbussessionbusdir], AC_HELP_STRING([--with-dbussessionbusdir=DIR],
> > >  				[path to D-Bus session bus services directory]),
> > > @@ -97,8 +97,8 @@ if (test -z "${path_dbussessionbusdir}"); then
> > >  		AC_MSG_ERROR([D-Bus session bus services directory is required])
> > >  	fi
> > >  	AC_MSG_RESULT([${path_dbussessionbusdir}])
> > > -	AC_SUBST(DBUS_SESSIONBUSDIR, [${path_dbussessionbusdir}])
> > >  fi
> > > +AC_SUBST(DBUS_SESSIONBUSDIR, [${path_dbussessionbusdir}])
> > >  
> > >  AC_ARG_ENABLE(library, AC_HELP_STRING([--enable-library],
> > >  		[install Bluetooth library]), [enable_library=${enableval}])
> > > @@ -121,8 +121,6 @@ AC_ARG_ENABLE(usb, AC_HELP_STRING([--disable-usb],
> > >  if (test "${enable_tools}" != "no" && test "${enable_usb}" != "no"  ); then
> > >  	PKG_CHECK_MODULES(USB, libusb, dummy=yes,
> > >  			AC_MSG_ERROR(USB library support is required))
> > > -	AC_SUBST(USB_CFLAGS)
> > > -	AC_SUBST(USB_LIBS)
> > >  	AC_CHECK_LIB(usb, usb_get_busses, dummy=yes,
> > >  		AC_DEFINE(NEED_USB_GET_BUSSES, 1,
> > >  			[Define to 1 if you need the usb_get_busses() function.]
> > > @@ -133,6 +131,8 @@ if (test "${enable_tools}" != "no" && test "${enable_usb}" != "no"  ); then
> > >  on.]))
> > >  	AC_DEFINE(HAVE_LIBUSB, 1, [Define to 1 if you have USB library.])
> > >  fi
> > > +AC_SUBST(USB_CFLAGS)
> > > +AC_SUBST(USB_LIBS)
> > 
> > What are these changes for? I don't see any reason for them. And also
> > they should not intermix in the patch. They need to explained
> > separately.
> > 
> 
> They are meant to follow the same logic used for
> 
> AC_SUBST(UDEV_CFLAGS)
> AC_SUBST(UDEV_LIBS)
> 
> which are outside of the test.

I do not see this being valid. The logic does not work since it will
abort if enabled.

Regards

Marcel