Dollar ($) expansion still needs attention

[David Holmes]

Just to close off this thread. The nashorn code was changed to not use $ 
in .java file names. So we can revert the \$$$$ changes:

JDK-8009428: Revert changes to $ substitution performed as part of 
nashorn integration

David

On 4/03/2013 2:59 PM, 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 <Alan.Bateman at oracle.com> 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
>>