configure: add support for libdir and prefix option

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;

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

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

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

Hi Andrea,

Quoted 8 lines · Andrea Claudi

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

Quoted 2 lines

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

Quoted 2 lines

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

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

Quoted 13 lines · Phil Sutter

Quoted 8 lines · Andrea Claudi

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:

Quoted 8 lines

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

Quoted 15 lines

Quoted 2 lines

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:

Quoted 10 lines

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

Quoted 12 lines

Quoted 2 lines

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

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

Can't you just:

Quoted 7 lines

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

Quoted 5 lines

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.

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

Quoted 14 lines

Quoted 9 lines

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

Quoted 3 lines

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.

Quoted 6 lines

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

We already accept things like this in the current configure, and I would try to not modify current behaviour as much as possible.