Re: [MERGE] build-test: fixed err detection and added multi-test support

Alex Rousskov wrote:
> On 03/10/2009 02:45 AM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> On 03/09/2009 04:01 PM, Amos Jeffries wrote:
>>>> Target string is logged by the master make file _after_ all make
>>>> targets
>>>> are completed successfully. Anything else is a bug, either in code
>>>> or in
>>>> TestBed.
>>>>
>>> This is not true, on several layers:
>>>
>>> 1) The string is logged when all-am target is done. The check target,
>>> among others, is build _after_ all-am. You can grep raw Makefile for
>>> all-am to see the dependencies.
>> Not by the testbed. It runs checks first to catch the dependencies,
>> then all-am last to be sure of the basic build.
> It does not matter what the testbed script tries to do in this case. The
> facts are that the overloaded all-am target in root Makefile is:
>
> 1) not always the last target evaluated by make
> 2) sometimes evaluated when previous targets have failed
>
> This should be enough to see the design flaw, IMO. You can argue that
> you have carefully scripted testbed's make calls to minimize the harmful
> effects of that flaw, but they are not completely eliminated, require
> constant attention as the script and Makefiles are updated, and the flaw
> is still there. Why not just fix the flaw? That is what the proposed
> patch does.
>
>>> 2) The string is logged when make fails if make is told to ignore
>>> errors. This does not happen in the build-test context (by default), but
>>> often happens during developement, when I ran "make -k" to get more
>>> errors than one. It is rather confusing to see the "Build Successful"
>>> message when there are hundreds of errors above it.
>> Would you not define a successful run when make finishes all its tasks
>> as requested to do so?
>
> The successful make execution is the one that finished all its tasks
> successfully (the all-am target is just one of the tasks). You can
> detect a successful run by examining the return code. That is why we
> (including the testbed's test-suite/buildtest.sh script!) often write
>
> make foo && make bar
>
> instead of writing
>
> make foo;
> if grep magic $log
> then
> make bar
> fi
>
> The latter is pretty much what the current test-builds.sh script
> attempts to do, and it is known to cause troubles at least for some of
> us, is more complex, less reliable, yet we are still arguing whether we
> should work harder on implementing that idea correctly!
>
>>> 3) "Make" is designed to exit with an error when there is an error. We
>>> should not be re-implementing that logic. The problems with the current
>>> script and the above caveats are all cases in point.
>>>
>>> Overall, it seems strange to me to reject the change that uses the
>>> correct interface to detect build errors while the current code is using
>>> a half-broken hack.
>> I agree we should not use a hack when there is a legit method of
>> detecting all and any errors. I' just recalling past history of the
>> development of the testbed when this exact approach left some errors
>> uncaught.
> Why did it leave some errors uncaught? Could it be due to all other bugs
> in the testbed? After checking script revision history, I suspect that
> was exactly the case:
>
> r9080: Correct overall approach, but ignores top-level errors.
> r9133: Correct overall approach, quits on the first top-level error.
> r9181: Correct overall approach, but incorrectly added error reporting
> breaks it.
> ... error reporting improves, but the bug is not noticed ...
> r9345: Correct overall approach killed in favor of a buggy "grep
> error" design.
> r9376: A "grep success" check added, in hope to fix "grep error"
> design bugs:
>
> "append success message to build so testbed can catch it and find
> any error without having to enumerate all failure cases."
>
> The unnecessary fight to detect more and more errors stemmed from an
> incorrect addition of error reporting in r9181. The correct approach
> probably worked, but was broken by that addition, and is now blamed for
> problems caused by the error reporting and then error detection code :-(.
>
> Alex.
>

I was hoping to catch you on IRC, but yesterday I applied your merge and
ran a bunch of tests for the the things which stood out most in my
memory as problems I expected from it.

Several of the tests I remember the testbed being unable to catch work
under your match. There are two that stand out though:

1) adding "choke me" as syntax error in config.h

The changes you make do not pass the exit value from buildtest.sh to
test-builds.sh. Everything I have read on portable shell result passing
is contrary to this working.
Yet it does. Any ideas why?

(yes explicitly passing the result value back will 'fix' your patch if
it turns out to be an issue on some system.)

2) adding ChokeMe to the translated languages list.

Results in the "Internal error" case with exit 0.

This occurs because the (optional and manually maintained) errors grep
locates po2html displaying fatal warnings in the log. Make itself is
happy, the po2html tool is broken regarding our needed exit values.

Something to think about is how many of the app tools do we use in
Makefiles and makefile helper scripts, don't return exit 0/1 the way we
need?

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE13
   Current Beta Squid 3.1.0.6