RFR: JDK-8065576: Enable pipefail in the shell used by make to better detect build errors

[Erik Joelsson]

Hello,

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

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