RFR: JDK-8065576: Enable pipefail in the shell used by make to better detect build errors
tim.bell at oracle.com
Mon Dec 1 15:38:46 UTC 2014
> 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.
> 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