| Message ID | cover.1633612111.git.aclaudi@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | configure: add support for libdir and prefix option | expand |
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
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/
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
On Fri, Oct 08, 2021 at 03:50:25PM +0200, Phil Sutter wrote: [...] > > 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. > Yes, this error can be disregarded. Still I would try to avoid a meaningless error message, if possible. [...] > > 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. [...] > > > 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. I don't know about autotools, but Debian packaging system makes use of this, and we cannot break their workflow. Regards, Andrea
On 10/8/21 10:19 AM, Andrea Claudi wrote: > >> >> 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. that is definitely not intended and I don't see a reason to allow it.
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 Changelog: ---------- v3 -> v4 - fix parsing issue on '--include_dir' and '--libbpf_dir' - split '--opt value' and '--opt=value' use cases, avoid code duplication moving semantic checks on value to dedicated functions v2 -> v3 - fix parsing error on prefix and libdir options. v1 -> v2 - consolidate '--opt value' and '--opt=value' use cases, as suggested by David Ahern. - added patch 2 to manage the --prefix option, used by the Debian packaging system, as reported by Luca Boccassi, and use it when setting lib directory. Andrea Claudi (5): configure: fix parsing issue on include_dir option configure: fix parsing issue on libbpf_dir option configure: support --param=value style configure: add the --prefix option configure: add the --libdir option Makefile | 7 +++-- configure | 85 +++++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 80 insertions(+), 12 deletions(-)