Erik:
New webrev, which addresses Magnus' concern below, and various fixes
in Hotspot for incompatibilities with errexit.
Webrev: http://cr.openjdk.java.net/~erikj/8065576/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8065576
Looks good to me.
/Tim
For the LOG=trace issue Magnus described below, it was enough to just
replace $$(BASH) with $$(SHELL) and things worked as expected for me.
In Hotspot there were several instances of run command X, followed by
"if test $? = 0" (or something similar). I changed this to "if X;
then" as errexit ignores commands that are part of if-statement tests.
Since this is now touching Hotspot makefiles, it will need to go in
through a Hotspot group forest.
/Erik
On 2014-11-27 00:21, Magnus Ihse Bursie wrote:
On 2014-11-26 15:56, Erik Joelsson wrote:
Hello,
Please review this build reliability fix. In JDK-8065138, we would
have caught the error much faster if the build had failed instead of
silently generating bad output. To avoid this in the future, this
patch activates pipefail and errexit in the shell, when available.
This means that long command lines, consisting of multiple commands,
chained together either by pipes or ';', will fail the build
regardless of which of the sub commands that failed. Currently, all
but the last command would be ignored.
Since these features my not always be available in all versions of
bash, I added a check to configure for each of them and only enable
them if they are available. I also had to fix some instances where
we have 'grep' and 'find' returning non zero without it being an error.
Thanks to Martin who suggested this in the first place.
Thank you for fixing this so quickly!
The webrev looks good as such, but I think you have missed how this
interact with common/bin/shell-tracer.sh. Which, unfortunately, is
non-trivial. :-(
First of all, I realize that the existing line in MakeBase:
WRAPPER_SHELL:=$$(BASH) $$(SRC_ROOT)/common/bin/shell-tracer.sh
$$(if $$(findstring yes,$$(IS_GNU_TIME)),$$(TIME),-)
$$(OUTPUT_ROOT)/build-trace-time.log $$(BASH)
is incorrect. (It's my bad...) The last $$(BASH) is supposed to be
the old value of $(SHELL), according to shell-tracer.sh. So, even
before your fix, it should have been $$(SHELL), not $$(BASH).
However, even if you fix that, I'm not sure how this will work with a
$(SHELL) that has spaces in it. I think you can get it working it by
sending "$$(SHELL)" as last argument in WRAPPER_SHELL, and updating
shell-tracer.sh, so that the assignment to OLD_SHELL keeps the ""
around $3, but the calls to "$OLD_SHELL" (note, two places) removes
the "". But you'll have to verify that.
/Magnus
