Re: [PATCH iproute2 v4 0/5] configure: add support for libdir and prefix option

Hi Andrea,

On Thu, Oct 07, 2021 at 03:40:00PM +0200, Andrea Claudi wrote:

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.

Patches 1-2 fix a parsing issue on current configure options, that may
trigger an endless loop when no value is provided with some options;

Hmm, "shift 2" is nasty. Good to be reminded that it fails if '$# < 2'.
I would avoid the loop using single shifts:

| case "$1" in
| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| [...]

Patch 3 introduces support for the --opt=value style on current options,
for uniformity;

My idea to avoid code duplication was to move the semantic checks out of
the argument parsing loop, basically:

| [ -d "$INCLUDE" ] || usage 1
| case "$LIBBPF_FORCE" in
| 	on|off|"") ;;
| 	*) usage 1 ;;
| esac

after the loop or even before 'echo "# Generated config ...'. This
reduces the parsing loop to cases like:

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

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

So this parses into $PREFIX and when checking it assigns to $prefix but
neither one of the two variables is used afterwards? Oh, there's patch
5 ...

Patch 5 add the --libdir option, and also drops the static LIBDIR var
from the Makefile

Can't you just:

| [ -n "$PREFIX" ] && echo "PREFIX=\"$PREFIX\"" >>config.mk
| [ -n "$LIBDIR" ] && echo "LIBDIR=\"$LIBDIR\"" >>config.mk

and leave the default ("?=") cases in Makefile in place?

Either way, calling 'eval' seems needless. I would avoid it at all
costs, "eval is evil". ;)

Cheers, Phil