This certainly needs to be looked at. Thanks for bringing it up. Your
assessment of the situation looks correct to me.
/Erik
On 2013-03-04 05:59, David Holmes wrote:
On 27/02/2013 10:14 PM, Jim Laskey (Oracle) wrote:
I wanted to double check and trace the origins of the MakeBase.gmk
patch before I responded. It was part of the original set Erik sent
me, but looking thru the other parts of the patch it's not clear why
it was necessary.
http://cr.openjdk.java.net/~erikj/nashorn-build/webrev.01/
So I got to the bottom of this. Basically if a make variable expands
to contain a filename for a nested class (ie one with $ in it) then
you need to escape the $ so that make doesn't treat it as a
meta-character. And because $ is also the shell meta-character you end
up needing a double escape of the form \$$$$, depending on how that
variables gets used in targets/pre-reqs/recipes.
Prior to the nashorn change ListPathsSafely didn't do any $ escaping
and any explicit reference to a nested class file, such as:
sun/awt/motif/X11GB2312\$$$$Decoder.class
in the makefile, had to use the \$$$$. And everything worked fine.
What nashorn introduced was .java files with $ in their name:
> dir ../nashorn/src/jdk/nashorn/internal/scripts/
total 16
drwxr-xr-x 2 daholme uucp 4096 Feb 26 01:26 .
drwxr-xr-x 8 daholme uucp 4096 Feb 26 01:26 ..
-rw-r--r-- 1 daholme uucp 2027 Feb 26 01:26 JO$.java
-rw-r--r-- 1 daholme uucp 1322 Feb 26 01:26 JS$.java
so when ListPathsSafely was used together with a src directory to
expand into a set of source files, there was no $ escaping and so the
$ got dropped from the name, and so the nashorn build failed.
So the escaping was added to ListPathsSafely. But not the full \$$$$
as the need for that depends upon exactly how the make variable is
used. For Java compilation commands ListPathSafely only had to do a
simpler $ -> $$ escape sequence.
But this means that anything explicitly escaped with \$$$$ and passed
to ListPathsSafely was now broken as there were too many escapes. This
is what broke the profiles builds. So to fix that we had to reduce the
number of escapes in the explicitly named files ie:
sun/awt/motif/X11GB2312\$$Decoder.class
so that after processing by ListPathsSafely it was back to the full
\$$$$ form.
That would have been the end of it, except, as I pointed out, not all
uses of nested class file names get passed through ListFilesSafely.
Those that do not still need the \$$$$ escape, while those that do
need only \$$. Hence we have the problem that when writing the name of
such a file you have to know how it is going to be used - which is not
a very maintainable solution.
I propose that we regress the change to ListpathsSafely so that it
doesn't do the $ escape, and instead we either:
a) rename the nashorn .java files with $ in their name; or else
b) add the $ escape at a different point in the src list expansion so
that it only affects src list expansion
Pragmatically I prefer (a) because having $ in filenames is really a
PITA when it is both the make meta-character and the shell
meta-character. It's too late for .class file names but if we can
avoid adding it to .java file names then we will simplify our lives. :)
Otherwise (b) should be done somewhere in the bowels of
JavaCompilation.gmk, probably by adjusting $1_ALL_SRCS.
David
-----
-- Jim
On 2013-02-27, at 6:15 AM, Alan Bateman <[email protected]> wrote:
On 27/02/2013 02:47, David Holmes wrote:
:
So the same file names are listed once with \$$ and once with
\$$$$, and they both have to be that way to work!
This is untenable. There should only be one way to write the name
of a nested class file inside the makefile.
FYI in Profiles.gmk when expanding foo/*.class I already had to do
a similar substitution as is now in ListPathsSafely:
# Function to expand foo/*.class into the set of classes
# NOTE: Classfiles with $ in their name are problematic as that is the
# meta-character for both make and the shell! Hence the \$$$$
substitution.
# But note that if you echo these values they will NOT display as
expected.
class_list = $(patsubst $(JDK_OUTPUTDIR)/classes/%,%,\
$(foreach i,$(1), $(subst $$,\$$$$, $(wildcard
$(JDK_OUTPUTDIR)/classes/$i))))
So I'd like to understand why the nashorn change was made so that
we can determine how to get back to only having one way to specify
file names containing $
I completely agree, it's very difficult to maintain. Also the two
patches yesterday were just to get the build working again (Erik is
out this week so it wasn't possible to establish why MakeBase.gmk
was changed, also I cannot find the review thread to know if it came
up during the review).
-Alan
