JDK 9 RFR of JDK-8072480: javac should support compilation for a specific platform version

Fri Jun 26 07:35:21 UTC 2015

This looks good to me now.

/Erik

On 2015-06-02 18:50, Jan Lahoda wrote:
> Hello Eric,
>
> Thanks for the change, this seems definitely better to me. I've folded 
> your change that into my patch. An updated version (just langtools 
> this time):
> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.04/langtools/
>
> Thanks!
>
> Jan
>
> On 2.6.2015 16:04, Erik Joelsson wrote:
>> Hello Jan,
>>
>> Sorry to bother you with even more build changes, but with these file
>> moves, I realized that this new file, ct.sym, is really a part of the
>> jdk.compiler module and really not a special case at all. Because of
>> this, it should be generated as part of the jdk.compiler-gendata target.
>> This also eliminates all the changes in the top level repo and only
>> leaves one new makefile in the langtools repo.
>>
>> http://cr.openjdk.java.net/~erikj/8072480/webrev.02/
>>
>> /Erik
>>
>> On 2015-06-01 17:56, Jan Lahoda wrote:
>>> Hi,
>>>
>>> I made a somewhat bigger update (partially based on other feedback).
>>> Summary of changes:
>>> -the history data has been moved into langtools (I also moved the
>>> Ctsym.gmk)
>>> -there are JDK 6 data now
>>> -renamed the "-platform" option to "-release". Code/tests directly
>>> related to the option has been also changed as well. I kept the
>>> internal PlatformProvider API in javac as is, and also kept related 
>>> code.
>>> -added a note that the <platform-description-file> is generated by
>>> CreateSymbols.
>>>
>>> Webrevs:
>>> top-level:
>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.03/top-level/
>>> langtools:
>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.03/langtools/
>>>
>>> Delta webrevs are also available.
>>>
>>> How does this look?
>>>
>>> Thanks for the comments so far!
>>>
>>> Jan
>>>
>>> On 27.5.2015 14:23, Jan Lahoda wrote:
>>>> Ah, yes, I did not realize that. Thanks, will fix.
>>>>
>>>> Thanks,
>>>>      Jan
>>>>
>>>> On 27.5.2015 11:59, Maurizio Cimadamore wrote:
>>>>> Looks great. The only nitpick is that the comments in CreateSymbols
>>>>> don't mention the fact that a side effect of the sym.txt 
>>>>> generation is
>>>>> the  <platform-description-file> mentioned earlier in the same 
>>>>> comments
>>>>> (so one might wonder where does that come from).
>>>>>
>>>>> Maurizio
>>>>>
>>>>> On 27/05/15 10:37, Jan Lahoda wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I've uploaded another iteration, with these changes:
>>>>>> -the "symbols" file is now generated automatically (for the typical
>>>>>> OpenJDK case).
>>>>>> -fixed a minor issue in CreateSymbols that could cause putting class
>>>>>> description into wrong a file (the "break" -> "break OUTER;" 
>>>>>> change).
>>>>>> -jdk.management module has been split out from java.management
>>>>>> recently, so splitting the corresponding .sym.txt files into
>>>>>> java.management and jdk.management as well.
>>>>>> -updating the copyright year in CreateSymbols, as noted by Magnus.
>>>>>>
>>>>>> Webrevs:
>>>>>> -top-level:
>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.02/top-level/
>>>>>> -langtools (no changes against the last webrev):
>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.02/langtools/
>>>>>>
>>>>>> Delta webrevs against the previous iteration are included under
>>>>>> "Author comments".
>>>>>>
>>>>>> Thanks for the comments so far!
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>> On 22.5.2015 15:22, Jan Lahoda wrote:
>>>>>>> On 22.5.2015 14:52, Maurizio Cimadamore wrote:
>>>>>>>> Excellent work.
>>>>>>>>
>>>>>>>> I think the comment in CreateSymbols could be made clearer w.r.t.
>>>>>>>> Probe
>>>>>>>> - i.e. that Probe should be ran on top of the JDK N - i.e.
>>>>>>>>
>>>>>>>> <JDK-8>/bin/java Probe --> classes-8
>>>>>>>> <JDK-7>/bin/java Probe --> classes-7
>>>>>>>> <JDK-6>/bin/java Probe --> classes-7
>>>>>>>>
>>>>>>>> etc.
>>>>>>>
>>>>>>> Sure, will do.
>>>>>>>
>>>>>>> I'll also look at generating the make/data/symbols/symbols
>>>>>>> descriptions
>>>>>>> automatically, per our offline discussion.
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>>>
>>>>>>>> Maurizio
>>>>>>>>
>>>>>>>> On 22/05/15 13:38, Jan Lahoda wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I've uploaded a new iteration of the patch(es):
>>>>>>>>> top-level repository:
>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.01/top-level/
>>>>>>>>> langtools:
>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.01/langtools/
>>>>>>>>>
>>>>>>>>> (besides full webrevs, there are also webrevs showing the
>>>>>>>>> differences
>>>>>>>>> between .00 and .01 available - see the "Delta webrev" link under
>>>>>>>>> "Author's comments")
>>>>>>>>>
>>>>>>>>> Summary of changes:
>>>>>>>>> -applied Eric's build changes
>>>>>>>>> -moved CreateSymbols into
>>>>>>>>> make/src/classes/build/tools/symbolgenerator
>>>>>>>>> -tried to improve the specification of base platforms in
>>>>>>>>> CreateSymbols, per Maurizio's comment
>>>>>>>>> -other cleanup in langtools per Maurizio's comments.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>     Jan
>>>>>>>>>
>>>>>>>>> On 21.5.2015 12:31, Maurizio Cimadamore wrote:
>>>>>>>>>> Hi Jan,
>>>>>>>>>> great work - couple of comments below:
>>>>>>>>>>
>>>>>>>>>> * it seems like some of the routines in Arguments can be
>>>>>>>>>> simplified
>>>>>>>>>> using lambdas - especially lookupPlatformProvider and
>>>>>>>>>> checkOptionAllowed
>>>>>>>>>> * Why JDKPlatformProvider is not called
>>>>>>>>>> JDKPlatformProvider*Factory* ?
>>>>>>>>>> * JavacProcessingEnvironment.JoiningIterator seems to have
>>>>>>>>>> commonalities
>>>>>>>>>> with CompoundScopeIterator - any chance that we might refactor
>>>>>>>>>> this a
>>>>>>>>>> bit?
>>>>>>>>>> * What's the rationale for giving an error if -platform is
>>>>>>>>>> specified
>>>>>>>>>> and
>>>>>>>>>> a non-StandardFileManager is provided? Can't we simply swallow
>>>>>>>>>> that,
>>>>>>>>>> ignore the platform settings and issue a Lint 'options' warning?
>>>>>>>>>> * Would it make sense for some of the classfile generation
>>>>>>>>>> logic in
>>>>>>>>>> CreateSymbols to be moved under the classfile API ?
>>>>>>>>>> * I had hard time reconciling some of the comments in
>>>>>>>>>> CreateSymbols
>>>>>>>>>> with
>>>>>>>>>> how the files were laid out. I think in the end, what you 
>>>>>>>>>> mean is
>>>>>>>>>> that
>>>>>>>>>> if you have platforms 7, 8, 9 - you should pick one baseline
>>>>>>>>>> (i.e. 8)
>>>>>>>>>> and then generate diffs for 9 and 7 against the 8 one. If
>>>>>>>>>> that's the
>>>>>>>>>> use
>>>>>>>>>> case, I think the command line can be simplified a bit in 
>>>>>>>>>> order to
>>>>>>>>>> explicitly state which of the platform is the baseline one, and
>>>>>>>>>> the
>>>>>>>>>> remaining parameters can be inferred from the tool (as the
>>>>>>>>>> <base-platform-for-platform1,2 ... N> seem to be typically all
>>>>>>>>>> identical
>>>>>>>>>> but one which is set to <none> - the one for the baseline). 
>>>>>>>>>> Maybe
>>>>>>>>>> the
>>>>>>>>>> inference logic should be different (i.e. use 8 as a baseline 
>>>>>>>>>> for
>>>>>>>>>> 7 and
>>>>>>>>>> 7 as a baseline for 6) - but - whatever the logic, I think it
>>>>>>>>>> should be
>>>>>>>>>> chosen once and for all, and live implicitly in the tool? Or are
>>>>>>>>>> there
>>>>>>>>>> reasons as to why it might be handy to customize the baselines?
>>>>>>>>>>
>>>>>>>>>> Maurizio
>>>>>>>>>>
>>>>>>>>>> On 21/05/15 08:01, Jan Lahoda wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> This is a patch adding a new option, -platform, to javac.
>>>>>>>>>>>
>>>>>>>>>>> Patch for the top-level repository is here:
>>>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.00/top-level/ 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Patch for the langtools repository is here:
>>>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.00/langtools/ 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> These changes also require additional langtools changes as 
>>>>>>>>>>> their
>>>>>>>>>>> prerequisite:
>>>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8080675/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> Overall, one can imagine '-platform N' to have the same 
>>>>>>>>>>> effect as
>>>>>>>>>>> '-source N -target N -bootclasspath <APIs-for-N>'. The possible
>>>>>>>>>>> values
>>>>>>>>>>> for 'N' are pluggable in a limited way, though (see
>>>>>>>>>>> langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/platform/PlatformProvider.java). 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Note that this patch only introduces N=7 and N=8, which
>>>>>>>>>>> correspond to
>>>>>>>>>>> Open JDK 7 and 8 GA.
>>>>>>>>>>>
>>>>>>>>>>> A tricky problem to solve is where to get the APIs for platform
>>>>>>>>>>> N. The
>>>>>>>>>>> proposal is to include history data in the textual format 
>>>>>>>>>>> inside
>>>>>>>>>>> the
>>>>>>>>>>> OpenJDK repositories (the input data), process them at build 
>>>>>>>>>>> time
>>>>>>>>>>> and
>>>>>>>>>>> create a lib/ct.sym file holding the data in the JDK image. 
>>>>>>>>>>> javac
>>>>>>>>>>> running with the -platform option then compiles against the
>>>>>>>>>>> lib/ct.sym
>>>>>>>>>>> file. The input history data are placed
>>>>>>>>>>> <top-level-repository>/make/data/symbols (the sym.txt files).
>>>>>>>>>>> This
>>>>>>>>>>> patches only includes data for OpenJDK 7 and 8, and only for
>>>>>>>>>>> public
>>>>>>>>>>> APIs (more or less Java SE and JDK @Exported APIs).
>>>>>>>>>>>
>>>>>>>>>>> The size of the history data is currently ~6MB in the JDK
>>>>>>>>>>> checkout and
>>>>>>>>>>> ~650kB inside the .hg directory (the size could change
>>>>>>>>>>> significantly
>>>>>>>>>>> if additional classes/elements, like non-public API classes,
>>>>>>>>>>> would
>>>>>>>>>>> need to be included). The lib/ct.sym file is currently ~4.5MB.
>>>>>>>>>>>
>>>>>>>>>>> The ct.sym file is a zip file containing signature files. The
>>>>>>>>>>> signature files are similar to classfiles (so javac can read
>>>>>>>>>>> them as
>>>>>>>>>>> classfiles), but are missing some attributes, most notably the
>>>>>>>>>>> Code
>>>>>>>>>>> attribute. On the top-level, the ct.sym contains directories
>>>>>>>>>>> named
>>>>>>>>>>> "7", "78" and "8". When compiling for version 'N', the
>>>>>>>>>>> bootclasspath
>>>>>>>>>>> for that version is obtained by using directories which have
>>>>>>>>>>> 'N' in
>>>>>>>>>>> their name.
>>>>>>>>>>>
>>>>>>>>>>> The sigfiles for ct.sym are created by
>>>>>>>>>>> <top-level-repository>/make/tools/symbolgenerator/CreateSymbols.java. 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The same file can also be used to construct the sym.txt files.
>>>>>>>>>>> Concise
>>>>>>>>>>> instructions are part of the CreateSymbols.java.
>>>>>>>>>>>
>>>>>>>>>>> I am sending this as one review, as the changes together 
>>>>>>>>>>> make one
>>>>>>>>>>> feature, but the langtools and top-level changes are 
>>>>>>>>>>> independent
>>>>>>>>>>> to a
>>>>>>>>>>> great degree: the langtools changes add the "-platform" javac;
>>>>>>>>>>> and the
>>>>>>>>>>> top-level changes add the history data and ability to build the
>>>>>>>>>>> ct.sym
>>>>>>>>>>> file. If desired, these could be pushed and/or reviewed
>>>>>>>>>>> independently.
>>>>>>>>>>>
>>>>>>>>>>> Many thanks go to Jon Gibbons, Joe Darcy, Magnus Ihse Bursie,
>>>>>>>>>>> Alan
>>>>>>>>>>> Bateman and others who have provided advices and help on the
>>>>>>>>>>> matter
>>>>>>>>>>> before.
>>>>>>>>>>>
>>>>>>>>>>> Any insights/comments are wholeheartedly welcome.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>     Jan
>>>>>>>>>>
>>>>>>>>
>>>>>
>>