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".
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.
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.

/Erik
Thanks,
David

/Erik

Reply via email to