Bug 283813 - Automate EXTRA_DIST consistency checking
Summary: Automate EXTRA_DIST consistency checking
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.7 SVN
Platform: Compiled Sources Linux
: NOR wishlist
Target Milestone: ---
Assignee: Bart Van Assche
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-11 18:35 UTC by Bart Van Assche
Modified: 2011-10-23 12:20 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Automate EXTRA_DIST consistency checking (1.83 KB, patch)
2011-10-11 18:35 UTC, Bart Van Assche
Details
Automate EXTRA_DIST consistency checking (2.40 KB, patch)
2011-10-12 16:28 UTC, Bart Van Assche
Details
Automate EXTRA_DIST consistency checking (2.94 KB, patch)
2011-10-13 18:30 UTC, Bart Van Assche
Details
Automate EXTRA_DIST consistency checking (3.32 KB, patch)
2011-10-16 19:52 UTC, Bart Van Assche
Details
Automate EXTRA_DIST consistency checking (3.74 KB, patch)
2011-10-19 16:34 UTC, Bart Van Assche
Details
Automate EXTRA_DIST consistency checking (4.34 KB, patch)
2011-10-20 16:25 UTC, Bart Van Assche
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bart Van Assche 2011-10-11 18:35:17 UTC
Created attachment 64429 [details]
Automate EXTRA_DIST consistency checking

Version:           3.7 SVN (using Devel) 
OS:                Linux

Valgrind's regression tests only work reliably in a tar ball generated with "make dist" if the regression test output files all have been specified in EXTRA_DIST in Makefile.am in the same directory. It is easy though to forget mentioning such a file. So it would be good to automate consistency checking of EXTRA_DIST with the actual regression test input files.

Reproducible: Didn't try


Actual Results:  
cachegrind/tests/x86/Makefile.am:1: error: insn_sse2.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-both.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-both.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-both.vgtest is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.vgtest is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.vgtest is missing in EXTRA_DIST
helgrind/tests/Makefile.am:1: error: tc22_exit_w_lock.stderr.exp-kfail-x86 is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: capget.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: syscalls-2007.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: syslog-syscall.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: timerfd-syscall.vgtest is missing in EXTRA_DIST
none/tests/Makefile.am:1: error: empty-exe.stderr.exp is missing in EXTRA_DIST
none/tests/Makefile.am:1: error: empty-exe.vgtest is missing in EXTRA_DIST

Expected Results:  
.
Comment 1 Bart Van Assche 2011-10-11 19:10:38 UTC
Note: the output format has been chosen such that it matches that of gcc - most editors should be able to parse that output.
Comment 2 Philippe Waroquiers 2011-10-11 19:54:49 UTC
The script looks good to me, and found quite a few missing files.

Some minor comments:

* Usage of /bin/bash
  Wouldn't it be better to use /bin/sh ?
  (except 2 other bash and one zsh, all others scripts are /bin/sh).

* I do not understand the reason for the below line:
     if grep -qw addsuffix $m; then continue; fi
  It looks like this will e.g. skip processing none/tests/amd64/Makefile.am
  (and several other Makefile.am containing addsuffix)
  Unclear why the *.vgtest and *.exp are not checked for these Makefile.am
  If needed to skip them, would be good to explain why.
 
* would be good to check the *.gdb files (these are gdb input files for gdbserver_tests).

* Wouldn't it be better to do this check at the end of regtest
  rather than at the beginning as these "failures" will not be visible
  in the summary of the failing tests but the output of the script will
  very probably not on the screen anymore.
  You might alsoexit the script with an error status if one or more files
  are not found.
Comment 3 Bart Van Assche 2011-10-12 16:28:47 UTC
Created attachment 64467 [details]
Automate EXTRA_DIST consistency checking

Version two of the attached patch addresses comments (1), (2) and (3). I'm not sure (4) is still relevant for version two of the patch since "make" will now stop if tests/check_tool_makefile finds any inconsistencies and exits with a non-zero exit status.
Comment 4 Philippe Waroquiers 2011-10-12 20:22:56 UTC
(In reply to comment #3)
> Created an attachment (id=64467) [details]
> Automate EXTRA_DIST consistency checking
> 
> Version two of the attached patch addresses comments (1), (2) and (3). I'm not
For what concerns comment 2: reason looks clear now (I just had to look :).
I understand this addsuffix technique is a shortcut to avoid
having the .vgtest, .stdout.exp and .stderr.exp be given explicitely.
As for most tests, these are given explicitely, removing the usage of this
addsuffix would not be a big work from what I can see.

Alternatively, the verification could be done by having a new target in
the Makefile.am making a full list of all the files in EXTRA_DIST,
and comparing with the files listed with a find -name '*.vgtest' -o -name '*....'
This would work whatever the way files are put in EXTRA_DIST.

Finally on this subject, I do not understand what is the desired
way to "compute" EXTRA_DIST. The comment in none/tests/amd64/Makefile.am
about insn_sse3 tells that the files for this test are included in any case,
but similar files for e.g. insn_pclmulqdq are not included.
So, it is unclear to me if make dist is or not to be influenced by the platform
on which the configure is done.


> sure (4) is still relevant for version two of the patch since "make" will now
> stop if tests/check_tool_makefile finds any inconsistencies and exits with a
> non-zero exit status.
I would still prefer to have the check at the end of make regtest: regression tests
should preferrably not be "blocked/stopped" by one failure (even this kind of failure).
Comment 5 Bart Van Assche 2011-10-13 18:30:53 UTC
Created attachment 64490 [details]
Automate EXTRA_DIST consistency checking

Changes in v3 compared to v2:
- Moved EXTRA_DIST consistency checking after running regression tests.
- Added primitive Makefile parser such that $(addsuffix ...) is understood.
- New output:
callgrind/tests/Makefile.am:1: error: simwork-both.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-both.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-both.vgtest is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-branch.vgtest is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.stderr.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.stdout.exp is missing in EXTRA_DIST
callgrind/tests/Makefile.am:1: error: simwork-cache.vgtest is missing in EXTRA_DIST
helgrind/tests/Makefile.am:1: error: tc22_exit_w_lock.stderr.exp-kfail-x86 is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: capget.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: syscalls-2007.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: syslog-syscall.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: timerfd-syscall.vgtest is missing in EXTRA_DIST
memcheck/tests/linux/Makefile.am:1: error: capget is in EXTRA_DIST but doesn't exist
memcheck/tests/linux/Makefile.am:1: error: syscalls-2007 is in EXTRA_DIST but doesn't exist
memcheck/tests/linux/Makefile.am:1: error: syslog-syscall is in EXTRA_DIST but doesn't exist
memcheck/tests/linux/Makefile.am:1: error: timerfd-syscall is in EXTRA_DIST but doesn't exist
none/tests/amd64/Makefile.am:1: error: insn_pclmulqdq.stderr.exp is missing in EXTRA_DIST
none/tests/amd64/Makefile.am:1: error: insn_pclmulqdq.stdout.exp is missing in EXTRA_DIST
none/tests/amd64/Makefile.am:1: error: insn_pclmulqdq.vgtest is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse.stderr.exp is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse.stdout.exp is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse.vgtest is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse2.stderr.exp is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse2.stdout.exp is missing in EXTRA_DIST
none/tests/x86/Makefile.am:1: error: insn_sse2.vgtest is missing in EXTRA_DIST
none/tests/Makefile.am:1: error: empty-exe.stderr.exp is missing in EXTRA_DIST
none/tests/Makefile.am:1: error: empty-exe.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: cksm.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: cksm.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: cksm.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: clcl.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: clcl.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: clcl.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: fgx.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: fgx.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: fgx.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: mvcl.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: mvcl.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: mvcl.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: op_exception.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: op_exception.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: op_exception.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stck.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stck.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stck.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stcke.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stcke.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stcke.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stckf.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stckf.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stckf.vgtest is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stfle.stderr.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stfle.stdout.exp is missing in EXTRA_DIST
none/tests/s390x/Makefile.am:1: error: stfle.vgtest is missing in EXTRA_DIST
Comment 6 Philippe Waroquiers 2011-10-14 21:30:49 UTC
(In reply to comment #5)
> Changes in v3 compared to v2:
Nice solution.

The big 'one line' awk script can be made more readable
by inserting line feeds (this is ok inside single quote)
and also | characters at the end of a line means no \ is
needed at the end of the line.

So, the below is somewhat more readable:

parse_makefile() {
  cat "$1" |
   awk '/\\$/ { n=$0; sub("\\\\$", "", n);
                if (line != "") {
                   line = line " " n
                }
                else {
                   line=n
                }
              } 
         /[^\\]$/ { if (line != "") { print line; line="" }; print}' |
   awk '{ if (match($0, "^ *([A-Za-z_]*) *= *(.*)$", m)) {
             var[m[1]] = m[2]
          };
          for (v in var) { 
             gsub("\\$\\(" v "\\)", var[v])
          }; 
          while (match($0, "\\$\\( *addsuffix *([^,)]*), *([^)]*)\\)", args)) {
             split(args[1], suff); split(args[2], name); name_and_suff="";
             for (n in name) {
                for (s in suff) {
                   name_and_suff = name_and_suff " " name[n] suff[s]
                }
             };
             i=index($0, args[0]);
             $0 = substr($0, 1, i-1) name_and_suff substr($0, i + length(args[0])) 
           };
           print 
         }' |
   sed 's/\$(noinst_SCRIPTS)//'
}
Comment 7 Bart Van Assche 2011-10-16 19:52:15 UTC
Created attachment 64605 [details]
Automate EXTRA_DIST consistency checking

Reindented the awk code and fixed a bug in the awk line joining code (end-of-line backslash handling). Output for trunk r12161 after having run ./autogen.sh and ./configure:
$ tests/check_tool_makefile *; echo $?
0

Or, in other words: no inconsistencies detected.
Comment 8 Florian Krohm 2011-10-19 12:22:32 UTC
(In reply to comment #7)

I played with it a bit.

(1) We need gawk (mentioned here again only for completeness sake)
    awk may map to something not powerful enough

(2) Handle comments
    I had changed none/tests/s390x/Makefile.am according to this patch

Index: none/tests/s390x/Makefile.am
===================================================================
--- none/tests/s390x/Makefile.am	(revision 12162)
+++ none/tests/s390x/Makefile.am	(working copy)
@@ -16,12 +16,13 @@
 	$(addsuffix .stderr.exp,$(INSN_TESTS)) \
 	$(addsuffix .stdout.exp,$(INSN_TESTS)) \
 	$(addsuffix .vgtest,$(INSN_TESTS)) \
-	ex_sig.stdout.exp ex_sig.stderr.exp ex_sig.vgtest \
 	ex_clone.stdout.exp ex_clone.stderr.exp ex_clone.vgtest \
 	op00.stderr.exp1 op00.stderr.exp2 op00.vgtest \
 	test.h opcodes.h add.h  and.h  div.h  insert.h \
 	mul.h  or.h  sub.h  test.h  xor.h
 
+#	ex_sig.stdout.exp ex_sig.stderr.exp ex_sig.vgtest \
+

    and then invoked like so

        ./tests/check_tool_makefile none/tests/s390x

    There were no complaints. The commented out files should be reported missing

(3) In memcheck/tests there is a file named filter_memcheck which I forgot to
    add to dist_noinst_SCRIPTS in Makefile.am there. It would be good to catch
    that somehow. Or, if that is too complex, perhaps have a command line
    option which would list all files present in the directory that are not
    listed in EXTRA_DIST.  Well, not all files, we could ignore stuff ending
    in ~ and the like. 

What do you think?

   Florian
Comment 9 Bart Van Assche 2011-10-19 16:34:59 UTC
Created attachment 64707 [details]
Automate EXTRA_DIST consistency checking

Ported from GNU awk to POSIX awk and made sure that commented out filenames are handled correctly. Does this help ?
Comment 10 Florian Krohm 2011-10-19 18:00:29 UTC
(In reply to comment #9)

> Ported from GNU awk to POSIX awk and made sure that commented out filenames are
> handled correctly. Does this help ?

Yes, it certainly does. Thanks! Are you planning to do something about (3) from comment #7? I think that would be a useful feature. For me anyhow :) 

One minor thing. The name "check_tool_makefile" is a bit misleading. What you're really doing is to check the makefiles in the regression buckets. So perhaps "check_regtest_makefile" would be a better name.
Comment 11 Bart Van Assche 2011-10-19 18:13:56 UTC
(In reply to comment #10)
> Are you planning to do something about (3) from
> comment #7? I think that would be a useful feature. For me anyhow :) 

The file patterns this script currently looks for are: *.exp* *.gdb *.vgtest. How about adding the pattern "filter*" to this list ?

> One minor thing. The name "check_tool_makefile" is a bit misleading. What
> you're really doing is to check the makefiles in the regression buckets. So
> perhaps "check_regtest_makefile" would be a better name.

Actually the script does even more - it currently checks all EXTRA_DIST parameters in all makefiles passed to it. In a clean source tree I get this:
$ tests/check_tool_makefile Makefile.am
Makefile.am:1: error: valgrind.spec is in EXTRA_DIST but doesn't exist
Comment 12 Bart Van Assche 2011-10-20 16:25:41 UTC
Created attachment 64738 [details]
Automate EXTRA_DIST consistency checking

Renamed script from check_tool_makefile into check_makefile_consistency and added consistency checks for the filter* scripts.
Comment 13 Bart Van Assche 2011-10-23 12:20:51 UTC
Applied v6 as r12214.