Re: [lmi] Attuning concinnity checks (was: PATCH: C++20 and clang)

[Vadim Zeitlin]

On Tue, 20 Apr 2021 20:41:42 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 4/20/21 12:58 PM, Vadim Zeitlin wrote:
GC> > On Tue, 20 Apr 2021 02:19:03 +0000 Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> [...]
GC> > GC> > GC> I did some fixups to make 'make $coefficiency check_concinnity'
GC> > GC> > GC> succeed.
GC> > GC> > 
GC> > GC> >  Oops, really sorry about this, I should have rerun it. But I don't
GC> > GC> > understand how did the CI builds pass, they're supposed to check for
GC> > GC> > this... oh, now I see: the problem was indeed detected, see
GC> > GC> > 
GC> > GC> > 
https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25
GC> 
GC> All I see is a circled "✓" next to "Check concinnity", which is not
GC> clickable. Just to be sure, I paste the URL back here from my browser:
GC> 
GC> 
https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25
GC> 
GC> and it seems to match yours. The 'uMatrix' add-in blocks
GC> "collector.githubapp.com", which it colors with the crimson shade
GC> it uses only for identified evil like "doubleclick.net", so maybe
GC> some evil script is required.

 To be frank, I don't care if it's evil (but I don't think it is), as long
as it only runs on this site and, of course, the big advantage of uMatrix
compared to the previous generation content-blockers is that it has
per-site settings. So I do enable it on this site and I think it's
perfectly safe for you to do the same. If you're still reluctant to do it,
you could visit this page in a separate profile/incognito mode/Firefox
container depending on whatever you prefer.

 You can also retrieve the raw logs from

https://github.com/let-me-illustrate/lmi/commit/e1ffd97febe8b220d585176805bb4182ee3ad754/checks/2382487360/logs

(accessible from the "gear" popup menu on the page above, but it probably
doesn't work without JS neither), but they're not very readable and I can't
link to the exact line in them, unfortunately.


GC> > GC> > but apparently "make check_concinnity" doesn't exit with error even 
when it
GC> > GC> > does detect problems. This is really unexpected and 
counter-intuitive.
GC> > GC> > Would you consider accepting patches changing this?
GC> > GC> 
GC> > GC> I'm loth to touch it,
GC> > 
GC> >  FWIW touching it would involve exactly one character change:
GC> > 
GC> > ---------------------------------- >8 
--------------------------------------
GC> > diff --git a/GNUmakefile b/GNUmakefile
GC> [...]
GC> > -       @-cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *
GC> > +       @cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *
GC> [...]
GC> > I have no idea what is this "-" doing here
GC> 
GC> A long time ago, that wasn't the last command in the recipe,
GC> so I imagine I wrote '-' to prevent it from stopping before
GC> running all commands. But now an error on this line is the
GC> last possible error in the recipe.

 Just to be clear, this "-" was always there, long before the leading "cd"
was added, please see the commit referenced in the previous message.

GC> >  Are you still reluctant to remove it?
GC> 
GC> I'll remove it if you confirm that this one-character change does
GC> exactly what you want. But I don't think it can. To test it, I
GC> removed that character locally, and inserted an obvious violation...
GC> 
GC> /opt/lmi/src/lmi[0]$make $coefficiency check_concinnity && printf 
'success???\n'
GC> make[1]: Entering directory '/opt/lmi/src/lmi'
GC> make[2]: 'test_coding_rules.exe' is up to date.
GC> make[1]: Leaving directory '/opt/lmi/src/lmi'
GC>   Problems detected by xmllint:
GC>   Miscellaneous problems:
GC> File 'irc7702.hpp' contains unlabelled '#endif' directive.
GC> File 'irc7702.hpp' lacks canonical header guards.
GC>       689 source files
GC>    195138 source lines
GC>       284 marked defects
GC> success???
GC> /opt/lmi/src/lmi[0]$ wine test_coding_rules * && printf 'success???\n'
GC> File 'irc7702.hpp' contains unlabelled '#endif' directive.
GC> File 'irc7702.hpp' lacks canonical header guards.
GC>       689 source files
GC>    195138 source lines
GC>       284 marked defects
GC> success???
GC> /opt/lmi/src/lmi[0]$
GC> 
GC> Thus, it reports success when it actually detected failure.

 Sorry, I've seen "error_flag" handling in test_coding_rules.cpp and
naively assumed that this already worked. One day I'll really learn to
check my assumptions, even the most plausible ones, but this day hasn't
come yet, apparently. The minimal fix I can propose is this:

---------------------------------- >8 --------------------------------------
diff --git a/test_coding_rules.cpp b/test_coding_rules.cpp
index 950b7290f..aa93f89ff 100644
--- a/test_coding_rules.cpp
+++ b/test_coding_rules.cpp
@@ -263,8 +263,11 @@ bool file::phyloanalyze(std::string const& s) const
     return boost::regex_search(leaf_name(), boost::regex(s));
 }

+bool error_flag = false;
+
 void complain(file const& f, std::string const& complaint)
 {
+    error_flag = true;
     std::cout << "File '" << f.full_name() << "' " << complaint << std::endl;
 }

@@ -1298,7 +1301,6 @@ statistics process_file(std::string const& file_path)

 int try_main(int argc, char* argv[])
 {
-    bool error_flag = false;
     statistics z;
     for(int j = 1; j < argc; ++j)
         {
---------------------------------- >8 --------------------------------------

and this time I did check that it worked.

GC> And even if we somehow make that last command return nonzero,
GC> other commands cannot do so without modification:

 Removing this "-" would still be better than nothing because in practice
close to 100% of the problems I introduce are detected by the coding rules
test.

GC>     @cd $(prefascicle_dir) && [ -f md5sums ] \
GC>       && <md5sums $(SED) 
-e'/\.test$$\|\.test0$$\|\.test1$$\|\.tsv$$\|\.xml$$/d' \
GC>       | $(MD5SUM) --check --quiet || true

 Of course, if it were up to me, I'd remove this "|| true". I don't think
we really gain anything by continuing to run the other checks if this one
fails: it's still fatal and it's easy to fix and quick to rerun. It's also
much simpler to understand what the first error actually is if you see only
it and not it and potentially a lot of other output.

GC> There, '`anything` || true' is true. Another example:
GC> 
GC>     @find $(prefascicle_dir) -maxdepth 1 -executable -type f \
GC>       -not -name '*.sh' -not -name '*.sed' \
GC>       -not -name 'commit-msg' \
GC>       -not -name 'pre-commit' \
GC>       -not -name 'post-checkout' \
GC>       | $(SED) -e's/^/Improperly executable: /'

 Here we could just do something like

        @bad_exec_files=$(find ...same arguments...); \
        if [ -n "$bad_exec_files" ]; then \
                echo "The following files are improperly executable: 
$bad_exec_files"; \
                exit 1; \
        fi

Alternatively, using "find ... -print -exec /bin/false" could work too, but
it might be too cryptic.


GC> In this and other cases, and so in general, "success" means
GC> emitting no unexpected output, and anything else is failure.

 I understand, I just believe it's wrong. I'd be willing to bet that this
is fragile and that you've already had to adjust the output filters because
new versions of something resulted in new "expected" output and that if it
somehow hasn't happened yet, it will in the future. Exit status is part of
the program contract and doesn't change (or at least shouldn't without any
really good reason), while its output is not. Relying on it is also
gratuitously unusual, just as appending "|| true" to the commands in make
recipe -- sure, you can do it, but this is not any person familiar with
make (or with Unix shell in the case of testing the output rather than the
exit code) would expect to happen.

GC> > GC> and AFAICT there's no need to touch it because ./hooks/pre-commit
GC> > GC> already does what you want.
GC> > 
GC> >  Just FYI, but I've had to disable it, as well as commit-msg hook, locally
GC> > because they don't deal with some less usual (although by no means exotic,
GC> > e.g. just cancelling editing the commit message is enough to confuse the
GC> > latter script) situations. I know that I love to complain, but these
GC> > scripts are really quite user-unfriendly.
GC> 
GC> They're friendly to me, i.e., in all the ways I use them
GC> (except, as noted previously, when the 'test_coding_rules'
GC> binary needs to be rebuilt). If you list ways to cause them
GC> to behave in an unfriendly way, maybe we can fix each one.

 I'll try to make a complete list, but there are many issues, mostly
related to the fact that sometimes lines do exceed 72 characters and it's
better to leave them be.

 But at the very least, the hook must ignore commented out lines inserted
by e.g. git-rebase (which I use very often), that are automatically
generated and often longer than the limit, but shouldn't count.

GC> When I cancel a commit, this happens:
GC> 
GC> /opt/lmi/src/lmi[1]$git commit --all
GC> checking [toplevel]...hooks...okay
GC> checking commit message...okay
GC> Aborting commit due to empty commit message.
GC> 
GC> That looks friendly to me; does it not, to you? Or are
GC> we cancelling in different ways? Me, I do this:
GC>   gg
GC>   Shift-V
GC>   Shift-G
GC>   x
GC>   :wq
GC> Do you use a different technique?

 Yes, I just use ":q!" which results in spurious error messages.

GC> >  I'm also hesitant to run the hook directly -- which is, if I understand
GC> > you correctly, what you propose -- because Git hooks are supposed to only
GC> > be run by Git and not manually and because outputting "COMMIT ABORTED" in
GC> > the build logs would be confusing.
GC> 
GC> Then perhaps the hook should be refactored:
GC>  - everything but "COMMIT ABORTED" becomes a new, normal, non-hook script
GC>  - the hook runs that script, and prints "COMMIT ABORTED" on failure

 Yes, this would be the second best solution if "make check_concinnity"
can't be modified to do the right thing directly. But this almost seems
like more work and, frankly, doesn't it seem just a tad weird to have a
script whose only purpose is to invoke a make target which is not useful on
its own? If we do it like this, I would say that this script should just
run test_coding_rules itself and "make check_concinnity" should be removed,
what is the point of having both of them?


GC> >  All in all, making "make check_concinnity" work as expected seems like
GC> > such an obviously better idea that it's very difficult for me to
GC> > understand why are you so loth to change it, even if it's perfectly
GC> > clear that you are.
GC> 
GC> I believe it'll take a lot of work to rewrite this the
GC> way you would like; and then it might not do what I want.
GC> We've often discussed this dilemma, which permits no
GC> middle ground: when a script finds something amiss, should
GC> it terminate immediately and return nonzero, or should it
GC> continue in order to find what else may be amiss?

 This is a secondary point of divergence, but I'm willing to admit that I
could be wrong about this one, and, at the very least, I do understand your
point of view. But the primary point is that all command line tools,
including make targets (which are, of course, nothing but mini-tools using
make as their command interpreter), must have a useful exit status and I'm
quite sure that I'm not wrong about this and have trouble understanding the
opposite point of view.

 To be more constructive, it would be unusual, but possible to output all
problems at once in "make check_concinnity", rather than stopping on the
first one and I'm willing to do (and test) it. However, the more I think
about it, the more I believe that the best would really be to have a shell
script (check_concinnity.sh?) doing this rather than a make target at all,
as it would be much simpler and more readable to do it there. What do you
think about this proposal?

 Thanks,
VZ

pgp2hfan_ol4L.pgp
Description: PGP signature