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
