[PATCH BlueZ] configure.ac: call AC_SUBST unconditionally

[PATCH BlueZ] configure.ac: call AC_SUBST unconditionally

[Antonio Ospite]

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?

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}])
 
 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)
 AM_CONDITIONAL(USB, test "${enable_usb}" != "no")
 
 AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
@@ -157,8 +157,8 @@ if (test "${enable_udev}" != "no" && test -z "${path_udevdir}"); then
 		AC_MSG_ERROR([udev directory is required])
 	fi
 	AC_MSG_RESULT([${path_udevdir}])
-	AC_SUBST(UDEV_DIR, [${path_udevdir}])
 fi
+AC_SUBST(UDEV_DIR, [${path_udevdir}])
 
 AM_CONDITIONAL(HID2HCI, test "${enable_tools}" != "no" &&
 		test "${enable_udev}" != "no" && test "${enable_usb}" != "no")
@@ -202,8 +202,8 @@ if (test "${enable_systemd}" != "no" && test -z "${path_systemunitdir}"); then
 		AC_MSG_ERROR([systemd system unit directory is required])
 	fi
 	AC_MSG_RESULT([${path_systemunitdir}])
-	AC_SUBST(SYSTEMD_SYSTEMUNITDIR, [${path_systemunitdir}])
 fi
+AC_SUBST(SYSTEMD_SYSTEMUNITDIR, [${path_systemunitdir}])
 
 AC_ARG_WITH([systemduserunitdir],
 			AC_HELP_STRING([--with-systemduserunitdir=DIR],
@@ -216,8 +216,8 @@ if (test "${enable_systemd}" != "no" && test -z "${path_userunitdir}"); then
 		AC_MSG_ERROR([systemd user unit directory is required])
 	fi
 	AC_MSG_RESULT([${path_userunitdir}])
-	AC_SUBST(SYSTEMD_USERUNITDIR, [${path_userunitdir}])
 fi
+AC_SUBST(SYSTEMD_USERUNITDIR, [${path_userunitdir}])
 
 AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 			[do not install configuration and data files]),
-- 
1.7.10.4

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

[Marcel Holtmann]

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.

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

Regards

Marcel

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

[Antonio Ospite]

On Thu, 07 Feb 2013 10:37:07 +0200
Marcel Holtmann <marcel@holtmann.org> wrote:

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

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

Unrelated, right, sorry. Maybe I'll send another patch for them.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

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

[Marcel Holtmann]

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

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

[Antonio Ospite]

On Fri, 08 Feb 2013 13:21:31 +0200
Marcel Holtmann <marcel@holtmann.org> wrote:

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

I will, thanks.

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

I double checked and I think that actually we can do the reverse that
this hunk was trying to do: bring inside the "if" tests other AC_SUBST
(*_CFLAGS) just like it is now done for USB_CFLAGS and USB_LIBS.

I'll try to explain that better in the commit message for the patch and
you'll decide whether to pick that up or not.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCHv2 BlueZ 0/2] configure.ac fixes

[Antonio Ospite]

Hi,

Patch 1 fixes --with-* options, it should be OK as Marcel
acknowledged.

Patch 2 is more for the sake of regularity with USB_CFLAGS and
USB_LIBS, it's not strictly necessary.

Thanks,
   Antonio

Antonio Ospite (2):
  configure.ac: call AC_SUBST unconditionally with --with-* options
  Call AC_SUBST(*_CFLAGS) and AC_SUBST(*_LIBS) only when needed

 configure.ac |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCHv2 BlueZ 1/2] configure.ac: call AC_SUBST unconditionally with --with-* options

[Antonio Ospite]

Call AC_SUBST unconditionally when specifying --with-* options,
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

This is what was going on in configure.ac:

  # 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 automatically
    path_dbusconfdir="`$PKG_CONFIG --variable=sysconfdir dbus-1`"

    ...

    # set DBUS_CONFDIR
    AC_SUBST(DBUS_CONFDIR, [${path_dbusconfdir}])
  endif

when --with-dbusconfdir=SOMEDIR was used the test above failed, and the
result was that ${path_dbusconfdir} was indeed defined as manually
specified, but DBUS_CONFDIR was not, and the latter was going to be used
in Makefile.am:

  dbusdir = @DBUS_CONFDIR@/dbus-1/system.d

The failure in mkdir can be exposed by the use of the "--prefix" option
and by running "make install" as a normal user; when running "make
install" with the root user /dbus-1/system.d would be happily (and
wrongly) created.

By always setting variables relative to --with-* options (like
DBUS_CONFDIR) the cases when --with-someoption=SOMEDIR are used get
covered.
---
 configure.ac |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index 070acea..b11dcf1 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}])
 
 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}])
@@ -157,8 +157,8 @@ if (test "${enable_udev}" != "no" && test -z "${path_udevdir}"); then
 		AC_MSG_ERROR([udev directory is required])
 	fi
 	AC_MSG_RESULT([${path_udevdir}])
-	AC_SUBST(UDEV_DIR, [${path_udevdir}])
 fi
+AC_SUBST(UDEV_DIR, [${path_udevdir}])
 
 AM_CONDITIONAL(HID2HCI, test "${enable_tools}" != "no" &&
 		test "${enable_udev}" != "no" && test "${enable_usb}" != "no")
@@ -202,8 +202,8 @@ if (test "${enable_systemd}" != "no" && test -z "${path_systemunitdir}"); then
 		AC_MSG_ERROR([systemd system unit directory is required])
 	fi
 	AC_MSG_RESULT([${path_systemunitdir}])
-	AC_SUBST(SYSTEMD_SYSTEMUNITDIR, [${path_systemunitdir}])
 fi
+AC_SUBST(SYSTEMD_SYSTEMUNITDIR, [${path_systemunitdir}])
 
 AC_ARG_WITH([systemduserunitdir],
 			AC_HELP_STRING([--with-systemduserunitdir=DIR],
@@ -216,8 +216,8 @@ if (test "${enable_systemd}" != "no" && test -z "${path_userunitdir}"); then
 		AC_MSG_ERROR([systemd user unit directory is required])
 	fi
 	AC_MSG_RESULT([${path_userunitdir}])
-	AC_SUBST(SYSTEMD_USERUNITDIR, [${path_userunitdir}])
 fi
+AC_SUBST(SYSTEMD_USERUNITDIR, [${path_userunitdir}])
 
 AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 			[do not install configuration and data files]),
-- 
1.7.10.4

[PATCHv2 BlueZ 2/2] configure.ac: call AC_SUBST(*_CFLAGS) and AC_SUBST(*_LIBS) only when needed

[Antonio Ospite]

Bring AC_SUBST(*_CFLAGS) and AC_SUBST(*_LIBS) in the same block of the
corresponding PKG_CHECK_MODULES() call.

Having these variables defined outside of the if tests is more than what
is needed as the corresponding PKG_CHECK_MODULES() might not have been
called at all there.

This is the same logic already used for USB_CFLAGS and USB_LIBS.
---
 configure.ac |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index b11dcf1..4ac8a78 100644
--- a/configure.ac
+++ b/configure.ac
@@ -140,12 +140,12 @@ AC_ARG_ENABLE(udev, AC_HELP_STRING([--disable-udev],
 if (test "${enable_tools}" != "no" && test "${enable_udev}" != "no"); then
 	PKG_CHECK_MODULES(UDEV, libudev >= 143, dummy=yes,
 				AC_MSG_ERROR(libudev >= 143 is required))
+	AC_SUBST(UDEV_CFLAGS)
+	AC_SUBST(UDEV_LIBS)
 	AC_CHECK_LIB(udev, udev_hwdb_new,
 		AC_DEFINE(HAVE_UDEV_HWDB_NEW, 1,
 			[Define to 1 if you have the udev_hwdb_new() function.]))
 fi
-AC_SUBST(UDEV_CFLAGS)
-AC_SUBST(UDEV_LIBS)
 AM_CONDITIONAL(UDEV, test "${enable_udev}" != "no")
 
 AC_ARG_WITH([udevdir], AC_HELP_STRING([--with-udevdir=DIR],
@@ -172,9 +172,9 @@ AC_ARG_ENABLE(obex, AC_HELP_STRING([--disable-obex],
 if (test "${enable_obex}" != "no"); then
 	PKG_CHECK_MODULES(ICAL, libical, dummy=yes,
 					AC_MSG_ERROR(libical is required))
+	AC_SUBST(ICAL_CFLAGS)
+	AC_SUBST(ICAL_LIBS)
 fi
-AC_SUBST(ICAL_CFLAGS)
-AC_SUBST(ICAL_LIBS)
 AM_CONDITIONAL(OBEX, test "${enable_obex}" != "no")
 
 AC_ARG_ENABLE(client, AC_HELP_STRING([--disable-client],
-- 
1.7.10.4

Re: [PATCHv2 BlueZ 0/2] configure.ac fixes

[Johan Hedberg]

Hi Antonio,

On Sun, Feb 10, 2013, Antonio Ospite wrote:
> Patch 1 fixes --with-* options, it should be OK as Marcel
> acknowledged.
> 
> Patch 2 is more for the sake of regularity with USB_CFLAGS and
> USB_LIBS, it's not strictly necessary.
> 
> Thanks,
>    Antonio
> 
> Antonio Ospite (2):
>   configure.ac: call AC_SUBST unconditionally with --with-* options
>   Call AC_SUBST(*_CFLAGS) and AC_SUBST(*_LIBS) only when needed
> 
>  configure.ac |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Both patches have been applied. Thanks.

Johan