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

[Tim Bell]

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
>