Re: maint.mk syntax check problems

[Jim Meyering]

Martin von Gagern wrote:
> I recently wrote a mail with various remarks about how maint.mk syntax
> checks give false positives, and some suggestions to avoid these. Bruno
> Haible was kind enough to voice an opinion on items 2 and 3 of that
> list, but I have seen no reply to any of the other problems.
>
> And I'm still interested in some feedback what you think about turning
> those syntax checks into a shell script file instead of embedding so
> much ugly backslash-continued shell code into the makefile.
>
> Hoping for some more reaction,
>  Martin von Gagern
>
> On 05.09.2011 14:16, Martin von Gagern wrote:
>> Hi!
>>
>> I'm currently updating GNU wdiff to use latest gnulib, 2c53fc42. In the
>> process, I've encountered a number of problems with maint.mk syntax checks.
>>
>>
>> 1. main.mk fails its own checks:
>>
>> The checks sc_makefile_at_at_check and sc_prohibit_undesirable_word_seq
>> both fail for me, as the maint.mk file itself violates these checks.
>>
>> I know, this will probably only affect projects which have gnulib
>> included in their own repository, but according to the manual
>> <http://www.gnu.org/software/gnulib/manual/gnulib.html#VCS-Issues> that
>> approach is officially supported, so the checks should deal with it.

Most projects do not version-control maint.mk.
IMHO, the cost of obfuscating the strings in maint.mk (in comments)
that trigger warnings would be too high, rendering the comments
significantly harder to read.  If you must version-control a copy of
maint.mk, you may still exempt it from its own checks by setting a
couple of variables in cfg.mk.

>> 2. sc_prohibit_undesirable_word_seq and gettext:
...
>>
>> 4. Obscure sc_tight_scope:
>>
>> The syntax check printed a message indicating that it skipped the
>> tight_scope check. I still don't know what this check is about, but
>> reading the code I found that it sometimes sets a variable called
>> "fail", but that variable doesn't appear to affect the result in any

There are a few comments after "# TS-start".
The bottom line is that you should get a diagnostic/failure
about variables in your project that are accidentally declared
with extern scope.

>> way. Other syntax checks have "test $$fail = 1" as a condition
>> somewhere, but tight_scope does not. So no matter what the test is
>> supposed do, I'd gues it currently doesn't do it.

Thanks.  That was a bug.
Fixed by the patch below.

>> 5. sc_prohibit_always-defined_macros reports missing files:
>>
>> The sc_prohibit_always-defined_macros check will cause error messages
>> about missing files to be emitted if elements from the gl_other_headers_
>> list are not present (i.e. not imported). This can be confusing. I've
>> added a "test -e $$f" check to the def_sym_regex code:
>> https://github.com/gagern/gnulib/commit/74524a2c993a809bbc20dcc3
>> Feel free to merge this.

gl_other_headers_ is defined like this:

  gl_other_headers_ ?= \
    intprops.h  \
    openat.h    \
    stat-macros.h

The "?=" assignment means that you can override it via cfg.mk.
Does that work for you?

>> 6. sc_po_check and generated files:
>>
>> If gnulib has its own po-base, then $(generated_files) should not be
>> included in the list of files to check, as the main project POTFILES.in
>> isn't responsible for these. Checking whether the po-base specific file
>> $(srcdir)/lib/po/POTFILES.in exists might be a good way to discern these
>> scenarios.

Again, the default generated_files definition in maint.mk may be
overridden by a project-specific definition, say in cfg.mk.

>> On the whole, I wonder whether it would be better to factor all of these
>> syntax checks from the makefile into a separate shell script file. After
>> all, most of them are largely composed of shell syntax snippets already.
>> And a shell script file would get rid of a HUGE number of line
>> continuation backslashes. It would probably also make things like output
>> coloring easier, I believe, as you could nest functions more easily.
>> What do you think?

It might not be worth the effort/disruption.
One advantage of using Makefile rules is that it's easy to override
the defaults, as you see in the examples above.



>From e2c54cb6998369800c2a18df71bb4d0253e472ec Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 14 Sep 2011 14:39:35 +0200
Subject: [PATCH] maint.mk: sc_tight_scope: propagate failure from sub-make

* top/maint.mk (sc_tight_scope): Actually initialize and use $fail.
Reported by Martin von Gagern.
---
 ChangeLog    |    6 ++++++
 top/maint.mk |    8 +++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 46c6251..bab5ba2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2011-09-14  Jim Meyering  <address@hidden>
+
+       maint.mk: sc_tight_scope: propagate failure from sub-make
+       * top/maint.mk (sc_tight_scope): Actually initialize and use $fail.
+       Reported by Martin von Gagern.
+
 2011-09-13  Bruno Haible  <address@hidden>

        tempname: Support for MSVC.
diff --git a/top/maint.mk b/top/maint.mk
index c9ffb95..bbb67ec 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -1398,7 +1398,8 @@ _gl_TS_dir ?= src

 ALL_RECURSIVE_TARGETS += sc_tight_scope
 sc_tight_scope: tight-scope.mk
-       @if ! grep '^ *export _gl_TS_headers *=' $(srcdir)/cfg.mk       \
+       @fail=0;                                                        \
+       if ! grep '^ *export _gl_TS_headers *=' $(srcdir)/cfg.mk        \
                > /dev/null                                             \
           && ! grep -w noinst_HEADERS $(srcdir)/$(_gl_TS_dir)/Makefile.am \
                > /dev/null 2>&1; then                                  \
@@ -1410,8 +1411,9 @@ sc_tight_scope: tight-scope.mk
                -f $(abs_top_builddir)/$<                               \
              _gl_tight_scope                                           \
                || fail=1;                                              \
-       fi
-       @rm -f $<
+       fi;                                                             \
+       rm -f $<;                                                       \
+       exit $$fail

 tight-scope.mk: $(ME)
        @rm -f $@ address@hidden
--
1.7.7.rc0.362.g5a14