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

Hi,

On Fri, Oct 08, 2021 at 03:08:23PM +0200, Andrea Claudi wrote:

On Thu, Oct 07, 2021 at 06:02:02PM +0200, Phil Sutter wrote:

quoted

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

quoted

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
| 	;;
| [...]

This avoid the endless loop and allows configure to terminate correctly,
but results in an error anyway:

$ ./configure --include_dir
./configure: line 544: shift: shift count out of range

Ah, I didn't see it with bash. I don't think it's a problem though:
Input is invalid, the loop is avoided and (depending on your patches)
there will be another error message complaining about invalid $INCLUDE
value.

But thanks anyway! Your comment made me think again about this, and I
think we can use the *) case to actually get rid of the second shift.

Indeed, when an option is specified, the --opt case will shift and get
its value, then the next while loop will take the *) case, and the
second shift is triggered this way.

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

quoted

quoted

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
| 	;;

Thanks. I didn't think about '-d', this also cover corner cases like:

$ ./configure --include_dir --libbpf_force off

that results in INCLUDE="--libbpf_force".

A common case would be (note the typo):

| ./configure --include_dir $MY_INCULDE_DIR --libbpf_force off

quoted

quoted

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 ...

quoted

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". ;)

Unfortunately this is needed because some packaging systems uses
${prefix} as an argument to --libdir, expecting this to be replaced with
the value of --prefix. See Luca's review to v1 for an example [1].

I can always avoid the eval trying to parse "${prefix}" and replacing it
with the PREFIX value, but in this case "eval" seems a bit more
practical to me... WDYT?

Do autotools support that? If not, I wouldn't bother.

Cheers, Phil