Opened 5 years ago

Closed 5 years ago

#14350 closed defect (fixed)

configure.ac: fix disabling systemd notification support

Reported by: blueness Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If --disable-systemd is given, we set $systemd to false, not $enable_systemd. As a result, the way configure.ac is currently written, if libsystemd is found, we still turn on systemd support even if we explicitly disable it with --disable-system.

Child Tickets

Attachments (1)

0001-configure.ac-fix-disabling-systemd-notification-supp.patch (834 bytes) - added by blueness 5 years ago.
Alternative fix making consistent use of $enable_system as we do elsewhere

Download all attachments as: .zip

Change History (4)

comment:1 Changed 5 years ago by nickm

Status: newneeds_review

But there's another occurrence of "$enable_systemd" later in the file; shouldn't this one change too?

if test x$enable_systemd = xyes -a x$have_systemd != xyes ; then
    AC_MSG_ERROR([Explicitly requested systemd support, but systemd not found])
fi

Maybe it would be better to do:

diff --git a/configure.ac b/configure.ac
index 9aac1e8..89fa882 100644
--- a/configure.ac
+++ b/configure.ac
@@ -118,10 +118,10 @@ AC_ARG_ENABLE(upnp,
 AC_ARG_ENABLE(systemd,
       AS_HELP_STRING(--enable-systemd, enable systemd notification support),
       [case "${enableval}" in
-        yes) systemd=true ;;
-        no)  systemd=false ;;
+        yes) enable_systemd=true ;;
+        no)  enable_systemd=false ;;
         * ) AC_MSG_ERROR(bad value for --enable-systemd) ;;
-      esac], [systemd=auto])
+      esac], [enable_systemd=auto])

What do you think?

comment:2 Changed 5 years ago by blueness

Okay a couple of points about how autoconf works.

AC_ARG_ENABLE(systemd ...) automatically sets $enable_systemd to "yes" if --enable-systemd is passed, to "no" if --disable-systemd is given and to "foo" if -enable-systemd="foo" is given. Its equal to "" if niether --enable-systemd nor --disable-systemd are given. So the code is inconsistent but works int that it uses $systemd in one palce and $enable_systemd in another. I can make that consistant.

You should never use $enableval becuase it is not reset between instances of AC_ARG_ENABLE(). However, in this case it works because you are uisng it in the 3rd parameter which is only run because you gave --{enable,disable}-systemd. So its net effect is to set shadow variable $systemd to true or false and make sure soemone didn't pass something like --enable-systemd="silly".

Convince yourself of how AC_ARG_ENABLE() works by playing with

AC_PREREQ([2.69])
AC_INIT([x],[1],[])

AC_ARG_ENABLE(a, [],
  [case "${enableval}" in
      yes) echo "a true" ;;
      no) echo "a false" ;;
      *) echo "a bad" ;;
   esac], [a=true])

echo "enableval="${enableval}
echo "enable_a="${enable_a}

AC_ARG_ENABLE(b, [],
  [case "${enableval}" in
      yes) echo "b true" ;;
      no) echo "b false" ;;
      *) echo "b bad" ;;
   esac], [b=true])

echo "enableval="${enableval}
echo "enable_b="${enable_b}
AC_OUTPUT

Try running it with ./configure --enable-a and you'll see the second echo of ${enableval} is yes because it was never reset.

Looking at the whole configure.a, there are a few places you make use of these shadowing variables like $systemd and $enable_system probably because whoever wrote that isn't aware of what AC_ARG_ENABLE() does behind the scenes. If you want, I can clean that up, but I try not to be too intrusive in how upstream likes to do things.

Last edited 5 years ago by blueness (previous) (diff)

Changed 5 years ago by blueness

Alternative fix making consistent use of $enable_system as we do elsewhere

comment:3 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay; merged. Thanks!

(I'd be happy to take a cleanup patch or two on configure.ac. It's mostly maintained and written through cargo-cult processes, apparently.)

Note: See TracTickets for help on using tickets.