RE: [RFA-v2] Add scripts to generate ARI web pages to gdb/contrib/ari directory

Pierre Muller
2012-05-27T19:53:31Z

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Sergio Durigan Junior
> Envoyé : dimanche 27 mai 2012 06:06
> À : Pierre Muller
> Cc : 'Jan Kratochvil'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] Add scripts to generate ARI web pages to
> gdb/contrib/ari directory
>
> Hi Pierre,
>
> On Saturday, May 26 2012, Pierre Muller wrote:
>
> >> The patch is corrupted by line wrapping, 48 lines and some are not
> trivial
> >> to recover.
> > Sorry,
> > I hope the attached patch will apply correctly.
>
> It applies, but it creates a new directoy named src/ari, instead of
> src/gdb/contrib/ari. Not sure if it was intended, but it doesn't work
> out of the box as I was expecting. See below.

This is strange indeed, but it seems to depend on the patch executable
that you use,
I just discovered, while trying to install my patch on a freebsd
machine, that on that system patch uses '-p0' option instead of '-p 0'
on Linux... Maybe you hit a similar problem.

> > Concerning the new create-web-ari-in-src.sh,
> > this is indeed a new script (hence the 2012 copyright only)
> > and it is just a way to be able to generate the ARI index.html web
> > page without any parameters.
> >
> > It basically only give default parameters
> > to update-web-ari.sh script, which requires four parameters.
> >
> > I hope this clarifies some of your questions.
>
> Yes, it does, thank you.
>
> > Concerning Sergio's suggestion to separate out the awk script into
> > a separate file, I would like to minimize the changes relative to the
> > existing ss
> > cvs repository files.
>
> Hm, do you mean that you prefer to postpone this separation, or that you
> don't intend to do it at all?
>
> If the latter, I still think it's valid to do it because it will improve
> the readability of the code, IMO. But of course I won't push it if you
> don't intend to do it.

I am not against the idea of separating it out, but I would like to
be careful to avoid problems if we later decide that the scripts
should be run in a build directory (especially if we call a regenerated
script
in the build dir using the source dir awk script...)

> > About the use of dirname, I think that
> > direname is like basename part of
> > coreutils, and basename is already use several times
> > inside update-web-ari script in ss.
>
> Yes, I agree, my only concern is that maybe some obscure system won't
> have some of these binaries (like `awk', for example).

Of course generating the Awk Regression Index web page
without an installed awk binary will be difficult :)

> > I agree that being made public and thus available to
> > many users, it would be nice to chack availability, and add a
workaround,
> > but I have no
> > precise how to do it, probably using a configure or Makefile could help
> here
> > .
>
> I was thinking more about a check in the shell script itself, no need
> for Makefiles or configure options.

OK.

> > Note that gdb directory cvonfigure script seems to contain both
> > dirname and basename...
>
> Yeah, good point. Maybe I'm being too paranoid.
>
> > I hope you will be able to generate a ARI web page,
> > and give more feedbacks,
>
> I wasn't able to generate the webpage easily. I had to do several fixes
> in the scripts. I am sending a new version of the patch which should
> apply cleanly and create the proper directory under src/gdb/contrib.

Could you tell us on which system you tried?

> I have taken the liberty to fix several errors that were not allowing me
> to generate the web page correctly. In order to test it, I was using
> the following command:
>
> /bin/sh update-web-ari.sh ~/work/src/git/gdb-src /tmp/create-ari
> /tmp/webdir-ari gdb
>
> i.e.,
>
> /bin/sh update-web-ari.sh <SRCDIR> <TMPDIR> <WEBDIR> <PROJECTNAME>
>
> Note that I did not set the executable bit in any of the scripts below.
> I have chosen to leave them as regular files, just like you did in your
> patch.

The patch generated by
cvs diff -u -p -N
doesn't contain any filemode information,
I don't know if this is possible with CVS directly,
but the files do have the executable permission set for user,
because this was suggested earlier in the emails.

To discusss your changes more easily, I
generated a diff between my version and yours,
I will comment on this diffs below.

>>diff -u -p -r ari/create-web-ari-in-src.sh
sergio-ari/create-web-ari-in-src.sh
>>--- ari/create-web-ari-in-src.sh 2012-05-27 12:01:02.000000000
+0200
>>+++ sergio-ari/create-web-ari-in-src.sh 2012-05-27 11:59:26.000000000
+0200
>>@@ -58,7 +58,7 @@ if [ -z "${webdir}" ] ; then
>> fi
>>
>> # Launch update-web-ari.sh in same directory as current script.
>>-${scriptpath}/update-web-ari.sh ${srcdir} ${tempdir} ${webdir} gdb
>>+/bin/sh ${scriptpath}/update-web-ari.sh ${srcdir} ${tempdir} ${webdir}
gdb
>>
>> if [ -f "${webdir}/index.html" ] ; then
>> echo "ARI output can be viewed in file \"${webdir}/index.html\""
This is not needed if we keep the exec permission for users
(or should it be for everyone?).

>>diff -u -p -r ari/gdb_ari.sh sergio-ari/gdb_ari.sh
>>--- ari/gdb_ari.sh 2012-05-27 12:01:02.000000000 +0200
>>+++ sergio-ari/gdb_ari.sh 2012-05-27 11:59:26.000000000 +0200
>>@@ -264,7 +264,7 @@ Do not use `Linux'\'', instead use `Linu
>> && !/(^|[^_[:alnum:]])Linux\[sic\]([^_[:alnum:]]|$)/ \
>> && !/(^|[^_[:alnum:]])GNU\/Linux([^_[:alnum:]]|$)/ \
>> && !/(^|[^_[:alnum:]])Linux kernel([^_[:alnum:]]|$)/ \
>>-&& !/(^|[^_[:alnum:]])Linux [:digit:]\.[:digit:]+)/ {
>>+&& !/(^|[^_[:alnum:]])Linux [[:digit:]]\.[[:digit:]]+)/ {
>> fail("GNU/Linux")
>> }
>>

This is one of the problems I saw, but wnted to fix only after
a first commit to have a minimal difference between ss and gdb/contrib/ari

>>diff -u -p -r ari/update-web-ari.sh sergio-ari/update-web-ari.sh
>>--- ari/update-web-ari.sh 2012-05-27 12:01:02.000000000 +0200
>>+++ sergio-ari/update-web-ari.sh 2012-05-27 11:59:26.000000000
+0200
>>@@ -85,6 +85,13 @@ else
>> AWK=gawk
>> fi
>>
>>+# Checking for `doschk' binary (if `check_doschk_p' is active)
>>+if [ "${check_doschk_p}" == "true" ] && doschk > /dev/null 2>&1
>>+then
>>+ have_doschk=true
>>+else
>>+ have_doschk=false
>>+fi
>>
>> # Set up a few cleanups
>> if ${delete_source_p}

I am not sure this is correct:
calling doschk without arguments
caused the program (if available)
to wait for input to analyze instead of analyzing files
given as parameters.
Was your intent to test if the binary is available?
I think we must do this without calling it.
Currently, id doschk binary doesn't exist,
line 302 of update-web-ari.sh will create an empty
doschk_out file (with some error probably generated by the shell)
but this is not a problem for me at least on cygwin or Linux

>>@@ -99,7 +106,7 @@ if [ -d ${snapshot} ]
>> then
>> module=${project}
>> srcdir=${snapshot}
>>- aridir=${srcdir}/${module}/ari
>>+ aridir=${srcdir}/${module}/contrib/ari
>> unpack_source_p=false
>> delete_source_p=false
>> version_in=${srcdir}/${module}/version.in
>>

This is indeed a required change that
I forgot, sorry about that one.

>>@@ -203,8 +210,8 @@ then
>> oldpruned=${oldf1}-pruned
>> newpruned=${newf1}-pruned
>>
>>- cp -f ${bugf} ${oldf}
>>- cp -f ${srcf} ${oldsrcf}
>>+ test -f ${bugf} && cp -f ${bugf} ${oldf}
>>+ test -f ${srcf} && cp -f ${srcf} ${oldsrcf}
>> rm -f ${srcf}
>> node=`uname -n`
>> echo "`date`: Using source lines ${srcf}" 1>&2

OK, I am still a bit weak on some
shell basics, this means
use 'cp' only if test -f ${file}
returns zero, i.e. if the file exists.
Is it really a problem to have a failing cp call here?

>>@@ -251,10 +258,7 @@ then
>>
>> fi
>>
>>-
>>-
>>-
>>-if ${check_doschk_p} && test -d "${srcdir}"
>>+if ${check_doschk_p} && test "${have_doschk}" == "true" && test -d
"${srcdir}"
>> then
>> echo "`date`: Checking for doschk" 1>&2
>> rm -f "${wwwdir}"/ari.doschk.*
Would
'if ${check_doschk_p} && ${have_doschk} && test -d ${srcdir}'
be also correct here?

>>@@ -744,7 +748,9 @@ END {
>> #print "tests/ok:", nr / ok
>> #print "bugs/tests:", bugs / nr
>> #print "bugs/ok:", bugs / ok
>>- print bugs / ( oks + legacy + deprecated )
>>+ if (oks + legacy + deprecated > 0) {
>>+ print bugs / ( oks + legacy + deprecated )
>>+ }
>> }
>> ' ${wwwdir}/ari.doc`
>>
This is nice, but once again, I would prefer to
add this only after an initial commit.

>>@@ -860,7 +866,9 @@ EOF
>> for a in $all; do
>> alls="$alls all[$a] = 1 ;"
>> done
>>- cat ari.*.doc | $AWK >> ${newari} '
>>+ if ls ${wwwdir}/ari.*.doc > /dev/null 2>&1
>>+ then
>>+ cat ${wwwdir}/ari.*.doc | $AWK >> ${newari} '
>> BEGIN {
>> FS = ":"
>> '"$alls"'
>>@@ -876,6 +884,7 @@ BEGIN {
>> }
>> }
>> '
>>+ fi
>>
>> cat >> ${newari} <<EOF
>>

Here also, your change improve the script quality,
and I would be glad to incorporate it later.

One more problem I encountered on FreeBSD
is that 'awk' and 'gawk' are not completely equivalent and
gawk seems to generate output while
freebsd awk fails...

Pierre