RFR: 8011350 : hgforesth.sh fails if sh is not bash

[Mike Duigou]

On Apr 3 2013, at 21:17 , David Holmes wrote:

> On 4/04/2013 11:45 AM, Tim Bell wrote:
>> Looks good to me, Mike
>> 
>> Thanks - a seemingly simple change inflated into a small project when
>> tested on multiple O/S platforms.
> 
> Indeed. A main take away from this exercise is that any changes to scripts need extensive cross-platform testing before they are pushed - including at a minimum a RE Solaris build machine.

I certainly agree. I was entirely unaware of the diversity of bad behaviour possible in this case. I had tested on only linux and macos believing that for the changes being made this was sufficient. From this experience and prior "lessons" I learned that the shell scripting environment is significantly different on each of:

linux (certainly not homogeneous)
macos
macos + macports
cygwin (every version is a different minefield)
solaris 10u6
solaris 10u9 (much less buggy toolchain and bash from my understanding)
solaris 11

One difficulty here is that the failure in hgforest.sh would not have been found by a standard JPRT test. I can and will test future script changes against as much of this list as possible and will look at the recommended configurations documented in the README-build for guidance as to what platform

It's important to say though that the moral of the story shouldn't be "don't try". The issue has come up with reviews of hgforest.sh, webrev, and other build changes that we don't have adequate reviews, testing procedures, shepherding, mentoring or, unfortunately, any leadership. Perhaps a champion will step forward to lead us boldly on, but in the meantime I think extra effort is required by the build team members (and interested interlopers) to go the extra mile in reviewing changesets and ask more questions about what testing has been done and suggest additional testing wherever appropriate.

Mike

> David
> 
>> Tim
>> 
>> On 04/03/13 16:30, Mike Duigou wrote:
>>> I've received some feedback from Tim Bell and David Katleman.
>>> 
>>> Here's an updated version of the patch.
>>> 
>>> http://cr.openjdk.java.net/~mduigou/JDK-8011350/2
>>> 
>>> It turns out the real shell incompatibility was more straightforward.
>>> The outright conversion to bash isn't necessary. Replacement of '=='
>>> with '=' and double to single quotes in the cut command are sufficient.
>>> 
>>> The whichhg changes are based on an observation by Tim Bell in another
>>> bug that some versions of Solaris "/usr/bin/which" don't generate an
>>> exit code and instead output the message "no foo in PATH". This change
>>> better handles no mercurial being present.
>>> 
>>> Mike
>>> 
>>> On Apr 3 2013, at 08:56 , Mike Duigou wrote:
>>> 
>>>> An alternative has been suggested: convert the hgforest.sh script to
>>>> a bash script. I have tested this alternative on unbuntu linux 11.04,
>>>> solaris 10u9, MacOS 10.7 and cygwin 1.7.17. This seems like less risk
>>>> and there doesn't seem to be a compelling reason to stick with
>>>> classic sh.
>>>> 
>>>> I have prepared an alternate webrev here:
>>>> 
>>>> http://cr.openjdk.java.net/~mduigou/JDK-8011350/1
>>>> 
>>>> We could still consider the original webrev if using bash turns out
>>>> to have unexpected issues.
>>>> 
>>>> Mike
>>>> 
>>>> On Apr 2 2013, at 20:03 , Mike Duigou wrote:
>>>> 
>>>>> Hello all;
>>>>> 
>>>>> Further testing on JDK-8011342 revealed that hgforest.sh can fail if
>>>>> the sh shell is not bash. The problem appears to be due to mixing of
>>>>> -o -a and ! in [] test expressions.
>>>>> 
>>>>> I have prepared a webrev here:
>>>>> 
>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8011350/0/webrev/common/bin/hgforest.sh.udiff.html
>>>>> 
>>>>> 
>>>>> This converts all of the potentially problematic [ expr -o expr ] [
>>>>> expr -a expr ] and [ expr -{o|a} ! expr ] to use "test". My
>>>>> conversions are based on the advice of the autotools chapter on
>>>>> "Writing portable Bourne Shell"
>>>>> (http://sourceware.org/autobook/autobook/autobook_208.html#SEC208)
>>>>> for avoiding potential problems.
>>>>> 
>>>>> The other option is just to require bash which is already required
>>>>> by the new build process.
>>>>> 
>>>>> Mike
>> 
>>