Re: [lmi] PATCH: C++20 and clang

[Greg Chicares]

On 4/20/21 12:58 PM, Vadim Zeitlin wrote:
> On Tue, 20 Apr 2021 02:19:03 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> > GC> I did some fixups to make 'make $coefficiency check_concinnity'
> GC> > GC> succeed.
> GC> > 
> GC> >  Oops, really sorry about this, I should have rerun it. But I don't
> GC> > understand how did the CI builds pass, they're supposed to check for
> GC> > this... oh, now I see: the problem was indeed detected, see
> GC> > 
> GC> > 
> https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25

All I see is a circled "✓" next to "Check concinnity", which is not
clickable. Just to be sure, I paste the URL back here from my browser:

https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25

and it seems to match yours. The 'uMatrix' add-in blocks
"collector.githubapp.com", which it colors with the crimson shade
it uses only for identified evil like "doubleclick.net", so maybe
some evil script is required.

> GC> > but apparently "make check_concinnity" doesn't exit with error even 
> when it
> GC> > does detect problems. This is really unexpected and counter-intuitive.
> GC> > Would you consider accepting patches changing this?
> GC> 
> GC> I'm loth to touch it,
> 
>  FWIW touching it would involve exactly one character change:
> 
> ---------------------------------- >8 --------------------------------------
> diff --git a/GNUmakefile b/GNUmakefile
[...]
> -       @-cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *
> +       @cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *
[...]
> I have no idea what is this "-" doing here

A long time ago, that wasn't the last command in the recipe,
so I imagine I wrote '-' to prevent it from stopping before
running all commands. But now an error on this line is the
last possible error in the recipe.

>  Are you still reluctant to remove it?

I'll remove it if you confirm that this one-character change does
exactly what you want. But I don't think it can. To test it, I
removed that character locally, and inserted an obvious violation...

/opt/lmi/src/lmi[0]$make $coefficiency check_concinnity && printf 'success???\n'
make[1]: Entering directory '/opt/lmi/src/lmi'
make[2]: 'test_coding_rules.exe' is up to date.
make[1]: Leaving directory '/opt/lmi/src/lmi'
  Problems detected by xmllint:
  Miscellaneous problems:
File 'irc7702.hpp' contains unlabelled '#endif' directive.
File 'irc7702.hpp' lacks canonical header guards.
      689 source files
   195138 source lines
      284 marked defects
success???
/opt/lmi/src/lmi[0]$ wine test_coding_rules * && printf 'success???\n'
File 'irc7702.hpp' contains unlabelled '#endif' directive.
File 'irc7702.hpp' lacks canonical header guards.
      689 source files
   195138 source lines
      284 marked defects
success???
/opt/lmi/src/lmi[0]$

Thus, it reports success when it actually detected failure.

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

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

There, '`anything` || true' is true. Another example:

        @find $(prefascicle_dir) -maxdepth 1 -executable -type f \
          -not -name '*.sh' -not -name '*.sed' \
          -not -name 'commit-msg' \
          -not -name 'pre-commit' \
          -not -name 'post-checkout' \
          | $(SED) -e's/^/Improperly executable: /'

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

> GC> and AFAICT there's no need to touch it because ./hooks/pre-commit
> GC> already does what you want.
> 
>  Just FYI, but I've had to disable it, as well as commit-msg hook, locally
> because they don't deal with some less usual (although by no means exotic,
> e.g. just cancelling editing the commit message is enough to confuse the
> latter script) situations. I know that I love to complain, but these
> scripts are really quite user-unfriendly.

They're friendly to me, i.e., in all the ways I use them
(except, as noted previously, when the 'test_coding_rules'
binary needs to be rebuilt). If you list ways to cause them
to behave in an unfriendly way, maybe we can fix each one.

When I cancel a commit, this happens:

/opt/lmi/src/lmi[1]$git commit --all
checking [toplevel]...hooks...okay
checking commit message...okay
Aborting commit due to empty commit message.

That looks friendly to me; does it not, to you? Or are
we cancelling in different ways? Me, I do this:
  gg
  Shift-V
  Shift-G
  x
  :wq
Do you use a different technique?

>  I'm also hesitant to run the hook directly -- which is, if I understand
> you correctly, what you propose -- because Git hooks are supposed to only
> be run by Git and not manually and because outputting "COMMIT ABORTED" in
> the build logs would be confusing.

Then perhaps the hook should be refactored:
 - everything but "COMMIT ABORTED" becomes a new, normal, non-hook script
 - the hook runs that script, and prints "COMMIT ABORTED" on failure

>  All in all, making "make check_concinnity" work as expected seems like
> such an obviously better idea that it's very difficult for me to understand
> why are you so loth to change it, even if it's perfectly clear that you
> are.

I believe it'll take a lot of work to rewrite this the
way you would like; and then it might not do what I want.
We've often discussed this dilemma, which permits no
middle ground: when a script finds something amiss, should
it terminate immediately and return nonzero, or should it
continue in order to find what else may be amiss?

I'm happy to consider concrete ideas, like the following:

> GC> The hook does have one problem, which I haven't addressed yet
> GC> because, while I'm somewhat less loth to touch that one, it
> GC> doesn't seem trivial or fun. The problem is that if the
> GC> 'test_coding_rules' binary needs to be rebuilt, the hook
> GC> reports failure even when it will succeed once rebuilt,
> GC> because rebuilding it spews out rebuilding commands that the
> GC> hook does not expect. I'd be glad if you want to patch that.
> 
>  I don't really want to (in part because the whole rebuilding thing doesn't
> work for me when using autotools anyhow), but this seems easy enough to 
fix
> by just invoking "make ... custom_tools" first and ignoring any messages
> from it, before the existing make command.

Tested and committed. Thanks. Perhaps I can be excessively
loth to touch certain things, sometimes, as a cat that's
been bitten by a snake is afraid of a rope.

> GC> Well...I deliberately try not to read those explanations, because
> GC> skimming them sometimes makes me fear that a ponderous change
> GC> is being proposed, when applying the changes shows it to be small.
> 
>  I'm really sorry, this definitely wasn't the intended effect and I'll stop
> writing frightening amounts of explanations for any further changes like
> this in the future. However I was still planing to write a cover letter 
for
> the std::filesystem patch, would this be superfluous or, worse,
> counterproductive, as well, or do you think that this could be useful for a
> bigger change like this one?

It will be useful. Getting rid of boost is important enough
to brave significant peril.