From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22791 invoked by alias); 27 May 2012 22:03:12 -0000 Received: (qmail 22780 invoked by uid 22791); 27 May 2012 22:03:10 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 27 May 2012 22:02:52 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4RM2lPo019687 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 27 May 2012 18:02:47 -0400 Received: from psique (ovpn-113-148.phx2.redhat.com [10.3.113.148]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4RM2iIo016494; Sun, 27 May 2012 18:02:45 -0400 From: Sergio Durigan Junior To: "Pierre Muller" Cc: "'Jan Kratochvil'" , Subject: Re: [RFA-v2] Add scripts to generate ARI web pages to gdb/contrib/ari directory References: <001201cd3547$377188b0$a6549a10$@muller@ics-cnrs.unistra.fr> <20120525194654.GA30472@host2.jankratochvil.net> <003c01cd3b3c$d85e07d0$891a1770$@muller@ics-cnrs.unistra.fr> <002101cd3c42$5473b670$fd5b2350$@muller@ics-cnrs.unistra.fr> X-URL: http://www.redhat.com Date: Sun, 27 May 2012 22:03:00 -0000 In-Reply-To: <002101cd3c42$5473b670$fd5b2350$@muller@ics-cnrs.unistra.fr> (Pierre Muller's message of "Sun, 27 May 2012 21:53:02 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-05/txt/msg00984.txt.bz2 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} <>>
> > 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