Looks good to me too; I like the name choice of the new Provider/Description pair.

Maurizio

On 25/06/15 22:32, Jonathan Gibbons wrote:
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






Reply via email to