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

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.
Received on Tue Mar 10 2009 - 17:48:53 MDT