| Message ID | caa9b65bef41acd51d45e45e1a158edb1eeefe7d.1633455436.git.aclaudi@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | configure: add support for libdir and prefix option | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Andrea, A remark regarding coding style: On Wed, Oct 06, 2021 at 12:08:04AM +0200, Andrea Claudi wrote: [...] > diff --git a/configure b/configure > index 7f4f3bd9..d57ce0f8 100755 > --- a/configure > +++ b/configure > @@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then > else > while true; do > case "$1" in > - --include_dir) > - INCLUDE=$2 > - shift 2 ;; > - --libbpf_dir) > - LIBBPF_DIR="$2" > - shift 2 ;; > - --libbpf_force) > - if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then > + --include_dir | --include_dir=*) So here the code combines the two cases, > + INCLUDE="${1#*=}" > + if [ "$INCLUDE" == "--include_dir" ]; then just to fiddle it apart again. Did you consider leaving the old cases in place and adding separate ones for the --opt=val cases like so: | --include_dir=*) | INCLUDE="${1#*=}" | shift | ;; [...] > + --libbpf_force | --libbpf_force=*) > + LIBBPF_FORCE="${1#*=}" > + if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then > + LIBBPF_FORCE="$2" > + shift > + fi > + if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then To avoid duplication here, I would move semantic checks into a second step. This would allow for things like: | --libbpf_force=invalid --libbpf_force=on but separating the syntactic parsing from semantic checks might be beneficial by itself, too. Cheers, Phil
On Wed, Oct 06, 2021 at 10:09:44AM +0200, Phil Sutter wrote: > Hi Andrea, > > A remark regarding coding style: > Hi Phil, Thanks for your review. > On Wed, Oct 06, 2021 at 12:08:04AM +0200, Andrea Claudi wrote: > [...] > > diff --git a/configure b/configure > > index 7f4f3bd9..d57ce0f8 100755 > > --- a/configure > > +++ b/configure > > @@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then > > else > > while true; do > > case "$1" in > > - --include_dir) > > - INCLUDE=$2 > > - shift 2 ;; > > - --libbpf_dir) > > - LIBBPF_DIR="$2" > > - shift 2 ;; > > - --libbpf_force) > > - if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then > > + --include_dir | --include_dir=*) > > So here the code combines the two cases, > > > + INCLUDE="${1#*=}" > > + if [ "$INCLUDE" == "--include_dir" ]; then > > just to fiddle it apart again. Did you consider leaving the old cases in > place and adding separate ones for the --opt=val cases like so: > > | --include_dir=*) > | INCLUDE="${1#*=}" > | shift > | ;; > > [...] That was my first proposal in v1 [1]. I changed it on David's suggestion to consolidate the two cases into a single one. Looking at the resulting code, v3 code results in an extra check to discriminate between the two use cases, while v0 uses the "case" structure to the same end. > > + --libbpf_force | --libbpf_force=*) > > + LIBBPF_FORCE="${1#*=}" > > + if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then > > + LIBBPF_FORCE="$2" > > + shift > > + fi > > + if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then > > To avoid duplication here, I would move semantic checks into a second > step. This would allow for things like: > > | --libbpf_force=invalid --libbpf_force=on > > but separating the syntactic parsing from semantic checks might be > beneficial by itself, too. Yes, I agree with you. David, does this answer to your concern about v1? If yes, I would proceed with a v4 integrating Phil's suggestions. > > Cheers, Phil > [1] https://lore.kernel.org/netdev/cover.1633191885.git.aclaudi@redhat.com/
Hi Andrea, On Wed, Oct 06, 2021 at 11:49:34AM +0200, Andrea Claudi wrote: [...] > That was my first proposal in v1 [1]. I changed it on David's suggestion > to consolidate the two cases into a single one. Oh, sorry. I missed the 'v3' tag and hence didn't check any earlier version. > Looking at the resulting code, v3 code results in an extra check to > discriminate between the two use cases, while v0 uses the "case" > structure to the same end. Given that David explicitly requested the change I'm complaining about in his reply to your initial version, I guess any further debate about it is irrelevant. Thanks, Phil
On 10/6/21 4:18 AM, Phil Sutter wrote: > Hi Andrea, > > On Wed, Oct 06, 2021 at 11:49:34AM +0200, Andrea Claudi wrote: > [...] >> That was my first proposal in v1 [1]. I changed it on David's suggestion >> to consolidate the two cases into a single one. > > Oh, sorry. I missed the 'v3' tag and hence didn't check any earlier > version. > >> Looking at the resulting code, v3 code results in an extra check to >> discriminate between the two use cases, while v0 uses the "case" >> structure to the same end. > > Given that David explicitly requested the change I'm complaining about > in his reply to your initial version, I guess any further debate about > it is irrelevant. > I did not like the exploding duplication of checks on the value just to support '=' in the command line. Phil's suggestion seems reasonable.
diff --git a/configure b/configure index 7f4f3bd9..d57ce0f8 100755 --- a/configure +++ b/configure @@ -501,18 +501,30 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then else while true; do case "$1" in - --include_dir) - INCLUDE=$2 - shift 2 ;; - --libbpf_dir) - LIBBPF_DIR="$2" - shift 2 ;; - --libbpf_force) - if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then + --include_dir | --include_dir=*) + INCLUDE="${1#*=}" + if [ "$INCLUDE" == "--include_dir" ]; then + INCLUDE=$2 + shift + fi + shift ;; + --libbpf_dir | --libbpf_dir=*) + LIBBPF_DIR="${1#*=}" + if [ "$LIBBPF_DIR" == "--libbpf_dir" ]; then + LIBBPF_DIR="$2" + shift + fi + shift ;; + --libbpf_force | --libbpf_force=*) + LIBBPF_FORCE="${1#*=}" + if [ "$LIBBPF_FORCE" == "--libbpf_force" ]; then + LIBBPF_FORCE="$2" + shift + fi + if [ "$LIBBPF_FORCE" != 'on' ] && [ "$LIBBPF_FORCE" != 'off' ]; then usage 1 fi - LIBBPF_FORCE=$2 - shift 2 ;; + shift ;; -h | --help) usage 0 ;; "")
This commit makes it possible to specify values for configure params using the common autotools configure syntax '--param=value'. Signed-off-by: Andrea Claudi <aclaudi@redhat.com> --- configure | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)