This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Sunday, May 27 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.

What I did was just `quilt import /path/to/ari.patch' and `quilt push',
both on top of src/ directory (not src/gdb).

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

I cannot see why separating the scripts could lead to any problems in
this case you mention.  My idea was just to make separated .awk files
which would be called using `$AWK -f script1.awk...' and so on.

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

Fedora 16 x86_64.

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

Thanks.

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

Well, I agree it's not needed if `update-web-ari.sh' has the executable
permission set for the user.  However, I think it's not harmful to leave
the `/bin/sh' there, since the file is interpreted anyway.

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

Oh, OK, as I said, I just fixed those problems because I saw those
warnings and they bothered me.  I can easily send a patch to fix those
minor issues later, when you have the code checked in.

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

Sorry, I did not know `doschk' waited for input, I don't have it
installed here and I should have investigated more.  My intention was to
check for its existence, yeah.  I think this check can be replaced by
something like:

    if which doschk > /dev/null 2>&1
    then
        ....
    fi

I was just trying to avoid another annoying warning that I was seeing
while generating the webpage.

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

AFAIR this is the main reason I couldn't generate the webpage at first
attempt.

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

Yes, the test means exactly that.

It is not a strict problem if `cp' fails, but again, I think useless
warning messages should be avoided for the sake of clarity of the
output.  The more useless warning we generate, the less attention the
user will pay to the really important messages.

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

Not with the code I wrote to check `doschk', because it sets
`${have_doschk}' anyway (to either `true' or `false').  You would have
to change the code to:

    if which doschk > /dev/null 2>&1
    then
        have_doschk=true
    fi

(i.e., remove the `else' command).  If you do that, you will be able to
use the `if' as you proposed, because `${have_doschk}' will only be set
when `doschk' exists.

However, I am not used to see this kind of construction in shell
scripts.  Maybe this can be another point of future improvement in the patch.

>>>@@ -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
>>> <center>
>  
>   Here also, your change improve the script quality,
> and I would be glad to incorporate it later.

Great.  Thanks for the comments; as I said earlier, I can easily send a
patch after you have checked in the first version of ARI.

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

Maybe we'll have to stick to `gawk' then?  This is one of the reasons I
proposed a check for the necessary binaries.

Thanks,

-- 
Sergio


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]