[PATCH nft] build: disable --with-unitdir by default

[PATCH nft] build: disable --with-unitdir by default

[Pablo Neira Ayuso]

Same behaviour as in the original patch:

  --with-unitdir	auto-detects the systemd unit path.
  --with-unitdir=PATH	uses the PATH

no --with-unitdir does not install the systemd unit path.

INSTALL description looks fine for what this does.

While at this, extend tests/build/ to cover for this new option.

Fixes: c4b17cf830510 ("tools: add a systemd unit for static rulesets")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 configure.ac             | 29 +++++++++++++++++++++--------
 tests/build/run-tests.sh |  2 +-
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 42708c9f2470..3a751cb054b9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -115,15 +115,22 @@ AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [],
 ]])
 
 AC_ARG_WITH([unitdir],
-	[AS_HELP_STRING([--with-unitdir=PATH], [Path to systemd service unit directory])],
-	[unitdir="$withval"],
+	[AS_HELP_STRING([--with-unitdir[=PATH]],
+	[Path to systemd service unit directory, or omit PATH to auto-detect])],
 	[
-		unitdir=$("$PKG_CONFIG" systemd --variable systemdsystemunitdir 2>/dev/null)
-		AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
-	])
+		if test "x$withval" = "xyes"; then
+			unitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd 2>/dev/null)
+			AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
+		elif test "x$withval" = "xno"; then
+			unitdir=""
+		else
+			unitdir="$withval"
+		fi
+	],
+	[unitdir=""]
+)
 AC_SUBST([unitdir])
 
-
 AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
@@ -137,5 +144,11 @@ nft configuration:
   use mini-gmp:			${with_mini_gmp}
   enable man page:              ${enable_man_doc}
   libxtables support:		${with_xtables}
-  json output support:          ${with_json}
-  systemd unit:			${unitdir}"
+  json output support:          ${with_json}"
+
+if test "x$unitdir" != "x"; then
+AC_SUBST([unitdir])
+echo "  systemd unit:                 ${unitdir}"
+else
+echo "  systemd unit:                 no"
+fi
diff --git a/tests/build/run-tests.sh b/tests/build/run-tests.sh
index 916df2e2fa8e..1d32d5d8afcb 100755
--- a/tests/build/run-tests.sh
+++ b/tests/build/run-tests.sh
@@ -3,7 +3,7 @@
 log_file="$(pwd)/tests.log"
 dir=../..
 argument=( --without-cli --with-cli=linenoise --with-cli=editline --enable-debug --with-mini-gmp
-	   --enable-man-doc --with-xtables --with-json)
+	   --enable-man-doc --with-xtables --with-json --with-unitdir --with-unidir=/lib/systemd/system)
 ok=0
 failed=0
 
-- 
2.30.2

Re: [PATCH nft] build: disable --with-unitdir by default

[Pablo Neira Ayuso]

Hi,

Cc'ing Phil, Jan.

Excuse me my terse proposal description.

Extension: This is an alternative patch to disable --with-unitdir by
default, to address distcheck issue.

I wonder also if this is a more conservative approach, this should
integrate more seamlessly into existing pipelines while allowing
distributors to opt-in to use this.

But maybe I'm worrying too much and it is just fine to change defaults
for downstream packagers.

On Wed, Aug 27, 2025 at 04:02:14PM +0200, Pablo Neira Ayuso wrote:
> Same behaviour as in the original patch:
> 
>   --with-unitdir	auto-detects the systemd unit path.
>   --with-unitdir=PATH	uses the PATH
> 
> no --with-unitdir does not install the systemd unit path.
> 
> INSTALL description looks fine for what this does.
> 
> While at this, extend tests/build/ to cover for this new option.
> 
> Fixes: c4b17cf830510 ("tools: add a systemd unit for static rulesets")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  configure.ac             | 29 +++++++++++++++++++++--------
>  tests/build/run-tests.sh |  2 +-
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 42708c9f2470..3a751cb054b9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -115,15 +115,22 @@ AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [],
>  ]])
>  
>  AC_ARG_WITH([unitdir],
> -	[AS_HELP_STRING([--with-unitdir=PATH], [Path to systemd service unit directory])],
> -	[unitdir="$withval"],
> +	[AS_HELP_STRING([--with-unitdir[=PATH]],
> +	[Path to systemd service unit directory, or omit PATH to auto-detect])],
>  	[
> -		unitdir=$("$PKG_CONFIG" systemd --variable systemdsystemunitdir 2>/dev/null)
> -		AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
> -	])
> +		if test "x$withval" = "xyes"; then
> +			unitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd 2>/dev/null)
> +			AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
> +		elif test "x$withval" = "xno"; then
> +			unitdir=""
> +		else
> +			unitdir="$withval"
> +		fi
> +	],
> +	[unitdir=""]
> +)
>  AC_SUBST([unitdir])
>  
> -
>  AC_CONFIG_FILES([					\
>  		Makefile				\
>  		libnftables.pc				\
> @@ -137,5 +144,11 @@ nft configuration:
>    use mini-gmp:			${with_mini_gmp}
>    enable man page:              ${enable_man_doc}
>    libxtables support:		${with_xtables}
> -  json output support:          ${with_json}
> -  systemd unit:			${unitdir}"
> +  json output support:          ${with_json}"
> +
> +if test "x$unitdir" != "x"; then
> +AC_SUBST([unitdir])
> +echo "  systemd unit:                 ${unitdir}"
> +else
> +echo "  systemd unit:                 no"
> +fi
> diff --git a/tests/build/run-tests.sh b/tests/build/run-tests.sh
> index 916df2e2fa8e..1d32d5d8afcb 100755
> --- a/tests/build/run-tests.sh
> +++ b/tests/build/run-tests.sh
> @@ -3,7 +3,7 @@
>  log_file="$(pwd)/tests.log"
>  dir=../..
>  argument=( --without-cli --with-cli=linenoise --with-cli=editline --enable-debug --with-mini-gmp
> -	   --enable-man-doc --with-xtables --with-json)
> +	   --enable-man-doc --with-xtables --with-json --with-unitdir --with-unidir=/lib/systemd/system)
>  ok=0
>  failed=0
>  
> -- 
> 2.30.2
> 
> 

Re: [PATCH nft] build: disable --with-unitdir by default

[Pablo Neira Ayuso]

On Wed, Aug 27, 2025 at 04:46:17PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> Cc'ing Phil, Jan.
> 
> Excuse me my terse proposal description.
> 
> Extension: This is an alternative patch to disable --with-unitdir by
> default, to address distcheck issue.
> 
> I wonder also if this is a more conservative approach, this should
> integrate more seamlessly into existing pipelines while allowing
> distributors to opt-in to use this.
> 
> But maybe I'm worrying too much and it is just fine to change defaults
> for downstream packagers.

I'm going to proceed with this approach to release nftables 1.1.5,
if there is a need to refine, please just follow up.

Thanks.

Re: [PATCH nft] build: disable --with-unitdir by default

[Phil Sutter]

On Wed, Aug 27, 2025 at 04:46:15PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> Cc'ing Phil, Jan.
> 
> Excuse me my terse proposal description.
> 
> Extension: This is an alternative patch to disable --with-unitdir by
> default, to address distcheck issue.
> 
> I wonder also if this is a more conservative approach, this should
> integrate more seamlessly into existing pipelines while allowing
> distributors to opt-in to use this.
> 
> But maybe I'm worrying too much and it is just fine to change defaults
> for downstream packagers.

I think both is fine, packagers will know what to pass to configure and
what not.

Though I think you should also pass the --with-unitdir=<something>
option during distcheck builds, otherwise this patch inadvertently
disables the sanity check which sparked just this discussion in the
first place. :)

As Jan's patch shows, there is AM_DISTCHECK_CONFIGURE_FLAGS for that.

Thinking about the original issue, I wonder if the pkg-config method of
finding the unit install location is actually problematic: My
development build script calls configure with --prefix="$PWD/install"
and runs 'make install' as regular user. The only reason why I didn't
complain yet is probably because I don't have systemd installed and thus
'pkg-config systemd' fails.

So, I'm tempted to ask a fundamental question: Shouldn't --prefix be
applied to all installed files? I would say yes, but simply prefixing
the pkg-config-returned path by $prefix seems wrong, too as it is
definitely not where the unit file is expected to be. Also, --prefix
defaults to /usr/local and Fedora for instance passes --prefix=/usr when
building SRPMs.

OTOH Fedora (Rawhide) returns /usr/lib/systemd/system when requesting
systemdsystemunitdir. So maybe prefix $prefix only if it doesn't start
with $prefix already? (Seems silly, though.)

Cheers, Phil

Re: [PATCH nft] build: disable --with-unitdir by default

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 3800 bytes --]

On 2025-08-27, at 16:46:15 +0200, Pablo Neira Ayuso wrote:
> Cc'ing Phil, Jan.
> 
> Excuse me my terse proposal description.
> 
> Extension: This is an alternative patch to disable --with-unitdir by
> default, to address distcheck issue.
> 
> I wonder also if this is a more conservative approach, this should
> integrate more seamlessly into existing pipelines while allowing
> distributors to opt-in to use this.
> 
> But maybe I'm worrying too much and it is just fine to change defaults
> for downstream packagers.

The upshot of this is that the service file is not installed by default, but
the related man-page and the example main.nft still are.  Gueesing this is an
oversight?  If so, I will send a patch.

J.

> On Wed, Aug 27, 2025 at 04:02:14PM +0200, Pablo Neira Ayuso wrote:
> > Same behaviour as in the original patch:
> > 
> >   --with-unitdir	auto-detects the systemd unit path.
> >   --with-unitdir=PATH	uses the PATH
> > 
> > no --with-unitdir does not install the systemd unit path.
> > 
> > INSTALL description looks fine for what this does.
> > 
> > While at this, extend tests/build/ to cover for this new option.
> > 
> > Fixes: c4b17cf830510 ("tools: add a systemd unit for static rulesets")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  configure.ac             | 29 +++++++++++++++++++++--------
> >  tests/build/run-tests.sh |  2 +-
> >  2 files changed, 22 insertions(+), 9 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 42708c9f2470..3a751cb054b9 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -115,15 +115,22 @@ AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [],
> >  ]])
> >  
> >  AC_ARG_WITH([unitdir],
> > -	[AS_HELP_STRING([--with-unitdir=PATH], [Path to systemd service unit directory])],
> > -	[unitdir="$withval"],
> > +	[AS_HELP_STRING([--with-unitdir[=PATH]],
> > +	[Path to systemd service unit directory, or omit PATH to auto-detect])],
> >  	[
> > -		unitdir=$("$PKG_CONFIG" systemd --variable systemdsystemunitdir 2>/dev/null)
> > -		AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
> > -	])
> > +		if test "x$withval" = "xyes"; then
> > +			unitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd 2>/dev/null)
> > +			AS_IF([test -z "$unitdir"], [unitdir='${prefix}/lib/systemd/system'])
> > +		elif test "x$withval" = "xno"; then
> > +			unitdir=""
> > +		else
> > +			unitdir="$withval"
> > +		fi
> > +	],
> > +	[unitdir=""]
> > +)
> >  AC_SUBST([unitdir])
> >  
> > -
> >  AC_CONFIG_FILES([					\
> >  		Makefile				\
> >  		libnftables.pc				\
> > @@ -137,5 +144,11 @@ nft configuration:
> >    use mini-gmp:			${with_mini_gmp}
> >    enable man page:              ${enable_man_doc}
> >    libxtables support:		${with_xtables}
> > -  json output support:          ${with_json}
> > -  systemd unit:			${unitdir}"
> > +  json output support:          ${with_json}"
> > +
> > +if test "x$unitdir" != "x"; then
> > +AC_SUBST([unitdir])
> > +echo "  systemd unit:                 ${unitdir}"
> > +else
> > +echo "  systemd unit:                 no"
> > +fi
> > diff --git a/tests/build/run-tests.sh b/tests/build/run-tests.sh
> > index 916df2e2fa8e..1d32d5d8afcb 100755
> > --- a/tests/build/run-tests.sh
> > +++ b/tests/build/run-tests.sh
> > @@ -3,7 +3,7 @@
> >  log_file="$(pwd)/tests.log"
> >  dir=../..
> >  argument=( --without-cli --with-cli=linenoise --with-cli=editline --enable-debug --with-mini-gmp
> > -	   --enable-man-doc --with-xtables --with-json)
> > +	   --enable-man-doc --with-xtables --with-json --with-unitdir --with-unidir=/lib/systemd/system)
> >  ok=0
> >  failed=0
> >  
> > -- 
> > 2.30.2
> > 
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 931 bytes --]

Re: [PATCH nft] build: disable --with-unitdir by default

[Pablo Neira Ayuso]

On Fri, Aug 29, 2025 at 04:43:50PM +0100, Jeremy Sowden wrote:
> On 2025-08-27, at 16:46:15 +0200, Pablo Neira Ayuso wrote:
> > Cc'ing Phil, Jan.
> > 
> > Excuse me my terse proposal description.
> > 
> > Extension: This is an alternative patch to disable --with-unitdir by
> > default, to address distcheck issue.
> > 
> > I wonder also if this is a more conservative approach, this should
> > integrate more seamlessly into existing pipelines while allowing
> > distributors to opt-in to use this.
> > 
> > But maybe I'm worrying too much and it is just fine to change defaults
> > for downstream packagers.
> 
> The upshot of this is that the service file is not installed by default, but
> the related man-page and the example main.nft still are.  Gueesing this is an
> oversight?  If so, I will send a patch.

Yes, it is an oversight. Thanks.