Re: [COMMITTED PATCH] Fix fallout from Joseph's untested Makeconfig change.

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Fri, 28 Feb 2014, Roland McGrath wrote:

> As others have noticed, Joseph's change was not properly tested and it
> broke the build.  Reviewers should have caught the issue, but moreover they
> should have pushed back on any structural change to the build system
> without clear testimony of definitely adequate testing.  It's clear that
> Joseph never tested a clean build from scratch.  Frankly, that's the bare
> minimum of testing for a change like this.

I tested a clean build from scratch as usual (it's incremental builds I 
don't generally test), on x86_64, including a full testsuite run, as well 
as doing the comparison of installed binaries before and after the patch.  
I retested a build from scratch just now for glibc including my patch and 
not your fix, and again it built cleanly.

> I was such a reviewer after the fact and I too am guilty of failing to
> catch this.  But I think it's important to point out that Joseph committed
> this change before I responded, when, in fact, nobody who credibly should
> know all the ins and outs of the makefiles had replied.  I think we have a
> general problem if the notion of "review by consensus" is being interpreted
> to mean that a positive reply from any contributor whatsoever constitutes
> adequate review.  Perhaps Ondrej did not mean to assert that his review was
> wholly sufficient for this change, but Joseph seems to have taken it that
> way.  Ondrej has made many fine contributions, but to me it is quite clear

That's why I waited for Brooks to comment, since Brooks is the main person 
to show any interest in this series of patches towards PASS/FAIL test 
results summaries.

> This covers the cases that broke the build and the similar ones that stood
> out to me just looking quickly.  Now the onus is on Joseph to do a thorough
> audit of the uses of variables in Makeconfig to make sure that all of them
> are compatible with the new inclusion order.  If Joseph doesn't testify
> that he has done this audit (and submit changes for any more fallout)
> within a week, then I'll revert the Makeconfig change.  (Of course, if
> anybody else wants to take up the slack for Joseph, that is great.  It's
> certainly not the case that Joseph doesn't do enough around here!)

I did the audit (for variables using += in Makeconfig, as those seemed to 
be the cases where things could have worked with a late include of 
Makeconfig but not an early one) when Stefan Liebler pointed out the 
problem.  That's how I identified the other directories with similarly 
affected includes of before-compile in 
<https://sourceware.org/ml/libc-alpha/2014-02/msg00771.html> - albeit that 
those settings of before-compile weren't in a position for which my patch 
made any difference.  I now see this audit missed files included by 
Makeconfig (config.make configparms $(sysdep-makeconfigs) sysd-sorted 
soversions.mk); on the basis that configparms has no business defining 
anything for which this could be relevant, I don't see any affected 
variables in those files either.

Your patch changes settings of generated and generated-dirs, but I don't 
see those as relevant at all, since they aren't set in Makeconfig 
(although generally I think use of += is to be preferred unless it's clear 
that a particular setting is the first or only setting of a variable, or 
that it's intended to override previous settings).

-- 
Joseph S. Myers
joseph@codesourcery.com