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

Thu Jun 25 21:32:42 UTC 2015

Looks good to me :-)

-- Jon

On 06/24/2015 06:13 AM, Jan Lahoda wrote:
> Hello,
>
> After some offline discussions, I've somewhat changed the internal API 
> for plugging in the platforms (based on Jon's advices). An updated 
> webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8072480/webrev.05/langtools/
>
> How does this look?
>
> Thanks for all the comments!
>
> Jan
>
> On 2.6.2015 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>