[PATCH iproute2 v3 0/3] configure: add support for libdir and prefix option

[PATCH iproute2 v3 0/3] configure: add support for libdir and prefix option

[Andrea Claudi]

This series add support for the libdir parameter in iproute2 configure
system. The idea is to make use of the fact that packaging systems may
assume that 'configure' comes from autotools allowing a syntax similar
to the autotools one, and using it to tell iproute2 where the distro
expects to find its lib files.

Patch 1 introduces support for the --param=value style on current
params, for uniformity.

Patch 2 add the --prefix option, that may be used by some packaging
systems when calling the configure script.

Patch 3 add the --libdir option to the configure script, and also drops
the static LIBDIR var from the Makefile.

Changelog:
----------
v2 -> v3
  - Fix parsing error on prefix and libdir options.

v1 -> v2
  - consolidate '--param value' and '--param=value' use cases, as
    suggested by David Ahern.
  - Added patch 2 to manage the --prefix option, used by the Debian
    packaging system, as reported by Luca Boccassi, and use it when
    setting lib directory.

Andrea Claudi (3):
  configure: support --param=value style
  configure: add the --prefix option
  configure: add the --libdir option

 Makefile  |  7 +++---
 configure | 72 +++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 66 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH iproute2 v3 1/3] configure: support --param=value style

[Andrea Claudi]

This commit makes it possible to specify values for configure params
using the common autotools configure syntax '--param=value'.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 configure | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 7f4f3bd9..d57ce0f8 100755
--- a/configure
+++ b/configure
@@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
 else
 	while true; do
 		case "$1" in
-			--include_dir)
-				INCLUDE=$2
-				shift 2 ;;
-			--libbpf_dir)
-				LIBBPF_DIR="$2"
-				shift 2 ;;
-			--libbpf_force)
-				if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then
+			--include_dir | --include_dir=*)
+				INCLUDE="${1#*=}"
+				if [ "$INCLUDE" == "--include_dir" ]; then
+					INCLUDE=$2
+					shift
+				fi
+				shift ;;
+			--libbpf_dir | --libbpf_dir=*)
+				LIBBPF_DIR="${1#*=}"
+				if [ "$LIBBPF_DIR" == "--libbpf_dir" ]; then
+					LIBBPF_DIR="$2"
+					shift
+				fi
+				shift ;;
+			--libbpf_force | --libbpf_force=*)
+				LIBBPF_FORCE="${1#*=}"
+				if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then
+					LIBBPF_FORCE="$2"
+					shift
+				fi
+				if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then
 					usage 1
 				fi
-				LIBBPF_FORCE=$2
-				shift 2 ;;
+				shift ;;
 			-h | --help)
 				usage 0 ;;
 			"")
-- 
2.31.1

Re: [PATCH iproute2 v3 1/3] configure: support --param=value style

[Phil Sutter]

Hi Andrea,

A remark regarding coding style:

On Wed, Oct 06, 2021 at 12:08:04AM +0200, Andrea Claudi wrote:
[...]
> diff --git a/configure b/configure
> index 7f4f3bd9..d57ce0f8 100755
> --- a/configure
> +++ b/configure
> @@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
>  else
>  	while true; do
>  		case "$1" in
> -			--include_dir)
> -				INCLUDE=$2
> -				shift 2 ;;
> -			--libbpf_dir)
> -				LIBBPF_DIR="$2"
> -				shift 2 ;;
> -			--libbpf_force)
> -				if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then
> +			--include_dir | --include_dir=*)

So here the code combines the two cases,

> +				INCLUDE="${1#*=}"
> +				if [ "$INCLUDE" == "--include_dir" ]; then

just to fiddle it apart again. Did you consider leaving the old cases in
place and adding separate ones for the --opt=val cases like so:

| 			--include_dir=*)
| 				INCLUDE="${1#*=}"
| 				shift
| 				;;

[...]
> +			--libbpf_force | --libbpf_force=*)
> +				LIBBPF_FORCE="${1#*=}"
> +				if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then
> +					LIBBPF_FORCE="$2"
> +					shift
> +				fi
> +				if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then

To avoid duplication here, I would move semantic checks into a second
step. This would allow for things like:

| --libbpf_force=invalid --libbpf_force=on

but separating the syntactic parsing from semantic checks might be
beneficial by itself, too.

Cheers, Phil

Re: [PATCH iproute2 v3 1/3] configure: support --param=value style

[Andrea Claudi]

On Wed, Oct 06, 2021 at 10:09:44AM +0200, Phil Sutter wrote:
> Hi Andrea,
> 
> A remark regarding coding style:
> 

Hi Phil,
Thanks for your review.

> On Wed, Oct 06, 2021 at 12:08:04AM +0200, Andrea Claudi wrote:
> [...]
> > diff --git a/configure b/configure
> > index 7f4f3bd9..d57ce0f8 100755
> > --- a/configure
> > +++ b/configure
> > @@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
> >  else
> >  	while true; do
> >  		case "$1" in
> > -			--include_dir)
> > -				INCLUDE=$2
> > -				shift 2 ;;
> > -			--libbpf_dir)
> > -				LIBBPF_DIR="$2"
> > -				shift 2 ;;
> > -			--libbpf_force)
> > -				if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then
> > +			--include_dir | --include_dir=*)
> 
> So here the code combines the two cases,
> 
> > +				INCLUDE="${1#*=}"
> > +				if [ "$INCLUDE" == "--include_dir" ]; then
> 
> just to fiddle it apart again. Did you consider leaving the old cases in
> place and adding separate ones for the --opt=val cases like so:
> 
> | 			--include_dir=*)
> | 				INCLUDE="${1#*=}"
> | 				shift
> | 				;;
> 
> [...]

That was my first proposal in v1 [1]. I changed it on David's suggestion
to consolidate the two cases into a single one.

Looking at the resulting code, v3 code results in an extra check to
discriminate between the two use cases, while v0 uses the "case"
structure to the same end.

> > +			--libbpf_force | --libbpf_force=*)
> > +				LIBBPF_FORCE="${1#*=}"
> > +				if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then
> > +					LIBBPF_FORCE="$2"
> > +					shift
> > +				fi
> > +				if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then
> 
> To avoid duplication here, I would move semantic checks into a second
> step. This would allow for things like:
> 
> | --libbpf_force=invalid --libbpf_force=on
> 
> but separating the syntactic parsing from semantic checks might be
> beneficial by itself, too.

Yes, I agree with you. David, does this answer to your concern about v1?
If yes, I would proceed with a v4 integrating Phil's suggestions.

> 
> Cheers, Phil
>

[1] https://lore.kernel.org/netdev/cover.1633191885.git.aclaudi@redhat.com/

Re: [PATCH iproute2 v3 1/3] configure: support --param=value style

[Phil Sutter]

Hi Andrea,

On Wed, Oct 06, 2021 at 11:49:34AM +0200, Andrea Claudi wrote:
[...]
> That was my first proposal in v1 [1]. I changed it on David's suggestion
> to consolidate the two cases into a single one.

Oh, sorry. I missed the 'v3' tag and hence didn't check any earlier
version.

> Looking at the resulting code, v3 code results in an extra check to
> discriminate between the two use cases, while v0 uses the "case"
> structure to the same end.

Given that David explicitly requested the change I'm complaining about
in his reply to your initial version, I guess any further debate about
it is irrelevant.

Thanks, Phil

Re: [PATCH iproute2 v3 1/3] configure: support --param=value style

[David Ahern]

On 10/6/21 4:18 AM, Phil Sutter wrote:
> Hi Andrea,
> 
> On Wed, Oct 06, 2021 at 11:49:34AM +0200, Andrea Claudi wrote:
> [...]
>> That was my first proposal in v1 [1]. I changed it on David's suggestion
>> to consolidate the two cases into a single one.
> 
> Oh, sorry. I missed the 'v3' tag and hence didn't check any earlier
> version.
> 
>> Looking at the resulting code, v3 code results in an extra check to
>> discriminate between the two use cases, while v0 uses the "case"
>> structure to the same end.
> 
> Given that David explicitly requested the change I'm complaining about
> in his reply to your initial version, I guess any further debate about
> it is irrelevant.
> 


I did not like the exploding duplication of checks on the value just to
support '=' in the command line.

Phil's suggestion seems reasonable.

[PATCH iproute2 v3 2/3] configure: add the --prefix option

[Andrea Claudi]

This commit add the '--prefix' option to the iproute2 configure script.

This mimics the '--prefix' option that autotools configure provides, and
will be used later to allow users or packagers to set the lib directory.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 configure | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/configure b/configure
index d57ce0f8..8be7e40b 100755
--- a/configure
+++ b/configure
@@ -148,6 +148,15 @@ EOF
 	rm -f $TMPDIR/ipttest.c $TMPDIR/ipttest
 }
 
+check_prefix()
+{
+	if [ -n "$PREFIX" ]; then
+		prefix="$PREFIX"
+	else
+		prefix="/usr"
+	fi
+}
+
 check_ipt()
 {
 	if ! grep TC_CONFIG_XT $CONFIG > /dev/null; then
@@ -490,6 +499,7 @@ Usage: $0 [OPTIONS]
 	--libbpf_force		Enable/disable libbpf by force. Available options:
 				  on: require link against libbpf, quit config if no libbpf support
 				  off: disable libbpf probing
+	--prefix		Path prefix of the lib files to install
 	-h | --help		Show this usage info
 EOF
 	exit $1
@@ -525,6 +535,13 @@ else
 					usage 1
 				fi
 				shift ;;
+			--prefix | --prefix=*)
+				PREFIX="${1#*=}"
+				if [ "$PREFIX" == "--prefix" ]; then
+					PREFIX="$2"
+					shift
+				fi
+				shift ;;
 			-h | --help)
 				usage 0 ;;
 			"")
@@ -558,6 +575,7 @@ if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 fi
 
 echo
+check_prefix
 if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 	echo -n "iptables modules directory: "
 	check_ipt_lib_dir
-- 
2.31.1

[PATCH iproute2 v3 3/3] configure: add the --libdir option

[Andrea Claudi]

This commit allows users/packagers to choose a lib directory to store
iproute2 lib files.

At the moment iproute2 ship lib files in /usr/lib and offers no way to
modify this setting. However, according to the FHS, distros may choose
"one or more variants of the /lib directory on systems which support
more than one binary format" (e.g. /usr/lib64 on Fedora).

As Luca states in commit a3272b93725a ("configure: restore backward
compatibility"), packaging systems may assume that 'configure' is from
autotools, and try to pass it some parameters.

Allowing the '--libdir=/path/to/libdir' syntax, we can use this to our
advantage, and let the lib directory to be chosen by the distro
packaging system.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 Makefile  |  7 ++++---
 configure | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 5bc11477..45655ca4 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Top level Makefile for iproute2
 
+-include config.mk
+
 ifeq ("$(origin V)", "command line")
 VERBOSE = $(V)
 endif
@@ -13,7 +15,6 @@ MAKEFLAGS += --no-print-directory
 endif
 
 PREFIX?=/usr
-LIBDIR?=$(PREFIX)/lib
 SBINDIR?=/sbin
 CONFDIR?=/etc/iproute2
 NETNS_RUN_DIR?=/var/run/netns
@@ -60,7 +61,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man vdpa
 LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a
 LDLIBS += $(LIBNETLINK)
 
-all: config
+all: config.mk
 	@set -e; \
 	for i in $(SUBDIRS); \
 	do echo; echo $$i; $(MAKE) -C $$i; done
@@ -80,7 +81,7 @@ help:
 	@echo "Make Arguments:"
 	@echo " V=[0|1]             - set build verbosity level"
 
-config:
+config.mk:
 	@if [ ! -f config.mk -o configure -nt config.mk ]; then \
 		sh configure $(KERNEL_INCLUDE); \
 	fi
diff --git a/configure b/configure
index 8be7e40b..858d2e41 100755
--- a/configure
+++ b/configure
@@ -157,6 +157,19 @@ check_prefix()
 	fi
 }
 
+check_lib_dir()
+{
+	echo -n "lib directory: "
+	if [ -n "$LIBDIR" ]; then
+		eval echo "$LIBDIR"
+		eval echo "LIBDIR:=$LIBDIR" >> $CONFIG
+		return
+	fi
+
+	eval echo "${prefix}/lib"
+	eval echo "LIBDIR:=${prefix}/lib" >> $CONFIG
+}
+
 check_ipt()
 {
 	if ! grep TC_CONFIG_XT $CONFIG > /dev/null; then
@@ -495,6 +508,7 @@ usage()
 	cat <<EOF
 Usage: $0 [OPTIONS]
 	--include_dir		Path to iproute2 include dir
+	--libdir		Path to iproute2 lib dir
 	--libbpf_dir		Path to libbpf DESTDIR
 	--libbpf_force		Enable/disable libbpf by force. Available options:
 				  on: require link against libbpf, quit config if no libbpf support
@@ -518,6 +532,13 @@ else
 					shift
 				fi
 				shift ;;
+			--libdir | --libdir=*)
+				LIBDIR="${1#*=}"
+				if [ "$LIBDIR" == "--libdir" ]; then
+					LIBDIR="$2"
+					shift
+				fi
+				shift ;;
 			--libbpf_dir | --libbpf_dir=*)
 				LIBBPF_DIR="${1#*=}"
 				if [ "$LIBBPF_DIR" == "--libbpf_dir" ]; then
@@ -576,6 +597,7 @@ fi
 
 echo
 check_prefix
+check_lib_dir
 if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 	echo -n "iptables modules directory: "
 	check_ipt_lib_dir
-- 
2.31.1