RFR: JDK-8158535: Configure script uses basic tools directly in many places

[David Holmes]

On 7/06/2016 9:04 PM, Erik Joelsson wrote:
> Hello,
>
> On 2016-06-06 01:36, David Holmes wrote:
>> Hi Erik,
>>
>> On 4/06/2016 1:56 AM, Erik Joelsson wrote:
>>> In the configure script we check for the presence of several basic
>>> tools, like grep, sed, echo etc, and assign variables for them. In the
>>> makefiles we are very diligent with always using the tools through these
>>> variables to make sure we have a well functioning version of the tool at
>>> hand. In many places in the configure script, we have been sloppy with
>>> this.
>>>
>>> I recently stumbled over this because we change the PATH variable in
>>> parts of the script (typically when looking for the toolchain) which can
>>> then potentially change which instance of the tool that is being run
>>> unless the resolved variables are used. This patch fixes all the
>>> instances that I could find.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8158535
>>> Webrev: http://cr.openjdk.java.net/~erikj/8158535/webrev.top.01/
>>
>> Two questions:
>>
>> 1. How do we know a $XXX variable actually exists for a given command?
> Well, we would have declared it somewhere so it's manual inspection.
> It's most commonly done through "BASIC_REQUIRE_PROGS".

I guess it also helps to know which commands are shell built-ins and 
which are external commands.

>> 2. How do we know it has been set when modifying the .m4 files? I
>> presume there are some parts of at least one .m4 file that can't use
>> $XXX because it won't have been set yet.
>>
> That is true, but for the most basic tools, we set them up very early.
> There is BASIC_INIT and then BASIC_SETUP_FUNDAMENTAL_TOOLS. It should be
> very rare to edit the logic in BASIC_INIT though. For more complex
> tools, there is more of a careful ordering. In this patch I'm pretty
> sure I only changed basic tools however.

OK - so basics.m4 is the only place you may need to be careful.

>> I also noticed on Windows that there are still inconsistencies. For
>> example we use $ECHO in places, but I also see:
>>
>>  99     new_path=`cmd /c "for %A in (\"$input_path\") do @echo
>> %~sA"|$TR \\\\\\\\ / | $TR ...
>>
> This particular line is executing echo inside cmd, so it actually needs
> to be just "echo". It is however possible that I missed something in
> this patch. I started out by hunting down the ones I had an issue with
> in my particular setup that triggered this change. Then I greped for
> more instances of the same.

Okay on the cmd use of echo - a little confusing though, especially when 
$TR is used in the same expression.

Thanks,
David

> /Erik
>> Thanks,
>> David
>>
>>> /Erik
>