Dollar ($) expansion still needs attention

[Erik Joelsson]

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