RFR: JDK-8065576: Enable pipefail in the shell used by make to better detect build errors
erik.joelsson at oracle.com
Mon Dec 1 13:27:10 UTC 2014
New webrev, which addresses Magnus' concern below, and various fixes in
Hotspot for incompatibilities with errexit.
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.
On 2014-11-27 00:21, Magnus Ihse Bursie wrote:
> On 2014-11-26 15:56, Erik Joelsson wrote:
>> 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.
More information about the build-dev