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

[Phil Sutter <phil@nwl.cc>]

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

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

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

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

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

Cheers, Phil