Message-ID: <YWBCx6yvm7gDZNId@renaissance-vector>
Date:   Fri, 8 Oct 2021 15:08:23 +0200
From:   Andrea Claudi <aclaudi@...hat.com>
To:     Phil Sutter <phil@....cc>, netdev@...r.kernel.org,
        stephen@...workplumber.org, dsahern@...il.com, bluca@...ian.org,
        haliu@...hat.com
Subject: Re: [PATCH iproute2 v4 0/5] configure: add support for libdir and
 prefix option

On Thu, Oct 07, 2021 at 06:02:02PM +0200, Phil Sutter wrote:
> 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
> | 	;;
> | [...]
> 

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

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.

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

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

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?

Regards,
Andrea

[1] https://lore.kernel.org/netdev/6363502d3ce806acdbc7ba194ddc98d3fac064de.camel@debian.org/

Powered by blists - more mailing lists