Re: [RFA] Make contrib/ari/gdb_find.sh script more configurable

Re: [RFA] Make contrib/ari/gdb_find.sh script more configurable

[Pedro Alves]

On 12/11/2012 01:01 PM, Pierre Muller wrote:
>     As we were talking about adding 
> gdbserver to the list of directories that should
> also be inspected by Awk regression scripts,
> I propose hereby a patch allowing to include directory
> gdbtk, gdbserver or gnulib to the list of inspected directories
> by simply exporting a variable named check_XXX_dir
> before running the scripts.

IMO, if you're going to support something like this, I think
it'd make more sense to make it a single variable that accepts
a list of dirs, like:

ARI_CHECK_EXTRA_DIRS=gdbtk,gdbserver,gnulib,whatnot

or the CLI version --check-extra-dirs=gdbtk,gdbserver,gnulib,whatnot
or some such.

From reading the patch, I find no real need to hardcode the set
of possible dirs.

-- 
Pedro Alves

[RFA] Make contrib/ari/gdb_find.sh script more configurable

[Pierre Muller]

    As we were talking about adding 
gdbserver to the list of directories that should
also be inspected by Awk regression scripts,
I propose hereby a patch allowing to include directory
gdbtk, gdbserver or gnulib to the list of inspected directories
by simply exporting a variable named check_XXX_dir
before running the scripts.

  Does this look OK to you?

  Is the ChangeLog entry OK?



Pierre Muller
as ARI maintainer


2012-12-11  Pierre Muller  <muller@sourceware.org>

        * contrib/ari/gdb_find.sh (add_pruned_directory): New function.
        (check_gdbtk_dir, check_gdbserver_dir, check_gnulib_dir): Add
        test for presence of variables to conditionally prune corresponding
        directory.


Index: contrib/ari/gdb_find.sh
===================================================================
RCS file: /cvs/src/src/gdb/contrib/ari/gdb_find.sh,v
retrieving revision 1.5
diff -u -p -r1.5 gdb_find.sh
--- contrib/ari/gdb_find.sh     7 Dec 2012 02:57:49 -0000       1.5
+++ contrib/ari/gdb_find.sh     11 Dec 2012 12:54:56 -0000
@@ -29,11 +29,38 @@ LC_ALL=C ; export LC_ALL
 # A find that prunes files that GDB users shouldn't be interested in.
 # Use sort to order files alphabetically.

-find "$@" \
-    -name testsuite -prune -o \
-    -name gdbserver -prune -o \
-    -name gdbtk -prune -o \
-    -name gnulib -prune -o \
+find_args=
+
+add_pruned_directory ()
+{
+  pruned_dir=$1
+  if [ ! -z "$verbose" ] ; then
+    echo "Adding directory \"$pruned_dir\" to list of pruned directories"
+  fi
+  findargs="$findargs -name $pruned_dir -prune -o "
+}
+
+# Setting environment variables
+# check_gdbtk_dir, check_gdbserver_dir and check_gnulib_dir
+# to non empty will trigger the addition of the files contained
+# in directory gdbtk and gdbserver respectively.
+# By default, these directory are not listed.
+
+if [ -z "$check_gdbtk_dir" ] ; then
+  add_pruned_directory gdbtk
+fi
+
+if [ -z "$check_gdbserver_dir" ] ; then
+  add_pruned_directory gdbserver
+fi
+
+if [ -z "$check_gnulib_dir" ] ; then
+  add_pruned_directory gnulib
+fi
+
+add_pruned_directory testsuite
+
+find "$@" $findargs \
     -name '*-stub.c' -prune -o \
     -name '*-exp.c' -prune -o \
     -name ada-lex.c -prune -o \

Re: [RFA] Make contrib/ari/gdb_find.sh script more configurable

[Joel Brobecker]

>     As we were talking about adding gdbserver to the list of
>     directories that should also be inspected by Awk regression
>     scripts, I propose hereby a patch allowing to include directory
>     gdbtk, gdbserver or gnulib to the list of inspected directories by
>     simply exporting a variable named check_XXX_dir before running the
>     scripts.

I am personally not very fond of environment variables in general,
but I see that the script passes all arguments straight through to
the find command, so we can't add new switches...

I do note, however, that the script is really only ever called with
one argument in practice, so we could simplify the problem if we
wanted to.

Just my 2 cents:
  - for gdbserver, I assume we will make it non-optional at some point.
  - For gnulib, I think it is pointless. I do not see why we'd start
    generating ARI info for some source code that we do not control.
  - for gdbtk, why not, although I don't know that the gdbtk is
    very active beyond minimal maintenance...

> 2012-12-11  Pierre Muller  <muller@sourceware.org>
> 
>         * contrib/ari/gdb_find.sh (add_pruned_directory): New function.
>         (check_gdbtk_dir, check_gdbserver_dir, check_gnulib_dir): Add
>         test for presence of variables to conditionally prune corresponding
>         directory.

I think the entry presents the environment variable names as entities
in your code. I don't know how to present changes to the "main" of
a script. I'd probably write a free-txt description of the changes
and see if I get away with it.

        * contrib/ari/gdb_find.sh: Check the "check_gdbtk_dir",
        "check_gdbserver_dir", and "check_gnulib_dir" environment
        variables to determine which directories should get pruned.

Or something like that.

-- 
Joel

PS: One of the issues I have with the current scripts is that they are
    pretty abstract. They were most likely written to handle several
    projects, with GDB being just one of them. Now that the scripts
    are inside the GDB repository, I'd enjoy some simplications in
    that department (Eg: delete variable "project", and just inline
    "gdb" everywhere - it seems like it would make the code a little
    easier to understand).

Re: [RFA] Make contrib/ari/gdb_find.sh script more configurable

[Pedro Alves]

On 12/12/2012 12:05 PM, Joel Brobecker wrote:
> I am personally not very fond of environment variables in general,

+1000.  We want reproducible setups.  Command line switches end up in
logs, while environment variables usually not.  Issues with
rogue environment variables are harder to debug than explicit command
line switches.

-- 
Pedro Alves