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
