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
