ARI/Commit rules

ARI/Commit rules

[Phil Muldoon]

I was writing a patch today, and I once again arrived at the: "Should I
use X or Y for this operation" dilemma.  There are a lot of unwritten
rules about programming usage in GDB (See ARI mails), and sometimes
maintainers don't catch them.  This is inevitable with a project this
size and age.  I have come to fear the post-commit ARI warnings ;)

This lead me to think, is it possible to distil this post-commit hook
script to a user installable script somewhere?  If GDB used git as the
primary repository we could just make it a pre-commit hook, and a user
script would not be needed.  But as GDB uses CVS I do not think
this is possible.

This would help ease the reviewing load.  We could put a link to the
script in the contributors guide, and a "RUN THIS FIRST" preamble.

Also we could perhaps encode some formatting rules in there as well.
Blank line after variable decl, two spaces after a period, space
in-between function and brackets, etc etc.  These nits always trip me up.

What do you think?

Cheers

Phil

Re: ARI/Commit rules

[Joel Brobecker]

> I was writing a patch today, and I once again arrived at the: "Should I
> use X or Y for this operation" dilemma.  There are a lot of unwritten
> rules about programming usage in GDB (See ARI mails), and sometimes
> maintainers don't catch them.  This is inevitable with a project this
> size and age.  I have come to fear the post-commit ARI warnings ;)

Yeah - the ARI is a useful tool (but, IMO, has to be taken with
a grain of salt), and it's a pity that the results always come after
the fact. Worse, from the WEB ARI results, I could see that some
files have warnings, but I couldn't find where exactly in the file
the warnings were.

I think it would be nice to be able to run this script by hand
from a checkout, and it shouldn't be too difficult, since it's
mostly an awk script.  And for those that use git, we could
produce a hook as well, I think.  We might have some issues
with volume (number of warnings due to the already-present
violations) though, or perhaps performance.  I don't know about
others, but since using git, I have been so spoiled that I expect
every operation to be instantaneous...

-- 
Joel

Re: ARI/Commit rules

[Phil Muldoon]

Joel Brobecker <brobecker@adacore.com> writes:

> I think it would be nice to be able to run this script by hand
> from a checkout, and it shouldn't be too difficult, since it's
> mostly an awk script.  And for those that use git, we could
> produce a hook as well, I think.  We might have some issues
> with volume (number of warnings due to the already-present
> violations) though, or perhaps performance.  I don't know about
> others, but since using git, I have been so spoiled that I expect
> every operation to be instantaneous...

It would be nice to fix those warnings eventually.  I believe a pre-commit
hook would only operate on your diffs, though.  Anyway we cannot use
that as the GIT repository is read only.  So to me, an ideal thing
would be just to run it on my files that I have changed, or even better
a patch I have generated.

What do you think?

Cheers

Phil

Re: ARI/Commit rules

[Joel Brobecker]

> It would be nice to fix those warnings eventually.  I believe a pre-commit
> hook would only operate on your diffs, though.  Anyway we cannot use
> that as the GIT repository is read only.  So to me, an ideal thing
> would be just to run it on my files that I have changed, or even better
> a patch I have generated.

There might be one technical issue with processing a diff rather
than the file (I think some regexp match only at the start of
a line), but yeah, that would be much better.

> What do you think?

I think it would be an improvement indeed.

-- 
Joel

Re: ARI/Commit rules

[Pedro Alves]

On Wednesday 15 June 2011 16:37:02, Phil Muldoon wrote:
> Joel Brobecker <brobecker@adacore.com> writes:
> 
> > I think it would be nice to be able to run this script by hand
> > from a checkout, and it shouldn't be too difficult, since it's
> > mostly an awk script.  And for those that use git, we could
> > produce a hook as well, I think.  We might have some issues
> > with volume (number of warnings due to the already-present
> > violations) though, or perhaps performance.  I don't know about
> > others, but since using git, I have been so spoiled that I expect
> > every operation to be instantaneous...
> 
> It would be nice to fix those warnings eventually.  I believe a pre-commit
> hook would only operate on your diffs, though.  Anyway we cannot use
> that as the GIT repository is read only.  So to me, an ideal thing
> would be just to run it on my files that I have changed, or even better
> a patch I have generated.

You can download the script from the ARI page:
<http://sourceware.org/gdb/ari/>.

Here's a direct link: <http://sourceware.org/gdb/ari/gdb_ari.sh>.

Something like `ls *.[hc] | xargs gdb_ari.sh` should work.

I actually don't know how to get to the repository 
where the script is hosted, and where to get the script
that is invoking the gdb_ari.sh.  I'm guessing its home
is at the same place the webpages are, and perhaps the logic
of which files to run the ari script on are only in some
cron script somewhere?

> 
> What do you think?
> 
> Cheers
> 
> Phil
> 

-- 
Pedro Alves

Re: ARI/Commit rules

[Joel Brobecker]

> I actually don't know how to get to the repository 
> where the script is hosted, and where to get the script
> that is invoking the gdb_ari.sh.  I'm guessing its home
> is at the same place the webpages are, and perhaps the logic
> of which files to run the ari script on are only in some
> cron script somewhere?

It's the 'ss' repository at sourceware.org, but you need to be
part of the gdbadmin group on this machine.

If there is interest, I think what we should do is move the gdb_ari.sh
script to the gdb repository, and I will adjust the nightly scripts
accordingly.

-- 
Joel

Re: ARI/Commit rules

[Pedro Alves]

On Wednesday 15 June 2011 17:47:43, Joel Brobecker wrote:
> > I actually don't know how to get to the repository 
> > where the script is hosted, and where to get the script
> > that is invoking the gdb_ari.sh.  I'm guessing its home
> > is at the same place the webpages are, and perhaps the logic
> > of which files to run the ari script on are only in some
> > cron script somewhere?
> 
> It's the 'ss' repository at sourceware.org, but you need to be
> part of the gdbadmin group on this machine.
> 
> If there is interest, I think what we should do is move the gdb_ari.sh
> script to the gdb repository, and I will adjust the nightly scripts
> accordingly.

Makes sense to me.  It'd be most useful if we brought along
most of the logic around the script invocation as
well, of course.

-- 
Pedro Alves

Re: ARI/Commit rules

[Alfred M. Szmidt]

   This lead me to think, is it possible to distil this post-commit
   hook script to a user installable script somewhere?  If GDB used
   git as the primary repository we could just make it a pre-commit
   hook, and a user script would not be needed.  But as GDB uses CVS I
   do not think this is possible.

It is possible, just warp the cvs command in a shell script, and
execute the pre-commit hook you wish to do.

RE: ARI/Commit rules

[Pierre Muller]

  Hi all,

  An important part of the job to generate 
the ARI post-commit messages to gdb-patches is done in
ss/update-web-ari
this script is not copied to http://sourceware.org/gdb/current/ari/
The relevant part is listed below.

  Each day, it copies ari.sources.bug to ari.sources.old
ari.sources.lines to ari.sources.lines-old
Runs update_ari script, and generates a diff of the two ARI lists
and looks for the corresponding source lines to generate the email message.

  It should be possible to restrict the script to changes files by 
replacing gdb_find by something that only lists the modified files.
After, you would need to restore the original versions to create the CVS
reference
ARI listing for those files, apply your patch,
  launch gdb_ari.sh script again
and compare the two output as done below...

  I am not bash expect and my AWK knowledge is rather limited, which
means that the modifications that I inserted to generate those emails
are probably far from being efficient and clean, but you are of course
welcome to propose enhancements,

Pierre Muller 
as ARI maintainer

PS: As to adding ARI rules about proper formatting, I have nothing
against the principle.
  I even already have a uncommitted gdb_ari.sh change that adds a "spaces"
category that looks for:
  - spaces at end of lines
  - tab/spaces mix
  - 8 or more spaces at line start.



> -----Message d'origine-----
> De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la part
> de Alfred M. Szmidt
> Envoyé : mercredi 15 juin 2011 20:03
> À : pmuldoon@redhat.com
> Cc : gdb@sourceware.org
> Objet : Re: ARI/Commit rules
> 
>    This lead me to think, is it possible to distil this post-commit
>    hook script to a user installable script somewhere?  If GDB used
>    git as the primary repository we could just make it a pre-commit
>    hook, and a user script would not be needed.  But as GDB uses CVS I
>    do not think this is possible.
> 
> It is possible, just warp the cvs command in a shell script, and
> execute the pre-commit hook you wish to do.



Relevant part of update-web-ari script:

if ${check_source_p} && test -d "${srcdir}"
then
    bugf=${wwwdir}/ari.source.bug
    oldf=${wwwdir}/ari.source.old
    srcf=${wwwdir}/ari.source.lines
    oldsrcf=${wwwdir}/ari.source.lines-old

    diff=${wwwdir}/ari.source.diff
    diffin=${diff}-in
    newf1=${bugf}1
    oldf1=${oldf}1
    oldpruned=${oldf1}-pruned
    newpruned=${newf1}-pruned

    cp -f ${bugf} ${oldf}
    cp -f ${srcf} ${oldsrcf}
    rm -f ${srcf}
    node=`uname -n`
    echo "`date`: Using source lines ${srcf}" 1>&2
    echo "`date`: Checking source code" 1>&2
    ( cd "${srcdir}" && /bin/sh $HOME/ss/gdb_find.sh "${project}" | \
        xargs /bin/sh $HOME/ss/gdb_ari.sh -Werror -Wall --print-idx
--src=${srcf}
    ) > ${bugf}
    # Remove things we are not interested in to signal by email
    # gdbarch changes are not important here
    # Also convert ` into ' to avoid command substitution in script below
    sed -e "/.*: gdbarch:.*/d" -e "s:\`:':g" ${oldf} > ${oldf1}
    sed -e "/.*: gdbarch:.*/d" -e "s:\`:':g" ${bugf} > ${newf1}
    # Remove line number info so that code inclusion/deletion
    # has no impact on the result
    sed -e "s/\([^:]*\):\([^:]*\):\(.*\)/\1:0:\3/" ${oldf1} > ${oldpruned}
    sed -e "s/\([^:]*\):\([^:]*\):\(.*\)/\1:0:\3/" ${newf1} > ${newpruned}
    # Use diff without option to get normal diff output that
    # is reparsed after
    diff ${oldpruned} ${newpruned} > ${diffin}
    # Only keep new warnings
    sed -n -e "/^>.*/p" ${diffin} > ${diff}
    sedscript=${wwwdir}/sedscript
    script=${wwwdir}/script
    sed -n -e "s|\(^[0-9,]*\)a\(.*\)|echo \1a\2 \n \
        sed -n \'\2s:\\\\(.*\\\\):> \\\\1:p\' ${newf1}|p" \
        -e "s|\(^[0-9,]*\)d\(.*\)|echo \1d\2\n \
        sed -n \'\1s:\\\\(.*\\\\):< \\\\1:p\' ${oldf1}|p" \
        -e "s|\(^[0-9,]*\)c\(.*\)|echo \1c\2\n \
        sed -n \'\1s:\\\\(.*\\\\):< \\\\1:p\' ${oldf1} \n \
        sed -n \"\2s:\\\\(.*\\\\):> \\\\1:p\" ${newf1}|p" \
        ${diffin} > ${sedscript}
    ${SHELL} ${sedscript} > ${wwwdir}/message
    sed -n \
        -e "s;\(.*\);echo \\\"\1\\\";p" \
        -e "s;.*< \([^:]*\):\([0-9]*\):.*;grep \"^\1:\2:\" ${oldsrcf};p" \
        -e "s;.*> \([^:]*\):\([0-9]*\):.*;grep \"^\1:\2:\" ${srcf};p" \
        ${wwwdir}/message > ${script}
    ${SHELL} ${script} > ${wwwdir}/mail-message
    if [ "x${branch}" != "x" ]; then
        email_suffix="`date` in ${branch}"
    else
        email_suffix="`date`"
    fi

    if [ "${node}" = "sourceware.org" ]; then
        warning_email=gdb-patches@sourceware.org
    else
        warning_email=muller@sourceware.org
    fi

    # Check if ${diff} is not empty
    if [ -s ${diff} ]; then
        # Send an email to muller@sourceware.org
        mutt -s "New ARI warning ${email_suffix}" \
            ${warning_email} < ${wwwdir}/mail-message
    else
      if [ -s ${wwwdir}/${mail-message} ]; then
        # Send an email to muller@sourceware.org
        mutt -s "ARI warning list change ${email_suffix}" \
            muller@sourceware.org < ${wwwdir}/mail-message
      fi
    fi
fi

>>>>>>>>>>>>>>>>>>>>>>>>
Index: gdb_ari.sh
===================================================================
RCS file: /cvs/gdbadmin/ss/gdb_ari.sh,v
retrieving revision 1.110
diff -u -p -r1.110 gdb_ari.sh
--- gdb_ari.sh  30 May 2011 19:55:37 -0000      1.110
+++ gdb_ari.sh  16 Jun 2011 11:42:22 -0000
@@ -20,7 +20,7 @@ LC_ALL=c ; export LC_ALL
 error="regression"
 warning="regression"
 ari="regression eol code comment deprecated legacy obsolete gettext"
-all="regression eol code comment deprecated legacy obsolete gettext
deprecate i
nternal gdbarch macro"
+all="regression eol code comment deprecated legacy obsolete gettext
deprecate i
nternal gdbarch macro spaces"
 print_doc=0
 print_idx=0

@@ -110,6 +110,7 @@ done

 awk -- '
 BEGIN {
+    DEBUG=1
     # NOTE, for a per-file begin use "FNR == 1".
     '"${aris}"'
     '"${errors}"'
@@ -118,6 +119,19 @@ BEGIN {
     print_doc =  '$print_doc'
     print_idx =  '$print_idx'
     PWD = "'`pwd`'"
+    _old_RS = RS
+    RS="@@"
+    "cat list-headers" | getline ALL_HEADERS
+    "cat list-nat" | getline ALL_NAT
+    "cat list-tdep" | getline ALL_TDEP
+    "cat list-common" | getline ALL_COMMON
+    RS=_old_RS
+    if (DEBUG == 1) {
+      print "ALL_HEADERS = " ALL_HEADERS
+      print "ALL_NAT = " ALL_NAT
+      print "ALL_TDEP = " ALL_TDEP
+      print "ALL_COMMON = " ALL_COMMON
+    }
 }

 # Print the error message for BUG.  Append SUPLEMENT if non-empty.
@@ -189,6 +203,14 @@ FNR == 1 {
     else {
       is_yacc_or_lex = 0
     }
+    if ( match (ALL_NAT, FILENAME) ) {
+      if (DEBUG == 1) {
+       print "File " FILENAME " is native"
+      }
+      is_native = 1
+    } else {
+      is_native = 0
+    }
 }
 END {
     if (print_idx) {
@@ -254,6 +276,34 @@ Do not use ARGSUSED, unnecessary"
     fail("ARGSUSED")
 }

+BEGIN { doc["trailing spaces"] = "\
+Lines should not have trailing spaces"
+    category["trailing spaces"] = ari_spaces
+}
+/[[:space:]]+$/ {
+    fail("trailing spaces")
+}
+
+BEGIN { doc["missing tabs"] = "\
+Lines should never have more than 7 spaces"
+    category["missing tabs"] = ari_spaces
+}
+/^ {8,}/ {
+    fail("missing tabs")
+}
+
+BEGIN { doc["tab-space mix"] = "\
+Tabs and spaces should not be mixed"
+    category["tab-space mix"] = ari_spaces
+}
+/^ +\t+/ {
+    fail("tab-space mix")
+}
+/^\t+ +\t/ {
+    fail("tab-space mix")
+}
+
+

 # SNIP - Strip out comments - SNIP