RFR: JDK-8189094: Change required boot jdk to JDK 9

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
With JDK 9 released, it's high time to change the required boot jdk for
building JDK 10. This time, the change wasn't as straight forward as it
usually is.

It's currently possible to use any of JDK 8, 9 or a recent build of 10
to boot the JDK 10 build. This support is however fragile. The most
sensitive part is the building and running of the interim javac and
javadoc tools, where we build the new JDK 10 versions of these tools,
but -source/-target set appropriate for the boot jdk, so we can run them
on the boot jdk when compiling the rest of the product.

In the current build, we compile the java source files of the modules
java.compiler, jdk.compiler, jdk.javadoc and jdk.jdeps in the legacy
way, using -Xbootclasspath and not including any module-info.java files.
We then run them by using the --patch-module argument. This means we are
running the JDK 10 classes but using the module definitions of the boot
jdk. This works for now, but when the JDK module definitions for any of
these modules need to change, this model will start to break.

In this patch I have tried to change this so we compile and run using
JDK 9 module style modes. The big problem to overcome then is that
jdk.compiler, and jdk.javadoc are not upgradeable. This means we can't
compile new versions of these modules and override them in the boot jdk.
This leaves us with two options: either run the interim classes in the
unnamed module, or define a new set of interim modules, based on the
existing modules but with new names. The first option seems simpler, but
that would require maintaining legacy service provider definitions for
these modules. So I chose the latter instead. The new module names have
".interim" as suffix.

To generate the new modules, I copy the module-info.java files to a new
gensrc dir and sed replace the module names. I also generate a new
ToolProvider.java so that the default tools are taken from the interim
modules.

I've made sure that jrtfs.jar is generated with --release 8 to keep
compatibility with JDK 8.

Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01

Bug: https://bugs.openjdk.java.net/browse/JDK-8189094

/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Jonathan Gibbons
On 10/16/17 11:53 AM, Martin Buchholz wrote:
> What's the canonical way to list all upgradeable modules?
> There's a list in JEP 261, but naturally we can't consider it authoritative.
>
> I was surprised that even java --describe-module doesn't tell me whether a
> module is upgradeable.
>
> I was surprised to see JEP 261 say that java.compiler is upgradeable, but
> not jdk.compiler.
> Perhaps thinking of the bootstrap use case?
java.compiler is upgradeable to allow alternate implementations, such as
in the bootstrap case, and in IDEs that want to run on older JDKs and still
use compile-time reflection using javax.lang.model API.

jdk.compiler is not upgradeable, because it is tied into other modules, and
we don't want to claim to support general mix-n-match of these modules.

-- Jon
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Magnus Ihse Bursie
In reply to this post by Erik Joelsson
On 2017-10-16 15:12, Erik Joelsson wrote:

> With JDK 9 released, it's high time to change the required boot jdk
> for building JDK 10. This time, the change wasn't as straight forward
> as it usually is.
>
> It's currently possible to use any of JDK 8, 9 or a recent build of 10
> to boot the JDK 10 build. This support is however fragile. The most
> sensitive part is the building and running of the interim javac and
> javadoc tools, where we build the new JDK 10 versions of these tools,
> but -source/-target set appropriate for the boot jdk, so we can run
> them on the boot jdk when compiling the rest of the product.
>
> In the current build, we compile the java source files of the modules
> java.compiler, jdk.compiler, jdk.javadoc and jdk.jdeps in the legacy
> way, using -Xbootclasspath and not including any module-info.java
> files. We then run them by using the --patch-module argument. This
> means we are running the JDK 10 classes but using the module
> definitions of the boot jdk. This works for now, but when the JDK
> module definitions for any of these modules need to change, this model
> will start to break.
>
> In this patch I have tried to change this so we compile and run using
> JDK 9 module style modes. The big problem to overcome then is that
> jdk.compiler, and jdk.javadoc are not upgradeable. This means we can't
> compile new versions of these modules and override them in the boot
> jdk. This leaves us with two options: either run the interim classes
> in the unnamed module, or define a new set of interim modules, based
> on the existing modules but with new names. The first option seems
> simpler, but that would require maintaining legacy service provider
> definitions for these modules. So I chose the latter instead. The new
> module names have ".interim" as suffix.
>
> To generate the new modules, I copy the module-info.java files to a
> new gensrc dir and sed replace the module names. I also generate a new
> ToolProvider.java so that the default tools are taken from the interim
> modules.
>
> I've made sure that jrtfs.jar is generated with --release 8 to keep
> compatibility with JDK 8.
>
> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01

This is a quite complex change. It's a bit unfortunate that we have to
make these kinds of changes to use JDK 9 as a boot JDK. Anyway, that's
the way it is.

A couple of remarks:

* In boot-jdk.m4, please update the comment as well:

    # When compiling code to be executed by the Boot JDK, force jdk8 compatibility.
- BOOT_JDK_SOURCETARGET="-source 8 -target 8"
+ BOOT_JDK_SOURCETARGET="-source 9 -target 9"

* In JavaCompilation.gmk:

+ # exist yet, is in it.
+ $$(foreach d,$$($1_SRC), \

You can add a space after the comma with no ill effects.

- $$(eval $$(call FillCacheFind,$$($1_SRC)))
+ $$(eval $$(call FillCacheFind, $$(wildcard $$($1_SRC))))

Should not the $(wildcard) filtering be done in the FillCacheFind
function? It seems reasonable that it should not complain with an error
if you give it a non-existing directory.

- $1_ALL_SRCS += $$(foreach s, $$($1_SRC), $$(call CacheFind, $$(s)))
+ $1_ALL_SRCS += $$($1_EXTRA_FILES) $$(foreach s, $$($1_SRC), $$(if
$$(wildcard $$s), \
+ $$(call CacheFind, $$s)))


The same goes here. Any wildcard filtering should be done in CacheFind,
I think.

* In spec.gmk.in, the code for INTERIM_LANGTOOLS_MODULES_COMMA is
basically repeating the pre-existing macro CommaList. However, that is
only available in MakeBase.gmk. I'm not sure it's possible to use, but
maybe. It looks like there's a lot of hard-coded stuff in spec.gmk.in,
and maybe that is not the right place for it. Where are 
INTERIM_LANGTOOLS_ARGS used?

* In jib-profiles.js:
You have no "var" declaration for the new boot_jdk_version etc. I'm not
too well-versed in javascript and it is probably quite legal not to, but
I think it's better style at least to keep it.

* Finally, $(BUILDTOOLS_OUTPUTDIR)/override_modules should be renamed
"interim_modules".

/Magnus
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
New webrev:


On 2017-10-17 13:10, Magnus Ihse Bursie wrote:

>
> This is a quite complex change. It's a bit unfortunate that we have to
> make these kinds of changes to use JDK 9 as a boot JDK. Anyway, that's
> the way it is.
>
> A couple of remarks:
>
> * In boot-jdk.m4, please update the comment as well:
>     # When compiling code to be executed by the Boot JDK, force jdk8 compatibility.
> - BOOT_JDK_SOURCETARGET="-source 8 -target 8"
> + BOOT_JDK_SOURCETARGET="-source 9 -target 9"
Fixed
> * In JavaCompilation.gmk:
> + # exist yet, is in it.
> + $$(foreach d,$$($1_SRC), \
Fixed

> You can add a space after the comma with no ill effects.
> - $$(eval $$(call FillCacheFind,$$($1_SRC)))
> + $$(eval $$(call FillCacheFind, $$(wildcard $$($1_SRC))))
> Should not the $(wildcard) filtering be done in the FillCacheFind
> function? It seems reasonable that it should not complain with an
> error if you give it a non-existing directory.
> - $1_ALL_SRCS += $$(foreach s, $$($1_SRC), $$(call CacheFind, $$(s)))
> + $1_ALL_SRCS += $$($1_EXTRA_FILES) $$(foreach s, $$($1_SRC), $$(if
> $$(wildcard $$s), \
> + $$(call CacheFind, $$s)))
>
> The same goes here. Any wildcard filtering should be done in
> CacheFind, I think.
>
I looked at CacheFind and figured it didn't already do wildcard
consistently so opted to change the call site. In the new patch I added
wildcard checking in all instances of CacheFind instead. This means we
likely have places where we can remove redundant wildcard calls in other
places. Removing those in this change is starting to feel like too much
however. I can file a followup.
> * In spec.gmk.in, the code for INTERIM_LANGTOOLS_MODULES_COMMA is
> basically repeating the pre-existing macro CommaList. However, that is
> only available in MakeBase.gmk. I'm not sure it's possible to use, but
> maybe. It looks like there's a lot of hard-coded stuff in spec.gmk.in,
> and maybe that is not the right place for it. Where are 
> INTERIM_LANGTOOLS_ARGS used?
The SPEC is always included first, so the CommaList macro is not
available. I agree that the spec is not necessarily the optimal place to
put static things like this, but we don't really have a Common or Defs
to put such things in. The args are used whenever we need to run the
interim langtools, including compiling java, running javadoc and some
gensrc/gendata. Some of the other variables are needed when compiling
the interim modules.

In my defense I'm not changing the location of these settings in the patch.
>
> * In jib-profiles.js:
> You have no "var" declaration for the new boot_jdk_version etc. I'm
> not too well-versed in javascript and it is probably quite legal not
> to, but I think it's better style at least to keep it.
Fixed
>
> * Finally, $(BUILDTOOLS_OUTPUTDIR)/override_modules should be renamed
> "interim_modules".
>
Fixed

/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Mandy Chung
In reply to this post by Erik Joelsson
Hi Erik,

On 10/16/17 6:12 AM, Erik Joelsson wrote:
>
> To generate the new modules, I copy the module-info.java files to a
> new gensrc dir and sed replace the module names. I also generate a new
> ToolProvider.java so that the default tools are taken from the interim
> modules.
>
> :
>
> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01

65 java.compiler_EXTRA_FILES :=
$(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
66 TARGETS += $(java.compiler_EXTRA_FILES)


What issue did you run into without patching ToolProvider?  I have assumed
the build always launches javac/javadoc main class and not using
ToolProvider.

580 JAVAC_MAIN_CLASS = -m jdk.compiler.interim/com.sun.tools.javac.Main
581 JAVADOC_MAIN_CLASS = -m
jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool using
the ToolProvider. That part of the change was just a reaction to ctsym
generation failing.

/Erik


On 2017-10-17 17:47, mandy chung wrote:

> Hi Erik,
>
> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>
>> To generate the new modules, I copy the module-info.java files to a
>> new gensrc dir and sed replace the module names. I also generate a
>> new ToolProvider.java so that the default tools are taken from the
>> interim modules.
>>
>> :
>>
>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>
> 65 java.compiler_EXTRA_FILES :=
> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>
>
> What issue did you run into without patching ToolProvider?  I have assumed
> the build always launches javac/javadoc main class and not using
> ToolProvider.
>
> 580 JAVAC_MAIN_CLASS = -m jdk.compiler.interim/com.sun.tools.javac.Main
> 581 JAVADOC_MAIN_CLASS = -m
> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Mandy Chung
It'd be simpler if we could avoid patching ToolProvider.   The ctsym
generator is a build tool.  Jan and Jon may have suggestion if the build
tool can find the compiler from the interim module instead and use
--limit-modules to java.base?

Mandy

On 10/17/17 8:55 AM, Erik Joelsson wrote:

>
> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
> using the ToolProvider. That part of the change was just a reaction to
> ctsym generation failing.
>
> /Erik
>
>
> On 2017-10-17 17:47, mandy chung wrote:
>> Hi Erik,
>>
>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>
>>> To generate the new modules, I copy the module-info.java files to a
>>> new gensrc dir and sed replace the module names. I also generate a
>>> new ToolProvider.java so that the default tools are taken from the
>>> interim modules.
>>>
>>> :
>>>
>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>
>> 65 java.compiler_EXTRA_FILES :=
>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>
>>
>> What issue did you run into without patching ToolProvider?  I have assumed
>> the build always launches javac/javadoc main class and not using
>> ToolProvider.
>>
>> 580 JAVAC_MAIN_CLASS = -m jdk.compiler.interim/com.sun.tools.javac.Main
>> 581 JAVADOC_MAIN_CLASS = -m
>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Jan Lahoda
The problematic part is TransitiveDependencies, right?

Might be easier to replace:
JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
with:
JavaCompiler compiler = JavacTool.create();

(import com.sun.tools.javac.api.JavacTool;)

That should avoid the ServiceLoader lookup.

Jan

On 17.10.2017 19:04, mandy chung wrote:

> It'd be simpler if we could avoid patching ToolProvider.   The ctsym
> generator is a build tool.  Jan and Jon may have suggestion if the build
> tool can find the compiler from the interim module instead and use
> --limit-modules to java.base?
>
> Mandy
>
> On 10/17/17 8:55 AM, Erik Joelsson wrote:
>>
>> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
>> using the ToolProvider. That part of the change was just a reaction to
>> ctsym generation failing.
>>
>> /Erik
>>
>>
>> On 2017-10-17 17:47, mandy chung wrote:
>>> Hi Erik,
>>>
>>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>>
>>>> To generate the new modules, I copy the module-info.java files to a
>>>> new gensrc dir and sed replace the module names. I also generate a
>>>> new ToolProvider.java so that the default tools are taken from the
>>>> interim modules.
>>>>
>>>> :
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>>
>>> 65 java.compiler_EXTRA_FILES :=
>>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>>>
>>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>>
>>>
>>> What issue did you run into without patching ToolProvider?  I have
>>> assumed
>>> the build always launches javac/javadoc main class and not using
>>> ToolProvider.
>>>
>>> 580 JAVAC_MAIN_CLASS = -m jdk.compiler.interim/com.sun.tools.javac.Main
>>> 581 JAVADOC_MAIN_CLASS = -m
>>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Martin Buchholz-3
Does $(INTERIM_LANGTOOLS_MODULES_COMMA) need to be repeated below?  I would
think you could drop it from --limit-modules

+INTERIM_LANGTOOLS_ARGS := \
+    --limit-modules java.base,jdk.zipfs,$(INTERIM_LANGTOOLS_MODULES_COMMA) \
+    --add-modules $(INTERIM_LANGTOOLS_MODULES_COMMA) \
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Mandy Chung
In reply to this post by Jan Lahoda
On 10/17/17 10:44 AM, Jan Lahoda wrote:

> The problematic part is TransitiveDependencies, right?
>
> Might be easier to replace:
> JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
> with:
> JavaCompiler compiler = JavacTool.create();
>
> (import com.sun.tools.javac.api.JavacTool;)
>
> That should avoid the ServiceLoader lookup.
>

Worth trying this and  I think this needs --limit-modules to hide
jdk.compiler and other interim modules.

Does this build tool just depend on javac?  Does it need all system
modules to be observable?

Mandy

> Jan
>
> On 17.10.2017 19:04, mandy chung wrote:
>> It'd be simpler if we could avoid patching ToolProvider.   The ctsym
>> generator is a build tool.  Jan and Jon may have suggestion if the build
>> tool can find the compiler from the interim module instead and use
>> --limit-modules to java.base?
>>
>> Mandy
>>
>> On 10/17/17 8:55 AM, Erik Joelsson wrote:
>>>
>>> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
>>> using the ToolProvider. That part of the change was just a reaction to
>>> ctsym generation failing.
>>>
>>> /Erik
>>>
>>>
>>> On 2017-10-17 17:47, mandy chung wrote:
>>>> Hi Erik,
>>>>
>>>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>>>
>>>>> To generate the new modules, I copy the module-info.java files to a
>>>>> new gensrc dir and sed replace the module names. I also generate a
>>>>> new ToolProvider.java so that the default tools are taken from the
>>>>> interim modules.
>>>>>
>>>>> :
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>>>
>>>> 65 java.compiler_EXTRA_FILES :=
>>>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>>>>
>>>>
>>>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>>>
>>>>
>>>> What issue did you run into without patching ToolProvider? I have
>>>> assumed
>>>> the build always launches javac/javadoc main class and not using
>>>> ToolProvider.
>>>>
>>>> 580 JAVAC_MAIN_CLASS = -m
>>>> jdk.compiler.interim/com.sun.tools.javac.Main
>>>> 581 JAVADOC_MAIN_CLASS = -m
>>>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Jonathan Gibbons
In reply to this post by Erik Joelsson


On 10/17/17 12:20 PM, Martin Buchholz wrote:

> On Tue, Oct 17, 2017 at 10:10 AM, Alan Bateman <[hidden email]>
> wrote:
>
>> On 17/10/2017 17:53, Martin Buchholz wrote:
>>
>> I tried to figure out how this patch actually works.  At first I thought
>> we were "shading" (renaming all the packages in the source files) but now I
>> think we're merely renaming the modules by appending ".interim" to the
>> names.  But that looks like cheating to me - we're not allowed to have
>> multiple modules containing the same packages, but are getting away with it
>> because the runtime happens to never look at the (unused) original jdk9
>> modules?
>>
>> The patch uses `--limit-modules` to limit the observability of modules and
>> prevent the jdk.compiler module in the boot JDK from being resolved. If the
>> boot JDK were to resolve jdk.compiler (in its own run-time image) and
>> jdk.compiler.interim on the module path then you would end up with two
>> modules containing the same classes mapped to the app class loader, can't
>> do that.
>>
> Wow, --limit-modules sure is a good trick.  So now we have two possible
> replacements for the demise of -Xbootclasspath/p:
> --patch-module
> --limit-modules combined with renamed replacement modules
>
> Looking at the patch I see
>
> +INTERIM_LANGTOOLS_ADD_EXPORTS := \
> +    --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim \
> +    --add-exports java.base/jdk.internal.util.jar=jdk.jdeps.interim \
> +    --add-exports java.base/jdk.internal.misc=jdk.jdeps.interim \
> +    #
>
> so the interim compiler is accessing JDK internals - isn't this what we're
> telling users NOT to do?  Especially when this is the internals of the boot
> jdk - You can't bootstrap jdk10 with any compliant implementation of Java
> SE 9!  The jdk bootstrap process should be setting a good example!
The add-exports for jdk.compiler is not specific to the bootstrap process,
it is needed for any copy of javac, and is a consequence of design decisions
made (or not made) long, long ago in the JDK 5 timeframe. And yes, it
would be nice to eventually get rid of this.

-- Jon
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
In reply to this post by Martin Buchholz-3
Hello Martin,


On 2017-10-17 19:54, Martin Buchholz wrote:
> Does $(INTERIM_LANGTOOLS_MODULES_COMMA) need to be repeated below?  I
> would think you could drop it from --limit-modules
> +INTERIM_LANGTOOLS_ARGS := \
> +    --limit-modules java.base,jdk.zipfs,$(INTERIM_LANGTOOLS_MODULES_COMMA) \
> +    --add-modules $(INTERIM_LANGTOOLS_MODULES_COMMA) \
>
--limit-modules is needed because jdk.compiler.interim contains the same
packages as the system modules jdk.compiler, so we need to prevent the
runtime from resolving jdk.compiler (and the other base interim modules).

I spent a lot of trial and error before arriving at the set of arguments
used here.

/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
In reply to this post by Mandy Chung
Hello,


On 2017-10-17 19:54, mandy chung wrote:

> On 10/17/17 10:44 AM, Jan Lahoda wrote:
>> The problematic part is TransitiveDependencies, right?
>>
>> Might be easier to replace:
>> JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
>> with:
>> JavaCompiler compiler = JavacTool.create();
>>
>> (import com.sun.tools.javac.api.JavacTool;)
>>
>> That should avoid the ServiceLoader lookup.
>>
>
I can try this instead of patching ToolProvider.
> Worth trying this and  I think this needs --limit-modules to hide
> jdk.compiler and other interim modules.
Yes, --limit-modules is definitely needed and already used.
>
> Does this build tool just depend on javac?  Does it need all system
> modules to be observable?
>
I'm currently running it with the same arguments as when I run interim
javac and it works:

--limit-modules
java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim

--add-modules
java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim

--module-path
/localhome/hg/jdk10-boot9/build/linux-x64/buildtools/interim_modules
--add-exports java.base/sun.reflect.annotation=jdk.compiler.interim
--add-exports java.base/jdk.internal.util.jar=jdk.jdeps.interim
--add-exports java.base/jdk.internal.misc=jdk.jdeps.interim

Some of these may be unnecessary for this specific tool, but it is
certainly simpler from my point of view to just use the same set of
arguments instead of maintaining another set.

/Erik

> Mandy
>
>> Jan
>>
>> On 17.10.2017 19:04, mandy chung wrote:
>>> It'd be simpler if we could avoid patching ToolProvider.   The ctsym
>>> generator is a build tool.  Jan and Jon may have suggestion if the
>>> build
>>> tool can find the compiler from the interim module instead and use
>>> --limit-modules to java.base?
>>>
>>> Mandy
>>>
>>> On 10/17/17 8:55 AM, Erik Joelsson wrote:
>>>>
>>>> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
>>>> using the ToolProvider. That part of the change was just a reaction to
>>>> ctsym generation failing.
>>>>
>>>> /Erik
>>>>
>>>>
>>>> On 2017-10-17 17:47, mandy chung wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>>>>
>>>>>> To generate the new modules, I copy the module-info.java files to a
>>>>>> new gensrc dir and sed replace the module names. I also generate a
>>>>>> new ToolProvider.java so that the default tools are taken from the
>>>>>> interim modules.
>>>>>>
>>>>>> :
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>>>>
>>>>> 65 java.compiler_EXTRA_FILES :=
>>>>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>>>>>
>>>>>
>>>>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>>>>
>>>>>
>>>>> What issue did you run into without patching ToolProvider? I have
>>>>> assumed
>>>>> the build always launches javac/javadoc main class and not using
>>>>> ToolProvider.
>>>>>
>>>>> 580 JAVAC_MAIN_CLASS = -m
>>>>> jdk.compiler.interim/com.sun.tools.javac.Main
>>>>> 581 JAVADOC_MAIN_CLASS = -m
>>>>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
In reply to this post by Erik Joelsson
Actual link: http://cr.openjdk.java.net/~erikj/8189094/webrev.02/


On 2017-10-17 15:59, Erik Joelsson wrote:

> New webrev:
>
>
> On 2017-10-17 13:10, Magnus Ihse Bursie wrote:
>>
>> This is a quite complex change. It's a bit unfortunate that we have
>> to make these kinds of changes to use JDK 9 as a boot JDK. Anyway,
>> that's the way it is.
>>
>> A couple of remarks:
>>
>> * In boot-jdk.m4, please update the comment as well:
>>     # When compiling code to be executed by the Boot JDK, force jdk8
>> compatibility.
>> - BOOT_JDK_SOURCETARGET="-source 8 -target 8"
>> + BOOT_JDK_SOURCETARGET="-source 9 -target 9"
> Fixed
>> * In JavaCompilation.gmk:
>> + # exist yet, is in it.
>> + $$(foreach d,$$($1_SRC), \
> Fixed
>> You can add a space after the comma with no ill effects.
>> - $$(eval $$(call FillCacheFind,$$($1_SRC)))
>> + $$(eval $$(call FillCacheFind, $$(wildcard $$($1_SRC))))
>> Should not the $(wildcard) filtering be done in the FillCacheFind
>> function? It seems reasonable that it should not complain with an
>> error if you give it a non-existing directory.
>> - $1_ALL_SRCS += $$(foreach s, $$($1_SRC), $$(call CacheFind, $$(s)))
>> + $1_ALL_SRCS += $$($1_EXTRA_FILES) $$(foreach s, $$($1_SRC), $$(if
>> $$(wildcard $$s), \
>> + $$(call CacheFind, $$s)))
>>
>> The same goes here. Any wildcard filtering should be done in
>> CacheFind, I think.
>>
> I looked at CacheFind and figured it didn't already do wildcard
> consistently so opted to change the call site. In the new patch I
> added wildcard checking in all instances of CacheFind instead. This
> means we likely have places where we can remove redundant wildcard
> calls in other places. Removing those in this change is starting to
> feel like too much however. I can file a followup.
>> * In spec.gmk.in, the code for INTERIM_LANGTOOLS_MODULES_COMMA is
>> basically repeating the pre-existing macro CommaList. However, that
>> is only available in MakeBase.gmk. I'm not sure it's possible to use,
>> but maybe. It looks like there's a lot of hard-coded stuff in
>> spec.gmk.in, and maybe that is not the right place for it. Where are
>> INTERIM_LANGTOOLS_ARGS used?
> The SPEC is always included first, so the CommaList macro is not
> available. I agree that the spec is not necessarily the optimal place
> to put static things like this, but we don't really have a Common or
> Defs to put such things in. The args are used whenever we need to run
> the interim langtools, including compiling java, running javadoc and
> some gensrc/gendata. Some of the other variables are needed when
> compiling the interim modules.
>
> In my defense I'm not changing the location of these settings in the
> patch.
>>
>> * In jib-profiles.js:
>> You have no "var" declaration for the new boot_jdk_version etc. I'm
>> not too well-versed in javascript and it is probably quite legal not
>> to, but I think it's better style at least to keep it.
> Fixed
>>
>> * Finally, $(BUILDTOOLS_OUTPUTDIR)/override_modules should be renamed
>> "interim_modules".
>>
> Fixed
>
> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Magnus Ihse Bursie
In reply to this post by Erik Joelsson
On 2017-10-17 15:59, Erik Joelsson wrote:

>
> New webrev:
>
>
> On 2017-10-17 13:10, Magnus Ihse Bursie wrote:
>>
>> This is a quite complex change. It's a bit unfortunate that we have
>> to make these kinds of changes to use JDK 9 as a boot JDK. Anyway,
>> that's the way it is.
>>
>> A couple of remarks:
>>
>> * In boot-jdk.m4, please update the comment as well:
>>     # When compiling code to be executed by the Boot JDK, force jdk8 compatibility.
>> - BOOT_JDK_SOURCETARGET="-source 8 -target 8"
>> + BOOT_JDK_SOURCETARGET="-source 9 -target 9"
> Fixed
>> * In JavaCompilation.gmk:
>> + # exist yet, is in it.
>> + $$(foreach d,$$($1_SRC), \
> Fixed
>> You can add a space after the comma with no ill effects.
>> - $$(eval $$(call FillCacheFind,$$($1_SRC)))
>> + $$(eval $$(call FillCacheFind, $$(wildcard $$($1_SRC))))
>> Should not the $(wildcard) filtering be done in the FillCacheFind
>> function? It seems reasonable that it should not complain with an
>> error if you give it a non-existing directory.
>> - $1_ALL_SRCS += $$(foreach s, $$($1_SRC), $$(call CacheFind, $$(s)))
>> + $1_ALL_SRCS += $$($1_EXTRA_FILES) $$(foreach s, $$($1_SRC), $$(if
>> $$(wildcard $$s), \
>> + $$(call CacheFind, $$s)))
>>
>> The same goes here. Any wildcard filtering should be done in
>> CacheFind, I think.
>>
> I looked at CacheFind and figured it didn't already do wildcard
> consistently so opted to change the call site. In the new patch I
> added wildcard checking in all instances of CacheFind instead. This
> means we likely have places where we can remove redundant wildcard
> calls in other places. Removing those in this change is starting to
> feel like too much however. I can file a followup.
Please do. I made a quick check, and it looks like it's at least 10
places we can simplify.

>> * In spec.gmk.in, the code for INTERIM_LANGTOOLS_MODULES_COMMA is
>> basically repeating the pre-existing macro CommaList. However, that
>> is only available in MakeBase.gmk. I'm not sure it's possible to use,
>> but maybe. It looks like there's a lot of hard-coded stuff in
>> spec.gmk.in, and maybe that is not the right place for it. Where are 
>> INTERIM_LANGTOOLS_ARGS used?
> The SPEC is always included first, so the CommaList macro is not
> available. I agree that the spec is not necessarily the optimal place
> to put static things like this, but we don't really have a Common or
> Defs to put such things in. The args are used whenever we need to run
> the interim langtools, including compiling java, running javadoc and
> some gensrc/gendata. Some of the other variables are needed when
> compiling the interim modules.
>
> In my defense I'm not changing the location of these settings in the
> patch.
Fair enough. Maybe we should have a make/common/Constants.gmk or
something like that which is included first in the spec?

>>
>> * In jib-profiles.js:
>> You have no "var" declaration for the new boot_jdk_version etc. I'm
>> not too well-versed in javascript and it is probably quite legal not
>> to, but I think it's better style at least to keep it.
> Fixed
>>
>> * Finally, $(BUILDTOOLS_OUTPUTDIR)/override_modules should be renamed
>> "interim_modules".
>>
> Fixed

Your fixed webrev looks good to me.

/Magnus

>
> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Erik Joelsson
In reply to this post by Erik Joelsson
New webrev again: http://cr.openjdk.java.net/~erikj/8189094/webrev.03/

Removed the ToolProvider patching.

Changed build/tools/symbolgenerator/TransitiveDependencies.java to use
JavacTool directly.

Changed Gendata-jdk.compiler.gmk to just use INTERIM_LANGTOOLS_ARGS
instead of listing a separate set of arguments for compilation.

/Erik


On 2017-10-18 10:18, Erik Joelsson wrote:

> Hello,
>
>
> On 2017-10-17 19:54, mandy chung wrote:
>> On 10/17/17 10:44 AM, Jan Lahoda wrote:
>>> The problematic part is TransitiveDependencies, right?
>>>
>>> Might be easier to replace:
>>> JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
>>> with:
>>> JavaCompiler compiler = JavacTool.create();
>>>
>>> (import com.sun.tools.javac.api.JavacTool;)
>>>
>>> That should avoid the ServiceLoader lookup.
>>>
>>
> I can try this instead of patching ToolProvider.
>> Worth trying this and  I think this needs --limit-modules to hide
>> jdk.compiler and other interim modules.
> Yes, --limit-modules is definitely needed and already used.
>>
>> Does this build tool just depend on javac?  Does it need all system
>> modules to be observable?
>>
> I'm currently running it with the same arguments as when I run interim
> javac and it works:
>
> --limit-modules
> java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim
>
> --add-modules
> java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim
>
> --module-path
> /localhome/hg/jdk10-boot9/build/linux-x64/buildtools/interim_modules
> --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim
> --add-exports java.base/jdk.internal.util.jar=jdk.jdeps.interim
> --add-exports java.base/jdk.internal.misc=jdk.jdeps.interim
>
> Some of these may be unnecessary for this specific tool, but it is
> certainly simpler from my point of view to just use the same set of
> arguments instead of maintaining another set.
>
> /Erik
>> Mandy
>>
>>> Jan
>>>
>>> On 17.10.2017 19:04, mandy chung wrote:
>>>> It'd be simpler if we could avoid patching ToolProvider.   The ctsym
>>>> generator is a build tool.  Jan and Jon may have suggestion if the
>>>> build
>>>> tool can find the compiler from the interim module instead and use
>>>> --limit-modules to java.base?
>>>>
>>>> Mandy
>>>>
>>>> On 10/17/17 8:55 AM, Erik Joelsson wrote:
>>>>>
>>>>> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
>>>>> using the ToolProvider. That part of the change was just a
>>>>> reaction to
>>>>> ctsym generation failing.
>>>>>
>>>>> /Erik
>>>>>
>>>>>
>>>>> On 2017-10-17 17:47, mandy chung wrote:
>>>>>> Hi Erik,
>>>>>>
>>>>>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>>>>>
>>>>>>> To generate the new modules, I copy the module-info.java files to a
>>>>>>> new gensrc dir and sed replace the module names. I also generate a
>>>>>>> new ToolProvider.java so that the default tools are taken from the
>>>>>>> interim modules.
>>>>>>>
>>>>>>> :
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>>>>>
>>>>>> 65 java.compiler_EXTRA_FILES :=
>>>>>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>>>>>>
>>>>>>
>>>>>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>>>>>
>>>>>>
>>>>>> What issue did you run into without patching ToolProvider? I have
>>>>>> assumed
>>>>>> the build always launches javac/javadoc main class and not using
>>>>>> ToolProvider.
>>>>>>
>>>>>> 580 JAVAC_MAIN_CLASS = -m
>>>>>> jdk.compiler.interim/com.sun.tools.javac.Main
>>>>>> 581 JAVADOC_MAIN_CLASS = -m
>>>>>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Magnus Ihse Bursie
On 2017-10-18 11:29, Erik Joelsson wrote:
> New webrev again: http://cr.openjdk.java.net/~erikj/8189094/webrev.03/
Looks good to me!

/Magnus

>
> Removed the ToolProvider patching.
>
> Changed build/tools/symbolgenerator/TransitiveDependencies.java to use
> JavacTool directly.
>
> Changed Gendata-jdk.compiler.gmk to just use INTERIM_LANGTOOLS_ARGS
> instead of listing a separate set of arguments for compilation.
>
> /Erik
>
>
> On 2017-10-18 10:18, Erik Joelsson wrote:
>> Hello,
>>
>>
>> On 2017-10-17 19:54, mandy chung wrote:
>>> On 10/17/17 10:44 AM, Jan Lahoda wrote:
>>>> The problematic part is TransitiveDependencies, right?
>>>>
>>>> Might be easier to replace:
>>>> JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
>>>> with:
>>>> JavaCompiler compiler = JavacTool.create();
>>>>
>>>> (import com.sun.tools.javac.api.JavacTool;)
>>>>
>>>> That should avoid the ServiceLoader lookup.
>>>>
>>>
>> I can try this instead of patching ToolProvider.
>>> Worth trying this and  I think this needs --limit-modules to hide
>>> jdk.compiler and other interim modules.
>> Yes, --limit-modules is definitely needed and already used.
>>>
>>> Does this build tool just depend on javac?  Does it need all system
>>> modules to be observable?
>>>
>> I'm currently running it with the same arguments as when I run
>> interim javac and it works:
>>
>> --limit-modules
>> java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim
>>
>> --add-modules
>> java.compiler.interim,jdk.compiler.interim,jdk.jdeps.interim,jdk.javadoc.interim
>>
>> --module-path
>> /localhome/hg/jdk10-boot9/build/linux-x64/buildtools/interim_modules
>> --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim
>> --add-exports java.base/jdk.internal.util.jar=jdk.jdeps.interim
>> --add-exports java.base/jdk.internal.misc=jdk.jdeps.interim
>>
>> Some of these may be unnecessary for this specific tool, but it is
>> certainly simpler from my point of view to just use the same set of
>> arguments instead of maintaining another set.
>>
>> /Erik
>>> Mandy
>>>
>>>> Jan
>>>>
>>>> On 17.10.2017 19:04, mandy chung wrote:
>>>>> It'd be simpler if we could avoid patching ToolProvider.   The ctsym
>>>>> generator is a build tool.  Jan and Jon may have suggestion if the
>>>>> build
>>>>> tool can find the compiler from the interim module instead and use
>>>>> --limit-modules to java.base?
>>>>>
>>>>> Mandy
>>>>>
>>>>> On 10/17/17 8:55 AM, Erik Joelsson wrote:
>>>>>>
>>>>>> The ctsym generator in Gendata-jdk.compiler.gmk looks up the tool
>>>>>> using the ToolProvider. That part of the change was just a
>>>>>> reaction to
>>>>>> ctsym generation failing.
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>
>>>>>> On 2017-10-17 17:47, mandy chung wrote:
>>>>>>> Hi Erik,
>>>>>>>
>>>>>>> On 10/16/17 6:12 AM, Erik Joelsson wrote:
>>>>>>>>
>>>>>>>> To generate the new modules, I copy the module-info.java files
>>>>>>>> to a
>>>>>>>> new gensrc dir and sed replace the module names. I also generate a
>>>>>>>> new ToolProvider.java so that the default tools are taken from the
>>>>>>>> interim modules.
>>>>>>>>
>>>>>>>> :
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8189094/webrev.01
>>>>>>>
>>>>>>> 65 java.compiler_EXTRA_FILES :=
>>>>>>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.compiler.interim/javax/tools/ToolProvider.java
>>>>>>>
>>>>>>>
>>>>>>> 66 TARGETS += $(java.compiler_EXTRA_FILES)
>>>>>>>
>>>>>>>
>>>>>>> What issue did you run into without patching ToolProvider? I have
>>>>>>> assumed
>>>>>>> the build always launches javac/javadoc main class and not using
>>>>>>> ToolProvider.
>>>>>>>
>>>>>>> 580 JAVAC_MAIN_CLASS = -m
>>>>>>> jdk.compiler.interim/com.sun.tools.javac.Main
>>>>>>> 581 JAVADOC_MAIN_CLASS = -m
>>>>>>> jdk.javadoc.interim/jdk.javadoc.internal.tool.Main Mandy
>>>>>>
>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189094: Change required boot jdk to JDK 9

Mandy Chung
In reply to this post by Erik Joelsson


On 10/18/17 2:29 AM, Erik Joelsson wrote:
> New webrev again: http://cr.openjdk.java.net/~erikj/8189094/webrev.03/
>

Looks fine  to me.

Thanks
Mandy