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

[Phil Sutter <phil@nwl.cc>]

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