hg: jigsaw/jake/jdk: 2 new changesets
Changeset: 58f07216d2e7 Author:mchung Date: 2016-11-22 15:32 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/58f07216d2e7 Fix test to check ACC_MODULE before getting this_class name ! test/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java Changeset: 8aecfe7850e9 Author:alanb Date: 2016-11-22 15:33 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/8aecfe7850e9 Revise Module attribute per #ClassFileModuleName ! src/java.base/share/classes/com/sun/java/util/jar/pack/intrinsic.properties ! src/java.base/share/classes/java/lang/module/ModuleInfo.java ! src/java.base/share/classes/jdk/internal/module/ClassFileAttributes.java ! src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java ! src/java.base/share/classes/jdk/internal/org/objectweb/asm/ClassWriter.java
hg: jigsaw/jake/langtools: 2 new changesets
Changeset: 0ade799cfe9d Author:mchung Date: 2016-11-22 15:33 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/0ade799cfe9d Check ACC_MODULE before getting this_class name ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java Changeset: ec19be6d1e19 Author:jjg Date: 2016-11-22 15:34 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/ec19be6d1e19 Revise Module attribute per #ClassFileModuleName ! src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassFile.java ! src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java ! src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java ! src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ModuleNameReader.java ! src/jdk.jdeps/share/classes/com/sun/tools/classfile/ClassWriter.java ! src/jdk.jdeps/share/classes/com/sun/tools/classfile/Module_attribute.java ! src/jdk.jdeps/share/classes/com/sun/tools/javap/AttributeWriter.java ! src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java ! test/tools/javap/4870651/T4870651.java ! test/tools/javap/MethodParameters.java ! test/tools/javap/T4975569.java
hg: jigsaw/jake/jdk: Fix FieldSetAccessibleTest test to work with setAccessible
Changeset: 6a19edab7f47 Author:mchung Date: 2016-11-22 16:38 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/6a19edab7f47 Fix FieldSetAccessibleTest test to work with setAccessible ! test/ProblemList.txt ! test/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java
Re: Review Request: JDK-8169816 Move src.zip and jrt-fs.jar under the lib directory
> On Nov 23, 2016, at 6:05 AM, Alan Bateman wrote:
>
>
>
> On 22/11/2016 21:07, Mandy Chung wrote:
>> This patch moves src.zip and jrt-fs.jar from the top-level into
>> the `lib` directory in the run-time image as we proposed [1].
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169816/webrev.00/
>>
>>
> This looks good. A minor point in SystemImage.findHome is that it's probably
> an InternalError if we are somehow loaded from a jrt-fs.jar that is not in
> the lib directory.
Agree. The existing check if non “file” URL should also throw InternalError
instead. Fixed.
@@ -113,12 +113,16 @@
if (cs == null)
return System.getProperty("java.home");
-// assume loaded from $TARGETJDK/jrt-fs.jar
+// assume loaded from $TARGETJDK/lib/jrt-fs.jar
URL url = cs.getLocation();
if (!url.getProtocol().equalsIgnoreCase("file"))
-throw new RuntimeException(url + " loaded in unexpected way");
+throw new InternalError(url + " loaded in unexpected way");
try {
-return Paths.get(url.toURI()).getParent().toString();
+Path lib = Paths.get(url.toURI()).getParent();
+if (!lib.getFileName().toString().equals("lib"))
+throw new InternalError(url + " unexpected path");
+
+return lib.getParent().toString();
} catch (URISyntaxException e) {
throw new InternalError(e);
}
Mandy
hg: jigsaw/jake/jdk: jar tool to handle entries with META-INF/versions path
Changeset: c0170badab7d Author:mchung Date: 2016-11-24 13:51 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/c0170badab7d jar tool to handle entries with META-INF/versions path ! src/java.base/share/classes/module-info.java ! src/jdk.jartool/share/classes/sun/tools/jar/Main.java ! src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties + test/tools/jar/multiRelease/Basic1.java
hg: jigsaw/jake/jdk: 3 new changesets
Changeset: e1d07d9c9e64 Author:mchung Date: 2016-11-24 22:30 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/e1d07d9c9e64 Remove temporary copying of copyright/license to jmod ! make/copy/Copy-java.base.gmk ! make/copy/CopyCommon.gmk Changeset: 673f6b89cd09 Author:mchung Date: 2016-11-24 22:31 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/673f6b89cd09 Clean up jlink system module plugin code ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/ResourcePoolEntry.java Changeset: b20b41370415 Author:mchung Date: 2016-11-24 22:32 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/b20b41370415 --list-modules to print automatic module ! src/java.base/share/classes/sun/launcher/LauncherHelper.java
hg: jigsaw/jake/jdk: Pre-generating ModuleDescriptor hashCode has direct and indirect benefits to startup
Changeset: 01f184198a68 Author:redestad Date: 2016-11-27 14:48 +0100 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/01f184198a68 Pre-generating ModuleDescriptor hashCode has direct and indirect benefits to startup Reviewed-by: alanb, mchung ! src/java.base/share/classes/java/lang/module/ModuleDescriptor.java ! src/java.base/share/classes/jdk/internal/misc/JavaLangModuleAccess.java ! src/java.base/share/classes/jdk/internal/module/Builder.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java
hg: jigsaw/jake: requires jtreg 4.2 b04
Changeset: 51f97296402c Author:mchung Date: 2016-11-28 21:43 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/rev/51f97296402c requires jtreg 4.2 b04 ! common/conf/jib-profiles.js
hg: jigsaw/jake/jdk: requires jtreg 4.2 b04
Changeset: b7ddd95fd610 Author:mchung Date: 2016-11-28 21:42 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/b7ddd95fd610 requires jtreg 4.2 b04 ! test/TEST.ROOT
hg: jigsaw/jake/langtools: requires jtreg 4.2 b04
Changeset: dd31ba802575 Author:mchung Date: 2016-11-28 21:43 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/dd31ba802575 requires jtreg 4.2 b04 ! test/TEST.ROOT
hg: jigsaw/jake/jaxp: requires jtreg 4.2 b04
Changeset: a264ad1c7139 Author:mchung Date: 2016-11-28 21:43 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jaxp/rev/a264ad1c7139 requires jtreg 4.2 b04 ! test/TEST.ROOT
hg: jigsaw/jake/hotspot: requires jtreg 4.2 b04
Changeset: e9fe2fec61f7 Author:mchung Date: 2016-11-28 21:43 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/hotspot/rev/e9fe2fec61f7 requires jtreg 4.2 b04 ! test/TEST.ROOT
hg: jigsaw/jake/nashorn: requires jtreg 4.2 b04
Changeset: 527b3a15b412 Author:mchung Date: 2016-11-28 21:43 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/nashorn/rev/527b3a15b412 requires jtreg 4.2 b04 ! test/TEST.ROOT
hg: jigsaw/jake/hotspot: Remove blank line - review comment from Lois
Changeset: 85448216fe28 Author:mchung Date: 2016-11-29 13:36 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/hotspot/rev/85448216fe28 Remove blank line - review comment from Lois ! src/share/vm/classfile/javaClasses.cpp
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Thanks Lois. I removed the blank line. Mandy > On Nov 28, 2016, at 6:32 AM, Lois Foltan wrote: > > Hi Alan, > > I have reviewed the hotspot changes and they look good. Minor nit, > src/share/vm/classfile/javaClasses.cpp only differs by the addition of a > blank line. > > Thanks, > Lois > > On 11/24/2016 10:25 AM, Alan Bateman wrote: >> Folks on jigsaw-dev will know that we are on a mission to bring the changes >> accumulated in the jake forest to jdk9/dev. We can think of this as a >> refresh of the module system in JDK 9, the last big refresh was in May with >> many small updates since then. >> >> The focus this time is to bring the changes that are tied to JSR issues into >> jdk9/dev, specifically the issues that are tracked on the JSR issues list >> [1] as: >> >> #CompileTimeDependences >> #AddExportsInManifest >> #ClassFileModuleName >> #ClassFileAccPublic >> #ServiceLoaderEnhancements >> #ResourceEncapsulation/#ClassFilesAsResources >> #ReflectiveAccessToNonExportedTypes >> #AwkwardStrongEncapsulation >> #ReadabilityAddedByLayerCreator >> #IndirectQualifiedReflectiveAccess (partial) >> #VersionsInModuleNames >> #NonHierarchicalLayers >> #ModuleAnnotations/#ModuleDeprecation >> #ReflectiveAccessByInstrumentationAgents >> >> Some of these issues are not "Resolved" yet, meaning there is still ongoing >> discussion on the EG mailing list. That is okay, there is nothing final >> here. If there are changes to these proposals then the implementation >> changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], >> is that we will keep the jake forest open for ongoing prototyping and >> iteration, also ongoing implementation improvements where iteration or bake >> time is important. >> >> For the code review then the focus is therefore on sanity checking the >> changes that we would like to bring into jdk9/dev. We will not use this >> review thread to debate alternative designs or other big implementation >> changes that are more appropriate to bake in jake. >> >> To get going, I've put the webrevs with a snapshot of the changes in jake >> here: >>http://cr.openjdk.java.net/~alanb/8169069/0/ >> >> The changes are currently sync'ed against jdk-9+146 and will be rebased (and >> re-tested) against jdk9/dev prior to integration. There are a number of >> small changes that need to be added to this in the coming days, I will >> refresh the webrev every few days to take account of these updates. >> >> >> A few important points to mention, even if you aren't reviewing the changes: >> >> 1. This refresh requires a new version of jtreg to run the tests. The >> changes for this new version are in the code-tools/jtreg repository and the >> plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been >> added then we'll update the requiredVersion property in each TEST.ROOT to >> force everyone to update. >> >> 2. For developers trying out modules with the main line JDK 9 builds then be >> aware that `requires public` changes to `requires transitive` and the >> `provides` clause changes to require all providers for a specific service >> type to be in the same clause. Also be aware that the binary form of the >> module declaration (module-info.class) changes so you will need to recompile >> any modules. >> >> 3. Those running existing code on JDK 9 and ignoring modules will need to be >> aware of a disruptive change in this refresh. The disruptive change is >> #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it >> can't be used to break into non-public fields/methods of JDK classes. This >> change is going to expose a lot of hacks in existing code. We plan to send >> mail to jdk9-dev in advance of this integration to create awareness of this >> change. As per the original introduction of strong encapsulation then >> command line options (and now the manifest of application JAR files) can be >> used to keep existing code working. The new option is `--add-opens` to open >> a package in a module for deep reflection by other modules. As an example, >> if you find yourself with code that hacks into the private `comparator` >> field in java.util.TreeMap then running with `--add-opens >> java.base/java.util=ALL-UNNAMED` will keep that code working. >> >> >> A few miscellaneous notes for those that are reviewing: >> >> 1. We have some temporary/transition code in the top-level repo to deal with >> the importing of the JavaFX modules. This will be removed once the changes >> are in JDK 9 for the OpenJFX project to use. >> >> 2. In the jdk repo then it's important to understand that the module system >> is initialized at startup and there are many places where we need to keep >> startup performance in mind. This sometimes means less elegant code than >> might be used if startup wasn't such a big concern. >> >> 3. The changes in the jaxws repo make use of new APIs that means the code >> doesn't compile with
hg: jigsaw/jake/jdk: Update ResourceBundle to find bundles visible to the class loader
Changeset: 46739df435c8 Author:mchung Date: 2016-11-30 13:49 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/46739df435c8 Update ResourceBundle to find bundles visible to the class loader ! src/java.base/share/classes/java/util/ResourceBundle.java ! src/java.base/share/classes/java/util/spi/AbstractResourceBundleProvider.java ! src/java.base/share/classes/jdk/internal/misc/JavaUtilResourceBundleAccess.java ! src/java.base/share/classes/jdk/internal/misc/SharedSecrets.java ! src/java.base/share/classes/module-info.java + src/java.base/share/classes/sun/security/util/AuthResourcesProvider.java + src/java.base/share/classes/sun/security/util/AuthResourcesProviderImpl.java - src/java.base/share/classes/sun/util/locale/provider/ResourceBundleProviderSupport.java ! src/java.base/share/classes/sun/util/resources/Bundles.java ! src/java.logging/share/classes/java/util/logging/Level.java ! src/java.logging/share/classes/java/util/logging/Logger.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/SolarisNumericGroupPrincipal.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/SolarisNumericUserPrincipal.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/SolarisPrincipal.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/X500Principal.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/module/JndiLoginModule.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/module/KeyStoreLoginModule.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/module/Krb5LoginModule.java ! src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java ! src/jdk.security.auth/share/classes/module-info.java ! test/java/util/ResourceBundle/modules/modlocal/modlocal.sh ! test/java/util/ResourceBundle/modules/visibility/visibility.sh ! test/java/util/logging/LocalizedLevelName.java ! test/java/util/logging/modules/pkgs/p3/test/ResourceBundleTest.java ! test/sun/tools/jconsole/ResourceCheckTest.java
Re: RFR 8160359: Improve jlink logging for cases when a plugin throws exception
> On Nov 30, 2016, at 3:05 AM, Sundararajan Athijegannathan
> wrote:
>
> Please review fix for https://bugs.openjdk.java.net/browse/JDK-8160359
>
> jdk webrev: http://cr.openjdk.java.net/~sundar/8160359/webrev.01/
I expect JlinkTask::run method be updated such that it will always print the
stack trace when PluginException is caught rather than just printing the
message only.
ImagePluginStack.java
273 } catch (PluginException pe) {
274 if (JlinkTask.DEBUG) {
275 System.err.println("Plugin " + p.getName() + " threw
exception during transform");
276 pe.printStackTrace();
277 }
278 throw pe;
It might be useful for PluginException to take a plugin name in the constructor
so that the exception message is always prepended with the plugin name to help
troubleshooting. If JlinkTask::run is changed as suggested above, is line
273-278 still needed?
454 if (JlinkTask.DEBUG) {
455 System.err.println("IOException while reading
resource: " + res.path());
456 ex.printStackTrace();
457 }
458 throw new PluginException(ex);
Same as above comment and also PluginException should include res.path() in the
message.
As Alan mentioned, make/InterimImage.gmk should also be updated.
Mandy
hg: jigsaw/jake/jdk: Fix typo and formatting nit
Changeset: 08f950844aab Author:mchung Date: 2016-11-30 16:52 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/08f950844aab Fix typo and formatting nit ! src/java.base/share/classes/java/lang/module/ModuleDescriptor.java ! src/java.base/share/classes/java/lang/reflect/Layer.java
hg: jigsaw/jake/jdk: 2 new changesets
Changeset: 0a41a3f8b890 Author:mchung Date: 2016-11-30 18:00 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/0a41a3f8b890 Minor cleanup: remove unused local variable ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java Changeset: f42eb43b30d3 Author:mchung Date: 2016-11-30 18:03 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/f42eb43b30d3 Fix jmod/jlink/jar w.r.t. the opens directive ! src/jdk.jartool/share/classes/sun/tools/jar/Main.java ! src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ResourcePoolConfiguration.java ! src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
hg: jigsaw/jake/langtools: Filter open packages when calling Builder::contains
Changeset: 3f46df92ca42 Author:mchung Date: 2016-11-30 17:57 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/3f46df92ca42 Filter open packages when calling Builder::contains ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsConfiguration.java ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/Module.java
Re: [9] Review request: JDK-8170485: Switch to building JavaFX with new module-info syntax
> On Dec 6, 2016, at 8:10 AM, Kevin Rushforth > wrote: > > Chien & Dave, > > Please review the preliminary webrev to allow building JavaFX with jdk-9+148 > and later: > > https://bugs.openjdk.java.net/browse/JDK-8170485 > http://cr.openjdk.java.net/~kcr/8170485/webrev.00/ > > The details are in the JBS issue and also in the "HEADS-UP" message [1] I > sent yesterday. > > As indicated in JBS, I will update the webrev later this week to bump the > minimum build to 148 once 148 is promoted and available on java.net. No other > changes are anticipated (unless you find anything while reviewing it). Looks good. Mandy
Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
This proposes to organize license files by module in source, JMOD,
and run-time image.
A summary of the proposal:
1. Organize third party notices by module in the source as follows:
src/$MODULE/{share,$OS}/legal/*
The `legal` directory contains one file for each third party
library in the module, for example,
src/java.base/share/legal/asm.md
unicode.md
zlib.md
The proposed template for this file is described in [1] and JEP 201
will be updated to reflect this proposed source layout.
2. Introduce a new LEGAL_NOTICES section in JMOD format. A new jmod
option `—-legal-notices` is added to package legal notices in
a JMOD file.
3. At jlink time, jlink will copy all legal notices from JMOD files
to the `legal` directory in the run-time image. A plugin is
added to de-duplicate the legal notices if the filename and the
content matches that may reduce the image footprint.
4. THIRD_PARTY_README in the top-level directory of each repo is removed.
Manual edit to this file, multiple copies is no longer needed.
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8169925
Re: RFR [9] 8166568 & 8169492 jmod extract and bug fix
> On Dec 6, 2016, at 2:46 AM, Chris Hegarty wrote: > > This change adds a basic option to the jmod tool to extract all its contents > to > the current working directory, 8166568 [1]. Additionally, there is a bug fix > for > a public mutable static, 8169492 [2]. > > http://cr.openjdk.java.net/~chegar/8166568_8169492.00/ Looks good. I agree with Alan and it’d be good to take the destination directory to which the contents are written. FYI. jimage extract command takes —-dir option. Mandy
Re: Java module graph png settings?
Hi Patrick Are you looking for: http://hg.openjdk.java.net/jdk9/dev/jdk/file/c9785b0f04fd/make/src/classes/build/tools/jigsaw/GenGraphs.java Mandy > On Dec 7, 2016, at 1:17 PM, Patrick Reinhart wrote: > > Can anyone point me to the place, where the java module dependencies are > created? I would like to do something similar and I curious how it’s being > done exactly… > > Thanks very much > > -Patrick
Re: RFR [9] 8166568 & 8169492 jmod extract and bug fix
> On Dec 8, 2016, at 5:09 AM, Chris Hegarty wrote: > > > Updated webrev contain a target destination dir: > http://cr.openjdk.java.net/~chegar/8166568_8169492.01/ > Looks fine (including the separate patch to JmodTest.java). Mandy
Re: Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
> On Dec 8, 2016, at 1:44 AM, Erik Joelsson wrote:
>
> Hello Mandy,
>
> In autoconf you are also adding a "man" dir, is this intended with this
> change?
>
Strictly speaking this is a separate issue but piggyback it here. This may be
used when building JDK with JavaFX to import a man page from JavaFX module.
> In make/InterimImage.gmk, -J-Djlink.debug=true looks like left over debug
> code.
This is very useful for troubleshooting (that was missed in the changeset of
JDK-8160359) until my other suggestion to jlink is implemented (always to print
the stack trace for PluginException).
>
> make/CreateJmods.gmk: 83-84, please indent 4 spaces for continuation.
>
> make/Main.gmk: You should not add the dependency there. Instead, add
> "$(JMOD_TARGETS): java.base-copy" around line 661.
>
> Copy-java.base.gmk: 246, please finish these kinds of lists with a # indented
> to the same level as the list of values.
>
Will fix the above.
Thanks
Mandy
> /Erik
>
>
> On 2016-12-07 22:28, Mandy Chung wrote:
>> This proposes to organize license files by module in source, JMOD,
>> and run-time image.
>>
>> A summary of the proposal:
>> 1. Organize third party notices by module in the source as follows:
>> src/$MODULE/{share,$OS}/legal/*
>> The `legal` directory contains one file for each third party
>> library in the module, for example,
>> src/java.base/share/legal/asm.md
>> unicode.md
>> zlib.md
>>
>> The proposed template for this file is described in [1] and JEP 201
>> will be updated to reflect this proposed source layout.
>>
>> 2. Introduce a new LEGAL_NOTICES section in JMOD format. A new jmod
>>option `—-legal-notices` is added to package legal notices in
>>a JMOD file.
>>
>> 3. At jlink time, jlink will copy all legal notices from JMOD files
>>to the `legal` directory in the run-time image. A plugin is
>>added to de-duplicate the legal notices if the filename and the
>>content matches that may reduce the image footprint.
>>
>> 4. THIRD_PARTY_README in the top-level directory of each repo is removed.
>>Manual edit to this file, multiple copies is no longer needed.
>>
>> Webrev at:
>>http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/
>>
>> Mandy
>> [1] https://bugs.openjdk.java.net/browse/JDK-8169925
>
Re: Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
> On Dec 8, 2016, at 2:58 AM, Magnus Ihse Bursie > wrote: > > Hi Mandy, > > Some comments. > > * In CreateJmods.gmk, you remove the TODO about headers, but I don't see a > resolution to that issue here. If you have opened a bug report for that > instead, please let me know the bug ID. > A new JMOD section was introduced to handle header files and man pages in JDK-8167558. See http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-October/009654.html I realized now that it was sent to jigsaw-dev only (sorry for missing build-dev). > * The definitions of JDK/JMOD_COPYRIGHT/LICENSE in Modules.gmk seems to be > only used in Copy-java.base.gmk. Please move them there. > Will fix. > * jdk/src/java.base/share/legal/asm.md seems to have messed up the character > set, it's copyright by "France Télécom". Possibly this is a webrev issue, > but please make sure it's correct before pushing. (I have just looked at a > few license files at random, so maybe you should double-check all of them to > make sure there are no other character encoding issues.) > Good catch. Will double check all of them. > * Overall, I'm a bit confused about the ".md" suffix on the license files. > This seem to indicate that the files are in markdown format. But from what I > can tell, they are just the same plain text licenses as before. Several of > them, contains text that will be interpreted as markdown markup codes, but > will probably result in hideous looks since it's not intentionally markdown > formatted. > > It seems that the licenses have a header with properly formatted markdown > prepended to them. I strongly suggest that you at the very least put the > original license text in code blocks (``` ```). > We will take another pass on these files. I agree that the original license text should be code blocks. > Also, is this markdown supposed to be processed into another format? I could > find no trace of such processing in the code. Not in the images build. This enables a user to run tools like pandoc to aggregate these files and generate a separate document for example one single HTML file seeing all third party licenses or other format. Mandy
Re: Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
> On Dec 8, 2016, at 9:43 AM, Alan Bateman wrote: > > On 07/12/2016 21:28, Mandy Chung wrote: > >> : >> >> Webrev at: >>http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/ >> >> > I went over the changes to the jmod tool, jlink, and the new plugin to dedup > legal notices. > > It looks straight-forward, I just wondering about the sym links hat > DefaultImageBuilder creates. I think this is the first sym link that a jlink > created image will have and just wondering if this could be problematic when > run time images are packaged up and transported to different platforms. I > guess time will tell. It creates sym link only when the target platform is not Windows and the host platform supports it. We will find out if it turns out to be problematic. Mandy
Re: RFR 8168925: MODULES property should be topologically ordered and space-separated list
> On Dec 9, 2016, at 12:49 AM, Sundararajan Athijegannathan
> wrote:
>
> Please review http://cr.openjdk.java.net/~sundar/8168925/webrev.01/index.html
> for https://bugs.openjdk.java.net/browse/JDK-8168925
>
Is the order of Plugin.Category enums significant? You moved COMPRESSOR down -
is it necessary?
Can you look at DefaultImageBuilder::releaseProperties which I think this
should be moved to ReleaseInfoPlugin? The content of `release` should be
written when the new entry "/java.base/release” is added to the resource pool.
DefaultImageBuilder does not need to add any more properties to this `release`
file as all properties in `release` are known once the graph is resolved.
550 String[] arr = mods.split(" ");
I think you will need to remove the double-quotes before doing the split.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ResourcePoolManager.java
- is this change needed?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/ReleaseInfoPlugin.java
137 release.put("MODULES", "\"" + new ModuleSorter(in.moduleView())
138
.sorted().map(ResourcePoolModule::name).collect(Collectors.joining(" "))
139 + "\"”);
Since there is always at least one module, you can use Collectors.joining(“ “,
"\””, "\””) instead of the prepending and appending.
test/tools/jlink/CustomPluginTest.java
It’s one option to disable the releaseinfo plugin. An alternative
way to fix this test is [2].
test/tools/jlink/ModuleNamesOrderTest.java
57 String moduleName = "bug8168925”;
Is it the output dir name? This is not a module name. @bug has the bug id. I
suggest to rename this to “image” or some other name.
One suggestion to the validation is to remove the double quotes from MODULES
property value and split it into an array. That may be a better way to verify
the dependences.
> PS. Mandy Chung wrote ModuleSorter for another fix (yet to be pushed). I'm
> using it for this fix after discussion with her (private email).
ModuleSorter is part of the patch for JDK-8169925[1]. I will take it out if
Sundar pushes this changeset before JDK-8169925.
Mandy
[1]
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010416.html
[2]
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.00/jdk/test/tools/jlink/CustomPluginTest.java.udiff.html
Re: Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
Erik, Magnus, I have made the change per your suggestion: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8169925/webrev.01/ Jeannette, Jeff, and I went through all .md files and I think asm.md is the only one got the characters messed up. These files are contributed by Jeannette. Also, per Naoto, cldr.md are now in both java.base and jdk.localedata. thanks Mandy
Re: RFR 8168925: MODULES property should be topologically ordered and space-separated list
> On Dec 9, 2016, at 7:32 PM, Sundararajan Athijegannathan > wrote: > > Hi, > > Thanks for your review. Comments below.. > > On 10/12/16, 2:18 AM, Mandy Chung wrote: >>> On Dec 9, 2016, at 12:49 AM, Sundararajan >>> Athijegannathan wrote: >>> >>> Please review >>> http://cr.openjdk.java.net/~sundar/8168925/webrev.01/index.html for >>> https://bugs.openjdk.java.net/browse/JDK-8168925 >>> >> Is the order of Plugin.Category enums significant? You moved COMPRESSOR >> down - is it necessary? > > Yes. Without that ReleaseInfoPlugin will receive compressed resources and > compressed module-info.class won't be parsed okay by ModuleDescriptor.read > (called by ModuleSorter). Note that auto-decompression is done only by > LastResourcePool - which is created after all plugins operate. [I kept > getting test failures and debugged to find this is the cause of failures!] > OK. If the order of the enums determines the plugin ordering, it’d be good adding a comment. >> >> Can you look at DefaultImageBuilder::releaseProperties which I think this >> should be moved to ReleaseInfoPlugin? The content of `release` should be >> written when the new entry "/java.base/release” is added to the resource >> pool. DefaultImageBuilder does not need to add any more properties to this >> `release` file as all properties in `release` are known once the graph is >> resolved. > Okay, I'll check if I can move all stuff there. From initial communication > [private email], I thought the suggestion was only about MODULES. This has beeen my suggestion - move the whole thing out from DefaultImageBuilder. I don’t see the reason why it has to be done in two separate places. That’ll be a good clean up. >> >> >> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ResourcePoolManager.java >> - is this change needed? > > Yes. Helped me debugging to find out which module's module-info.class was not > parsed fine. Exception translation puts the name of the module in the message > - which will be missing in the exception thrown by > ModuleDescriptor.read(ByteBuffer). OK. > >> >> test/tools/jlink/CustomPluginTest.java >>It’s one option to disable the releaseinfo plugin. An alternative >>way to fix this test is [2]. > > I could. But that test checks for the module dependency checks that happen > after all plugins are exercised. That should happen regardless of > release-info plugin is enabled. i.e., even when release-info plugin is > disabled, that module missing error should be thrown and the test checks for > that. > OK. If I happen to push JDK-8169925 before this, you can revert my change to this test. Thanks Mandy
Re: Java module graph png settings?
dot -Tpng Mandy > On Dec 9, 2016, at 4:58 PM, Patrick Reinhart wrote: > > Hi Mandy, > > Can you tell me what options with the Graphviz framework are to get the > actual PNG file out of the jdk.dot file? I tried to find this, but did not > find the actual place. > > -Patrick > > >> Am 08.12.2016 um 07:18 schrieb Mandy Chung : >> >> Hi Patrick >> >> Are you looking for: >> >> http://hg.openjdk.java.net/jdk9/dev/jdk/file/c9785b0f04fd/make/src/classes/build/tools/jigsaw/GenGraphs.java >> >> Mandy >> >>> On Dec 7, 2016, at 1:17 PM, Patrick Reinhart wrote: >>> >>> Can anyone point me to the place, where the java module dependencies are >>> created? I would like to do something similar and I curious how it’s being >>> done exactly… >>> >>> Thanks very much >>> >>> -Patrick >> >
Re: Review Request JDK-8169925: Organize licenses by module in source, JMOD file, and run-time image
> On Dec 9, 2016, at 11:51 PM, Erik Joelsson wrote: > > Hello Mandy, > > In Copy-java.base.gmk. Modules.gmk is no longer needed. JMOD_* varaibles are > no longer used. > Good catch. > In Main.gmk, it would be good with a comment explaining why the jmods need > java.base-copy. Something like: > > # All modules include the main license files from java.base. > > Otherwise I'm ok with the build changes now. Updated per your comment: $ hg diff make/copy/Copy-java.base.gmk diff --git a/make/copy/Copy-java.base.gmk b/make/copy/Copy-java.base.gmk --- a/make/copy/Copy-java.base.gmk +++ b/make/copy/Copy-java.base.gmk @@ -233,3 +233,17 @@ endif + +# JDK license and assembly exception files to be packaged in JMOD + +JDK_LICENSE ?= $(JDK_TOPDIR)/LICENSE +JDK_NOTICE ?= $(JDK_TOPDIR)/ASSEMBLY_EXCEPTION + +$(eval $(call SetupCopyFiles, COPY_JDK_NOTICES, \ +FILES := $(JDK_LICENSE) $(JDK_NOTICE), \ +DEST := $(LEGAL_DST_DIR), \ +FLATTEN := true, \ +)) + +TARGETS += $(COPY_JDK_NOTICES) + $ hg diff make/Main.gmk diff --git a/make/Main.gmk b/make/Main.gmk --- a/make/Main.gmk +++ b/make/Main.gmk @@ -659,6 +659,9 @@ exploded-image-optimize endif + # All modules include the main license files from java.base. + $(JMOD_TARGETS): java.base-copy + zip-security: java.base-java java.security.jgss-java java.security.jgss-libs \ $(filter jdk.crypto%, $(JAVA_TARGETS))
Re: RFR 8168925: MODULES property should be topologically ordered and space-separated list
> On Dec 11, 2016, at 9:30 PM, Sundararajan Athijegannathan > wrote: > > Hi, > > Please review the updated webrev: > http://cr.openjdk.java.net/~sundar/8168925/webrev.02/ This looks better. Thanks for making the change. One thing to mention is that the storeRelease(ResourcePool pool) method to write the release file because the accept(ResourcePoolEntry file) method excludes “/java.base/release” entry: 384 case TOP: 385 break; This release file entry could have been written in the same way as other entries now and no need for this storeRelease method. I’m okay if you want to leave this as is and revisit the TOP and OTHER entry type in the future. Mandy
hg: jigsaw/jake/jdk: Restore system modules jlink plugin to work with hashes attribute
Changeset: ec901a312639 Author:mchung Date: 2016-12-12 20:09 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/ec901a312639 Restore system modules jlink plugin to work with hashes attribute ! src/java.base/share/classes/java/lang/module/ModuleFinder.java ! src/java.base/share/classes/jdk/internal/module/ModuleHashes.java ! src/java.base/share/classes/jdk/internal/module/SystemModuleFinder.java ! src/java.base/share/classes/jdk/internal/module/SystemModules.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java < src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java ! src/jdk.jlink/share/classes/module-info.java
hg: jigsaw/jake/jdk: System modules jlink plugin support for compiled version
Changeset: 0757b2d2968d Author:mchung Date: 2016-12-13 21:10 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/0757b2d2968d System modules jlink plugin support for compiled version ! src/java.base/share/classes/jdk/internal/module/Builder.java ! src/java.base/share/classes/jdk/internal/module/SystemModuleFinder.java ! src/java.base/share/classes/jdk/internal/module/SystemModules.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java + test/tools/jlink/plugins/SystemModuleDescriptors/CompiledVersionTest.java + test/tools/jlink/plugins/SystemModuleDescriptors/src/test/jdk/test/Main.java + test/tools/jlink/plugins/SystemModuleDescriptors/src/test/module-info.java
Review Request: JDK-8171201 & JDK-8171202: Drop java.compact$N aggregator modules
JDK-8171201: Drop java.compact$N aggregator modules JDK-8171202: Rename jdk.crypto.pkcs11 and jdk.pack200 to end with Java letters http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171201%2b8171202/webrev.00/ Compact Profiles were introduced in SE 8. The java.compact$N aggregator module in SE 9 module graph gives unnecessary importance going forward. We want people who care about small systems to think about modules, not profiles. In addition, the compact profile builds use these aggregator modules but those builds still have to pull in the necessary service providers manually, so they aren't a complete solution. We propose to drop java.compact1, java.compact2, java.compact3 aggregator modules. This patch also includes the change for JDK-8171202 to rename jdk.crypto.pkcs11 to jdk.crypto.token and jdk.pack200 to jdk.pack to prepare the stronger constraint that module names must end in Java letters [1] Mandy [1] http://mail.openjdk.java.net/pipermail/jpms-spec-observers/2016-December/000682.html
Re: Review Request: JDK-8171201 & JDK-8171202: Drop java.compact$N aggregator modules
> On Dec 14, 2016, at 12:03 AM, Alan Bateman wrote:
>
> On 14/12/2016 06:49, Mandy Chung wrote:
>
>> JDK-8171201: Drop java.compact$N aggregator modules
>> JDK-8171202: Rename jdk.crypto.pkcs11 and jdk.pack200 to end with Java
>> letters
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171201%2b8171202/webrev.00/
>>
>>
> The hg moves are showing up as new files in the webrev - is that because the
> webrev is on several repos?
>
Not sure and it might be webrev on several repos.
The number of lines changed is zero which is correct.
> In the LimitModsTest test then it might be simpler to leave java.scripting
> out of the test.
Sure.
$ hg diff limitmods/LimitModsTest.java
diff --git a/test/tools/launcher/modules/limitmods/LimitModsTest.java
b/test/tools/launcher/modules/limitmods/LimitModsTest.java
--- a/test/tools/launcher/modules/limitmods/LimitModsTest.java
+++ b/test/tools/launcher/modules/limitmods/LimitModsTest.java
@@ -24,7 +24,7 @@
/**
* @test
* @library /lib/testlibrary
- * @modules java.desktop java.compact1 jdk.compiler
+ * @modules java.desktop java.logging jdk.compiler
* @build LimitModsTest CompilerUtils jdk.testlibrary.*
* @run testng LimitModsTest
* @summary Basic tests for java --limit-modules
@@ -83,13 +83,12 @@
assertTrue(exitValue == 0);
-// java --limit-modules java.compact1 --list-modules
-exitValue = executeTestJava("--limit-modules", "java.compact1",
"--list-modules")
+// java --limit-modules java.logging --list-modules
+exitValue = executeTestJava("--limit-modules", "java.logging",
"--list-modules")
.outputTo(System.out)
.errorTo(System.out)
.shouldContain("java.base")
.shouldContain("java.logging")
-.shouldContain("java.compact1")
.shouldNotContain("java.xml")
.getExitValue();
> Also in the java.se aggregator then it looks like the module comment has a
> line break after "The module defining" and can probably be fixed as part of
> this edit.
>
Fixed.
/**
* Defines the core Java SE API.
*
* The modules defining CORBA and Java EE APIs are not required by
* this module, but they are required by {@code java.se.ee}.
*/
Mandy
Re: RFR 8171138: Remove FileCopierPlugin
> On Dec 14, 2016, at 9:02 AM, Sundararajan Athijegannathan
> wrote:
>
> Please review http://cr.openjdk.java.net/~sundar/8171138/webrev.00/ for
> https://bugs.openjdk.java.net/browse/JDK-8171138
>
> Piggybacking cleanup: release file generation ("TOP" entry type handling)
> moved to accept(ResourcePoolEntry)
>
The removal of FileCopierPlugin looks fine.
The image builder writes out all entries from the ResourcePool.
`release` file is already an entry added to ResourcePool and the content
is known when the entry is created. The image builder could just write out
`release` file in the same way as all other entries (i.e. with
ResourcePoolEntry::contents). A related question is whether we want to keep
TOP type going forward. You could either leave the `release` file generation
change out, leave it as is, or write the content when the entry is created in
ReleaseInfoPlugin (i.e. storeRelease method can also be removed).
Mandy
hg: jigsaw/jake/jdk: fix formatting nit
Changeset: e0558b224fdf Author:mchung Date: 2016-12-14 16:44 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/e0558b224fdf fix formatting nit ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
hg: jigsaw/jake/jdk: Fix up @modules in incubator and jlink plugin tests
Changeset: 3d3212804cd8 Author:mchung Date: 2016-12-14 17:00 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/3d3212804cd8 Fix up @modules in incubator and jlink plugin tests ! test/jdk/modules/incubator/ImageModules.java ! test/tools/jlink/plugins/SystemModuleDescriptors/CompiledVersionTest.java ! test/tools/jlink/plugins/SystemModuleDescriptors/UserModuleTest.java
Re: 8170987: Module system implementation refresh (12/2016)
>
> The webrevs with the changes for this update are here:
>
> http://cr.openjdk.java.net/~alanb/8170987/1
I have pushed the change to rename jdk.crypto.pkcs11 and jdk.pack200
and dropped java.compact$N. So module-info.java changes will not be
needed when you sync with jdk9/dev.
I reviewed all changes except javac/javadoc changes. Looks good in
general.
src/java.base/share/classes/jdk/internal/module/Checks.java
115 /**
116 * Returns {@code true} if the given name is a legal binary name.
117 */
118 public static boolean isJavaIdentifier(String name) {
Not sure if it’s intended to have the javadoc for isJavaIdentifier
method be the same as isBinaryName.
When we use —-module-version for user modules, the runtime will load
regex. The system modules jlink plugin uses the cached version if
JDK modules to be compiled with —0module-version in the future.
This might be something we should look at in the future for performance.
src/java.base/share/classes/jdk/internal/module/ModuleResolution.java
64 throw new RuntimeException("cannot add deprecated to " +
value);
This comment applies to ModuleResoluton::with* methods. This should
probably be an InternalError?
108 return String.valueOf(value);
Nit: since you override toString method, might be helpful to print
an informative description.
src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
1102 if (value.equals("deprecated"))
1103 return (new ModuleResolution(0)).withDeprecated();
1104 else if (value.equals("deprecated-for-removal"))
1105 return (new
ModuleResolution(0)).withDeprecatedForRemoval();
1106 else if (value.equals("incubating"))
1107 return (new ModuleResolution(0)).withIncubating();
Why not passing the flag to ModuleResolution constructor? Similar
statement is also in sun/tools/jar/GNUStyleOptions.java.
I was wondering if jmod describe and jar —-print-module-descriptor should
print all optional attributes. While the module resolution is of limited
use, it would be handy to print all optional attributes, if present rather
than having to run javap.
It’s okay to follow up as a separate JBS issue if we want to do that.
test/jdk/modules/incubator/ImageModules.java
@modules jdk.jlink jdk.jartool are missing. I have fixed it.
Mandy
Re: RFR 8171138: Remove FileCopierPlugin
> On Dec 14, 2016, at 8:35 PM, Sundararajan Athijegannathan
> wrote:
>
> Hi Mandy,
>
> Thanks for your review. I updated to handle TOP entries in accept callback
> uniformly: http://cr.openjdk.java.net/~sundar/8171138/webrev.01/index.html
395 // Copy TOP files of the "java.base" module (only)
396 if ("java.base".equals(file.moduleName())) {
It may be better to throw an InternalError for any entry other than
“java.base”.
Otherwise looks good. You can fix this before you push. No need for a new
webrev.
Mandy
Re: RFR 8171316: Add IMPLEMENTOR property to the release file
> On Dec 15, 2016, at 7:32 AM, Sundararajan Athijegannathan > wrote: > > Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8171316 > > top level webrev: http://cr.openjdk.java.net/~sundar/8171316/top/webrev.00/ > jdk webrev: http://cr.openjdk.java.net/~sundar/8171316/jdk/webrev.00/ Looks okay. Mandy
Re: 8170987: Module system implementation refresh (12/2016)
> On Dec 15, 2016, at 10:13 AM, Alan Bateman wrote: > > On 15/12/2016 14:13, Claes Redestad wrote: > >> >> The context here, I assume, is the increased startup cost to initialize >> java.util.regex in 9 (and a few regression fixes related to this that >> I've done in the area which may have involved avoiding adding a >> regex-free fast path for trivial but common cases): > Yes although it's not an issue at this time.. Looking at it again then we > should be able to decompose it at link time and generate code that > reconstitutes it from its parts. That would avoid needing to reparse at > startup. That’s one possibility that we could consider in the future. Mandy
Review Request JDK-8171323: generate dot file for java.se and java.se.ee with only API dependences
This updates the GenGraphs build tool to generate a dot file for `java.se` and `java.se.ee` modules including API dependences only that can be used to display Java SE module graph. http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171323/webrev.00/ Mandy
hg: jigsaw/jake/jdk: SystemModuleFinder.Hashes class not needed - review feedback from Claes
Changeset: 9db6639ded4c Author:mchung Date: 2016-12-15 13:05 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/9db6639ded4c SystemModuleFinder.Hashes class not needed - review feedback from Claes ! src/java.base/share/classes/jdk/internal/module/SystemModuleFinder.java
Re: Review Request JDK-8171323: generate dot file for java.se and java.se.ee with only API dependences
> On Dec 15, 2016, at 5:12 PM, Paul Sandoz wrote:
>
>
>> On 15 Dec 2016, at 10:53, Mandy Chung wrote:
>>
>> This updates the GenGraphs build tool to generate a dot file for
>> `java.se` and `java.se.ee` modules including API dependences only
>> that can be used to display Java SE module graph.
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171323/webrev.00/
>>
>
> Looks ok.
>
> You can marginally improve genDotFile:
>
>
> List mds = cf.modules().stream()
> .map(ResolvedModule::reference)
> .map(ModuleReference::descriptor)
> .collect(toList());
>
> …
>
> printGraph(out, name, gengraph(cf),
> mds.stream().collect(toMap(ModuleDescriptor::name, Function.identity()));
>
> …
>
> printGraph(out, name, graph,
>mds.stream()
>.filter(md -> !md.name().startsWith("jdk.”) &&
> graph.nodes().contains(md.name()))
>.collect(toMap(ModuleDescriptor::name, Function.identity()))
Sure I can clean that up. Thanks.
Mandy
Re: Review Request JDK-8171323: generate dot file for java.se and java.se.ee with only API dependences
A small fix to the GenGraphs tool:
diff --git a/make/src/classes/build/tools/jigsaw/GenGraphs.java
b/make/src/classes/build/tools/jigsaw/GenGraphs.java
--- a/make/src/classes/build/tools/jigsaw/GenGraphs.java
+++ b/make/src/classes/build/tools/jigsaw/GenGraphs.java
@@ -214,13 +214,13 @@
// same ranks
ranks.stream()
-.forEach(group -> out.format("{rank=same %s}%n",
-descriptors.stream()
+.map(group -> descriptors.stream()
.map(ModuleDescriptor::name)
.filter(group::contains)
.map(mn -> "\"" + mn + "\"")
-.collect(joining(","))
-));
+ .collect(joining(",")))
+.filter(group -> group.length() > 0)
+.forEach(group -> out.format("{rank=same %s}%n", group));
descriptors.stream()
.filter(jdkGroup::contains)
Mandy
> On Dec 15, 2016, at 10:53 AM, Mandy Chung wrote:
>
> This updates the GenGraphs build tool to generate a dot file for
> `java.se` and `java.se.ee` modules including API dependences only
> that can be used to display Java SE module graph.
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171323/webrev.00/
>
> Mandy
Re: RFR: 8171373: Reduce copying during initialization of ModuleHashes
> On Dec 16, 2016, at 8:00 AM, Claes Redestad wrote: > > Hi, > > recent changes to split out ModuleHashes from ModuleDescriptor caused a > small/tiny increase in HashMap creation, resize and copying: > > Webrev: http://cr.openjdk.java.net/~redestad/8171373/webrev.01/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8171373 I like the optimization for one single module (i.e. java.base) containing the module hashes which is the common case. Can you add a comment in SystemModuleFinder about the common case and avoid creating a new HashMap unless there is more than one module containing ModuleHashes. Builder::nameToHash can now be final. Mandy
hg: jigsaw/jake/jdk: 8171374: GenGraphs should filter the rank grouping if the group is empty
Changeset: 4c3ba34b0749 Author:mchung Date: 2016-12-16 09:42 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/4c3ba34b0749 8171374: GenGraphs should filter the rank grouping if the group is empty Reviewed-by: alanb, psandoz ! make/src/classes/build/tools/jigsaw/GenGraphs.java
Re: RFR 8170289: Re-examine entry point support in jlink
> On Dec 16, 2016, at 8:36 AM, Sundararajan Athijegannathan
> wrote:
>
> Hi,
>
> Please review http://cr.openjdk.java.net/~sundar/8170289/webrev.01/ for
> https://bugs.openjdk.java.net/browse/JDK-8170289
273 Optional mainClass =
ModuleDescriptor.read(stream).mainClass();
274 if (mainClass.isPresent()) {
275 mainClassName = mainClass.get();
276 }
This should set mainClassName only if the main class is not specified
in the -—launcher option. One may want to create launchers for
multiple entry points.
I think it should validate if the main class is present in the image.
If not found, it should output an error.
Something to be considered in a future release - the existing implementation
creates the launcher scripts as a special case in DefaultImageBuilder.
It seems cleaner to keep DefaultImageBuilder just for the image creation,
i.e. simply write out entries of the ResourcePool to the image.
The launchers could be added to the ResourcePool entries to the
corresponding module by one builtin plugin implementation.
For this issue, keeping the change to minimal is good.
Mandy
Re: RFR: 8171373: Reduce copying during initialization of ModuleHashes
+1 Mandy > On Dec 16, 2016, at 4:45 PM, Claes Redestad wrote: > > Chris, Mandy, > > thanks for reviewing! > > I've made nameToHash final and added a comment, updated in place. > > I'll push this tomorrow unless I hear any objections. > > /Claes > > On 2016-12-16 18:52, Mandy Chung wrote: >> >>> On Dec 16, 2016, at 8:00 AM, Claes Redestad >>> wrote: >>> >>> Hi, >>> >>> recent changes to split out ModuleHashes from ModuleDescriptor caused a >>> small/tiny increase in HashMap creation, resize and copying: >>> >>> Webrev: http://cr.openjdk.java.net/~redestad/8171373/webrev.01/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171373 >> >> I like the optimization for one single module (i.e. java.base) containing >> the module hashes which is the common case. >> >> Can you add a comment in SystemModuleFinder about the common case and avoid >> creating a new HashMap unless there is more than one module containing >> ModuleHashes. >> >> Builder::nameToHash can now be final. >> >> Mandy >>
Re: RFR 8170289: Re-examine entry point support in jlink
282 throw new RuntimeException(module + " does not have
main class: " + mainClassName);
- Throwing IllegalArgumentException would probably be better.
101 err.launcher.value.format:launcher value should be of form
=: {0}
- should this message include optional main class; something like
=[/]
This validation is done after the image is created. Can you file a JBS issue
to separate the launcher file creation and the validation from image creation?
That can be improved in the future.
Otherwise, looks okay. No need to send a new webrev.
Mandy
P.S. There is some issue to access cr.openjdk.java.net. I got a copy of
webrev.02 from Sundar to review.
> On Dec 18, 2016, at 7:29 AM, Sundararajan Athijegannathan
> wrote:
>
> Updated it: http://cr.openjdk.java.net/~sundar/8170289/webrev.02
>
> Thanks,
> -Sundar
>
> On 17/12/16, 12:33 AM, Mandy Chung wrote:
>>> On Dec 16, 2016, at 8:36 AM, Sundararajan
>>> Athijegannathan wrote:
>>>
>>> Hi,
>>>
>>> Please review http://cr.openjdk.java.net/~sundar/8170289/webrev.01/ for
>>> https://bugs.openjdk.java.net/browse/JDK-8170289
>>
>> 273 Optional mainClass =
>> ModuleDescriptor.read(stream).mainClass();
>> 274 if (mainClass.isPresent()) {
>> 275 mainClassName = mainClass.get();
>> 276 }
>>
>> This should set mainClassName only if the main class is not specified
>> in the -—launcher option. One may want to create launchers for
>> multiple entry points.
>>
>> I think it should validate if the main class is present in the image.
>> If not found, it should output an error.
>>
>> Something to be considered in a future release - the existing implementation
>> creates the launcher scripts as a special case in DefaultImageBuilder.
>> It seems cleaner to keep DefaultImageBuilder just for the image creation,
>> i.e. simply write out entries of the ResourcePool to the image.
>> The launchers could be added to the ResourcePool entries to the
>> corresponding module by one builtin plugin implementation.
>>
>> For this issue, keeping the change to minimal is good.
>>
>> Mandy
>>
>>
Review Request: JDK-8168836 Minor clean up on warning/error messages on --add-exports and --add-reads
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168836/webrev.00/ This patch improves the warning/error message to include the option name, emit a warning if unknown module is specified with —-patch-module be consistent with the options. Mandy
Re: Review Request: JDK-8168836 Minor clean up on warning/error messages on --add-exports and --add-reads
> On Dec 19, 2016, at 9:45 AM, Alan Bateman wrote: > > > > On 19/12/2016 01:44, Mandy Chung wrote: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168836/webrev.00/ >> >> This patch improves the warning/error message to include the option name, >> emit a warning if unknown module is specified with —-patch-module be >> consistent with the options. >> > The implementation update looks good. As --patch-modules is a slow path then > you could use cf.findModule(mn).ifPresent(…) Fixed. > ExplodedModuleBuilder is useful infrastructure but I'm not sure that > test/tools/lib is the right place as that location seems to be for jimage > infrastructure (I've often thought this should move). Maybe > test/lib/testlibrary would be better because that there is where ModuleUtils > and the other test infrastructure used by these tests is. Also just wondering > if there is a better name for this class as it supports both creation and > compilation, maybe it should be in ModuleUtils as Builder or ModuleMaker (you > might have tried a few names already). Once this is in then we might look at > changes some of the existing tests to use it. What about renaming it to test/lib/testlibrary/ModuleSourceBuilder.java? http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168836/webrev.01 Mandy
Re: Review Request: JDK-8168836 Minor clean up on warning/error messages on --add-exports and --add-reads
> On Dec 19, 2016, at 11:39 AM, Mandy Chung wrote: > > >> On Dec 19, 2016, at 9:45 AM, Alan Bateman wrote: >> >> >> >> On 19/12/2016 01:44, Mandy Chung wrote: >>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168836/webrev.00/ >>> >>> This patch improves the warning/error message to include the option name, >>> emit a warning if unknown module is specified with —-patch-module be >>> consistent with the options. >>> >> The implementation update looks good. As --patch-modules is a slow path >> then you could use cf.findModule(mn).ifPresent(…) > > Fixed. Having a second thought, I prefer to keep the if statement testing isPresent(). Or I could do something like cf.findModule(mn).orElse(warnUnknownModule(PATCH_MODULE, mn)) but warnUnknownModule method would need to return ResolvedModule. Mandy
Re: RFR: 8171400: Move checking of duplicate packages in the boot layer to link time
tools/launcher/modules/patch/systemmodules/PatchSystemModules.java needs to be
updated since ModuleBootstrap now depends on this new method:
diff --git
a/test/tools/launcher/modules/patch/systemmodules/src1/java.base/jdk/internal/modules/SystemModules.java
b/test/tools/launcher/modules/patch/systemmodules/src1/java.base/jdk/internal/modules/SystemModules.java
---
a/test/tools/launcher/modules/patch/systemmodules/src1/java.base/jdk/internal/modules/SystemModules.java
+++
b/test/tools/launcher/modules/patch/systemmodules/src1/java.base/jdk/internal/modules/SystemModules.java
@@ -29,4 +29,8 @@
*/
public final class SystemModules {
public static final String[] MODULE_NAMES = new String[0];
+
+public static boolean hasSplitPackages() {
+return true;
}
+}
Since this fix has been pushed, I will fix this with a separate issue.
Mandy
> On Dec 19, 2016, at 4:30 AM, Claes Redestad wrote:
>
> Hi,
>
> this patch adds a check to see if there are any split packages in the system
> modules at link time, and uses this information to enable us to safely skip
> a runtime check during bootstrap for the common case that there are none
> of the sort.
>
> Webrev[1]: http://cr.openjdk.java.net/~redestad/8171400/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8171400
>
> This removes a chunk of the module system bootstrap overhead, and also
> amends a small issue where PACKAGES_IN_BOOT_LAYER would be wrong in the
> presence of split packages.
>
> Thanks!
>
> /Claes
>
> [1] Since cr.openjdk.java.net is down I've also attached the raw patch.
>
Re: RFR 8170618: jmod should validate if any exported or open package is missing
> On Dec 21, 2016, at 4:08 AM, Sundararajan Athijegannathan > wrote: > > Please review http://cr.openjdk.java.net/~sundar/8170618/webrev.00/ for > https://bugs.openjdk.java.net/browse/JDK-8170618 thanks for fixing this Sundar. Minor comment on the test: the bug id's already listed in @bug. You can take out the bug id from line 117. line 132 should be broken into multiple lines. Otherwise, looks good. Mandy
Re: Invoking default methods from a Proxy's InvocationHandler in JDK9
> On Jan 2, 2017, at 11:20 PM, Peter Levart wrote: > > Hi Matthew, > > On 01/03/2017 04:28 AM, Matthew Hall wrote: >> I'm a member of the JDBI [1] project, an open source SQL access library >> atop JDBC. >> >> A major part of our API provides implementations of declarative interfaces >> defined by users (similar to MyBatis). Interface methods may be default (in >> which case the default method implementation is used) or abstract (in which >> case JDBI provides the implementation). >> >> We're using JDK 8, and we use java.lang.reflect.Proxy and InvocationHandler >> to provide declarative interface implementations for our users. >> >> Prior to release of JDK 8, it appears that the subject of enhancing Proxies >> for default methods was discussed [2], but ultimately did not make it into >> the release. >> >> The root of the problem is that the generated Proxy class overrides all >> methods in the proxy interfaces. This means that the interface default >> methods are now superclass methods to the proxy class, which makes them >> un-invokable from outside code--including the InvocationHandler. >> >> As far as we can tell, the _only_ way to invoke a default method--on a >> proxy, from an InvocationHandler--is to resort to some horrible >> setAccessible shenanigans [3]. >> >> Specifically, we have to: >> 1. Call Constructor.setAccessible(true) to make the private constructor >> MethodHandles.Lookup(Class lookupClass, int allowedModes) accessible, >> 2. Invoke that private constructor to instantiate a MethodHandles.Lookup >> with private (!) access to all the things, >> 3. Call Lookup.unreflectSpecial to get the MethodHandle for the default >> method, and >> 4. Invoke the method through the MethodHandle. >> >> This is ugly, but it works--for now. If JDK 9 takes away access to >> setAccessible, it may cease to work. >> >> I found some discussion about this last summer [4], but it's hard to tell >> if anything came out of the discussion. >> >> Is there anything on the roadmap for JDK9 to allow a proxy's >> InvocationHandler to invoke default methods on proxies? Are there any >> existing proposals I should be aware of? > > I have created a prototype last year: > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html > > But I think the JDK 9 train has already left the station. So perhaps in JDK > 10... https://bugs.openjdk.java.net/browse/JDK-8159746 is the JBS issue for this. One other possibility is to fix proxy generated class not to override default methods but there would require more investigation to tease out before we can be certain if this can make JDK 9. Mandy
Re: Review Request: JDK-8168836 Minor clean up on warning/error messages on --add-exports and --add-reads
> On Jan 3, 2017, at 1:04 PM, David Holmes wrote: > > Hi Mandy, > > >> >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168836/webrev.00/ >> >> This patch improves the warning/error message to include the option name, >> emit a warning if unknown module is specified with —-patch-module be >> consistent with the options. >> >> Mandy > > As a result of this change we now reject an empty classpath with an error: > > /java/re/jdk/9/promoted/all/150/binaries/linux-x64/bin/java -cp "" Hello > Error: -cp requires class path specification > > was that intentional? We've accepted an empty -cp value for a very long time. No this is not intentional. This line seems to be the cause of the regression: 1250 jboolean has_arg = value != NULL && JLI_StrLen(value) > 0; I’ll fix it. I had a test checking on empty classpath that passed. I’ll look into it how this test case is missed. http://hg.openjdk.java.net/jdk9/dev/jdk/file/d27bab22ff62/test/tools/launcher/modules/classpath/JavaClassPathTest.java Thanks for finding it. Mandy
Re: RFR [9] 8168149: Examine the behavior of jmod command-line options - repeating vs last one wins
> On Dec 22, 2016, at 9:11 AM, Chris Hegarty wrote: > > Most options for the jmod tool should be last one wins, to be consistent > with the JDK tool convention, 8168149 [1]. Excludes is the only > repeatable option. > > Given the existing usage of JOpt Simple, the most straight forward way > to achieve the last-one-wins behaviour is to drop the > withValuesSeparatedBy() from the OptionSpec have have the ValueConverters > themselves do the separation, if any. That way all options can be made > repeatable and the last element of the list of the option’s values will > be the final one on the command line. > > http://cr.openjdk.java.net/~chegar/8168149.00/ This looks okay. I wonder if it would be more helpful if it fails when a non-repeating option is specified more than once for a packaging tool. Otherwise, the only way to find out if the command-line is correct is to list the content after the JMOD file is created. OptionSpec::value throws an exception if the option is specified more than once. Mandy
Re: RFR [9] 8168149: Examine the behavior of jmod command-line options - repeating vs last one wins
> On Jan 5, 2017, at 5:14 AM, Chris Hegarty wrote: > > >> I wonder if it would be more helpful if it fails when a non-repeating option >> is specified more than once for a packaging tool. Otherwise, the only way >> to find out if the command-line is correct is to list the content after the >> JMOD file is created. OptionSpec::value throws an exception if the option >> is specified more than once. > > Right. I was following the existing longstanding precedent of > last-one-wins for JDK tool options. If I read your comment > correctly you are suggesting that packaging tools do not > follow this, correct? This will need further discussion, and > maybe a clarification in JEP 293 "Guidelines for JDK > Command-Line Tool Options”. jmod is a new JDK 9 tool that we should consider what’s the right thing to do with the new tool-specific options that specify the location of the content to be packaged such as —-cmds, —-libs, etc. The discussion would be around `—-module-path` whether JDK tools accepting these options is required to follow the last-one-wins convention (which I have no objection to make them last-one-wins for consistency while I’m not too concern for it to be different for jmod tool). I agree that JEP 293 should cover this guideline. Mandy
Re: RFR: 8171855: Move package name transformations during module bootstrap into native code
> On Jan 5, 2017, at 8:47 AM, Claes Redestad wrote:
>
> Hi,
>
> after a round of review comments I've now reworked this to do the
> transformations in the JNI layer rather than inside the VM, with
> similar - if not better - results.
>
> Webrevs:
> http://cr.openjdk.java.net/~redestad/8171855/hotspot.03/
> http://cr.openjdk.java.net/~redestad/8171855/jdk.03/
>
Happy to know the performance gain is comparable when pushing down the
conversion to internal form in native instead of doing it in the VM. This is
good work.
src/java.base/share/native/libjava/Module.c
This can be refactored e.g. adding a new function GetInternalPackageName that
takes a jstring and returns const char*.
GetStringUTFChars will return a copy of the utf-8 chars. That is an
alternative to malloc, GetStringUTFLength, GetStringLength, GetStringUTFRegion
calls. Use ReleaseStringUTFChars to free the copy after use.
Nit: it may be clearer to rename pkgs_len to num_pkgs
src/share/vm/classfile/modules.cpp
49 static bool verify_module_name(const char *module_name) {
To be consistent with the convention in this file: const char*
Otherwise, looks fine.
Mandy
Re: RFR: 8171855: Move package name transformations during module bootstrap into native code
> On Jan 6, 2017, at 5:18 AM, Claes Redestad wrote: > > Updated jdk webrev in-place: > > http://cr.openjdk.java.net/~redestad/8171855/jdk.04/ Looks good to me. I assume you are adding a sanity test on the package name > 128 chars in this webrev? Mandy
Re: Does jlink work on exploded build?
> On Jan 10, 2017, at 6:29 AM, Wang Weijun wrote: > > $ jlink --module-path ../build/windows-x64-debug/jdk/modules/ --add-modules > jdk.jartool --output /tmp/jartool > Error: java.lang.RuntimeException: Module jdk.jartool's descriptor returns > inconsistent package set jlink in the exploded build works if you specify the proper module path. The module path should be `images/jmods` where JMOD files are found because java.base and jdk.jartools contains more than classes such as executables, shared libraries, etc. Mandy
Re: Does jlink work on exploded build?
Correct, in your case, it won’t work. Mandy > On Jan 10, 2017, at 4:09 PM, Wang Weijun wrote: > > Ah, when I said exploded build, I meant jdk/modules/java/base/.../*.classes. > There is no native files there so I guess not supported. > > --Max > >> On Jan 11, 2560 BE, at 1:12 AM, Mandy Chung wrote: >> >> >>> On Jan 10, 2017, at 6:29 AM, Wang Weijun wrote: >>> >>> $ jlink --module-path ../build/windows-x64-debug/jdk/modules/ --add-modules >>> jdk.jartool --output /tmp/jartool >>> Error: java.lang.RuntimeException: Module jdk.jartool's descriptor returns >>> inconsistent package set >> >> jlink in the exploded build works if you specify the proper module path. The >> module path should be `images/jmods` where JMOD files are found because >> java.base and jdk.jartools contains more than classes such as executables, >> shared libraries, etc. >> >> Mandy >
Re: RFR [9] 8168149: Examine the behavior of jmod command-line options - repeating vs last one wins
> On Jan 5, 2017, at 12:08 PM, Mandy Chung wrote: > > >> On Jan 5, 2017, at 5:14 AM, Chris Hegarty wrote: >> >> >>> I wonder if it would be more helpful if it fails when a non-repeating >>> option is specified more than once for a packaging tool. Otherwise, the >>> only way to find out if the command-line is correct is to list the content >>> after the JMOD file is created. OptionSpec::value throws an exception if >>> the option is specified more than once. >> >> Right. I was following the existing longstanding precedent of >> last-one-wins for JDK tool options. If I read your comment >> correctly you are suggesting that packaging tools do not >> follow this, correct? This will need further discussion, and >> maybe a clarification in JEP 293 "Guidelines for JDK >> Command-Line Tool Options”. > > jmod is a new JDK 9 tool that we should consider what’s the right thing to do > with the new tool-specific options that specify the location of the content > to be packaged such as —-cmds, —-libs, etc. > > The discussion would be around `—-module-path` whether JDK tools accepting > these options is required to follow the last-one-wins convention (which I > have no objection to make them last-one-wins for consistency while I’m not > too concern for it to be different for jmod tool). I agree that JEP 293 > should cover this guideline. I checked jlink, jmod and the new options in jdeps and jar tools. Most of them are last one wins. With an offline discussion, we agree that we are getting late in the release cycle and best to keep this policy. And possibly revisit this in a future release. I’m okay with this patch to go. Thanks Mandy
Review Request JDK-8160286: jmod hash is creating unlinkable modules.
Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/ jmod and jar -—hash-modules option to specify a pattern of modules to be hashed in the module M being created. It records the modules that depend on M directly and indirectly. jmod hash command records the hashes in modules that have already been packaged. It will identify the base module(s) with no going edges and record the hashes in that module. This patch fixes jmod hash command how the base modules are identified. For example, $ jmod hash -—dry-run —-module-path jmods -—hash-modules "jdk.jdeps|jdk.compiler|java.compiler” java.compiler should be the only module recording the hashes of jdk.compiler and jdk.jdeps. It first builds a graph of the modules matching the pattern specified in the --hash-module option. It traverses the graph in topological order, for each module, it is a base module if it’s a matching module and not hashed and then record the matching modules that directly and indirectly depend on it. This also cleans up the duplicated code and shared by jar and jmod tools now. I found that jar -—module-path is a repeating option and this patch fixes it to be the last-one-wins, consistent with the runtime option, and other tools. Mandy
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
> On Jan 11, 2017, at 5:47 PM, Paul Sandoz wrote:
>
> JmodTask
> —
>
> 839
> 840 hashesBuilder.computeHashes(modules).entrySet().forEach(e -> {
>
> You can use the Map.forEach method accepting BiFunction of key and value.
>
Good suggestion. Fixed.
>
> Make static?
>
It could but the Hasher class needs to be made static too and pass in other
instance variables from JmodTask. There are other inner classes in JmodTask
that accesses JmodTask instance variables. I prefer to leave this for a future
clean up to make the implementation consistent, if this is okay with you?
Mandy
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
> On Jan 12, 2017, at 7:08 AM, Alan Bateman wrote: > > On 11/01/2017 23:47, Mandy Chung wrote: > >> Webrev: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/ >> >> jmod and jar -—hash-modules option to specify a pattern of modules >> to be hashed in the module M being created. It records the modules >> that depend on M directly and indirectly. >> > This looks quite good. At some point then we'll need to move the tool > support out of jdk.internal.module but is something for another day. > > For ModuleHashesBuilder then it might be useful to put a comment on the > constructors as it's not immediately obvious why both are needed. Also I > wonder if we should use a term other than "base" for the modules that don't > have references to other modules in the sub-graph (they are sort of leaf > modules in the sub-graph). A typo at L96 "in topological orders" -> "order”. I’ll take a pass and update the comments. I can see “base” can be confusing. > One of the changes in this patch is that the `jar` tool will locate JMOD > files on the module path. I assume this is to provide flexibility to those > creating a modular JAR that want to tie it to a specific JDK build. I guess > it's okay but I suspect will not be widely used. Right and another case is packaged module with a native library (security providers). It will not be widey used. Mandy
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
> On Jan 12, 2017, at 5:58 AM, Chris Hegarty wrote: > > >> On 11 Jan 2017, at 23:47, Mandy Chung wrote: >> >> Webrev: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/ > > Looks fine. It’s good to share as much of this code as possible between > these packaging tools. > > I didn’t look too hard at Graph and TopoSorter, these look similar to > what is in other areas, e.g. jdeps, > Yes it is from jdeps. > In the jar option parsing did you mean to construct ModulePath with true? > This will support jmod files. Yes that will allow a modular JAR to tie with a specific JDK build or packaged modules with native library. Mandy
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.01/ I did a further clean up and ModuleHashesBuilder now has one constructor. Also updated the comment to avoid using the term “base”. Due to the refactoring, I moved the location of the tmp jmod file to be in the tmp directory rather than the same directory of the jmod file; otherwise, it would confuse the ModuleFinder with the tmp file. I added a few more test cases that remind me that a packaged user module can’t tie with a specific JDK build since a module requiring a JDK module that would be a leaf node of the subgraph rather than that module. The `jar` tool locating JMOD files on the module path allows to tie a modular JAR with a module requiring it but packaged as JMOD file (e.g. with native library). Mandy > On Jan 12, 2017, at 7:52 AM, Mandy Chung wrote: > > >> On Jan 12, 2017, at 7:08 AM, Alan Bateman wrote: >> >> On 11/01/2017 23:47, Mandy Chung wrote: >> >>> Webrev: >>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/ >>> >>> jmod and jar -—hash-modules option to specify a pattern of modules >>> to be hashed in the module M being created. It records the modules >>> that depend on M directly and indirectly. >>> >> This looks quite good. At some point then we'll need to move the tool >> support out of jdk.internal.module but is something for another day. >> >> For ModuleHashesBuilder then it might be useful to put a comment on the >> constructors as it's not immediately obvious why both are needed. Also I >> wonder if we should use a term other than "base" for the modules that don't >> have references to other modules in the sub-graph (they are sort of leaf >> modules in the sub-graph). A typo at L96 "in topological orders" -> "order”. > > I’ll take a pass and update the comments. I can see “base” can be confusing. > >> One of the changes in this patch is that the `jar` tool will locate JMOD >> files on the module path. I assume this is to provide flexibility to those >> creating a modular JAR that want to tie it to a specific JDK build. I guess >> it's okay but I suspect will not be widely used. > > Right and another case is packaged module with a native library (security > providers). It will not be widey used. > > Mandy
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
Not really need to be. I can take it out. Mandy > On Jan 13, 2017, at 1:43 AM, Andrej Golovnin > wrote: > > Hi Mandy, > > src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java > > 806 final Optional moduleName; // a specific module > to record hashes > > I thought that using Optional in a class field is considered a misuse > of the API. The field hashesBuilder can also be null. But you don't > wrap it into an Optional. So why it is needed to wrap moduleName into > Optional? > > Best regards, > Andrej Golovnin > > On Fri, Jan 13, 2017 at 6:13 AM, Mandy Chung wrote: >> Updated webrev: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.01/ >> >> I did a further clean up and ModuleHashesBuilder now has one constructor. >> Also updated the comment to avoid using the term “base”. Due to the >> refactoring, I moved the location of the tmp jmod file to be in the >> tmp directory rather than the same directory of the jmod file; >> otherwise, it would confuse the ModuleFinder with the tmp file. >> >> I added a few more test cases that remind me that a packaged user >> module can’t tie with a specific JDK build since a module requiring >> a JDK module that would be a leaf node of the subgraph rather than >> that module. The `jar` tool locating JMOD files on the module path >> allows to tie a modular JAR with a module requiring it but packaged >> as JMOD file (e.g. with native library). >> >> Mandy >> >>> On Jan 12, 2017, at 7:52 AM, Mandy Chung wrote: >>> >>> >>>> On Jan 12, 2017, at 7:08 AM, Alan Bateman wrote: >>>> >>>> On 11/01/2017 23:47, Mandy Chung wrote: >>>> >>>>> Webrev: >>>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/ >>>>> >>>>> jmod and jar -—hash-modules option to specify a pattern of modules >>>>> to be hashed in the module M being created. It records the modules >>>>> that depend on M directly and indirectly. >>>>> >>>> This looks quite good. At some point then we'll need to move the tool >>>> support out of jdk.internal.module but is something for another day. >>>> >>>> For ModuleHashesBuilder then it might be useful to put a comment on the >>>> constructors as it's not immediately obvious why both are needed. Also I >>>> wonder if we should use a term other than "base" for the modules that >>>> don't have references to other modules in the sub-graph (they are sort of >>>> leaf modules in the sub-graph). A typo at L96 "in topological orders" -> >>>> "order”. >>> >>> I’ll take a pass and update the comments. I can see “base” can be >>> confusing. >>> >>>> One of the changes in this patch is that the `jar` tool will locate JMOD >>>> files on the module path. I assume this is to provide flexibility to those >>>> creating a modular JAR that want to tie it to a specific JDK build. I >>>> guess it's okay but I suspect will not be widely used. >>> >>> Right and another case is packaged module with a native library (security >>> providers). It will not be widey used. >>> >>> Mandy >>
Re: Review Request JDK-8160286: jmod hash is creating unlinkable modules.
> On Jan 13, 2017, at 6:09 AM, Alan Bateman wrote: > > On 13/01/2017 05:13, Mandy Chung wrote: > >> Updated webrev: >>http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.01/ >> >> I did a further clean up and ModuleHashesBuilder now has one constructor. >> Also updated the comment to avoid using the term “base”. Due to the >> refactoring, I moved the location of the tmp jmod file to be in the >> tmp directory rather than the same directory of the jmod file; >> otherwise, it would confuse the ModuleFinder with the tmp file. >> > The updated patches look good and ModuleHashesBuilder is much simpler with > one constructor. > > For computeHashes then it says "... in a subgraph and that has .." which > might need a few tweaks to make more readable. > What about this: * The key for each entry in the returned map is a module M that has * no outgoing edges to any of the candidate modules to be hashed * i.e. M is a leaf node in a connected subgraph containing M and * other candidate modules from the module graph filtering * the outgoing edges from M to non-candidate modules. Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.02/ > A typo in JmodTask.Hasher L829, should be "the". Also I assume the field > Hasher.moduleName should be a String as Andrej noted. Fixed. Mandy
Re: RFR 8171380: Remove exports from jdk.jlink
> On Jan 16, 2017, at 7:46 AM, Chris Hegarty wrote: > > The Jlink Plugin API is not going to be supported in 9, so > should not be exported [1], for use by third parties. Besides > the removal of the export, most of the changes are adding > @modules tags to existing tests. > > http://cr.openjdk.java.net/~chegar/8171380.00/ > > : > > This change depends on fix in jtreg b05 [3], so bumps the > minimum required version. > langtools/test/TEST.ROOT should also be updated as there are tests in langtools/test also depending on the fix for jtreg b05. Otherwise looks good. Mandy
Re: Compact profile modules
java.compact$N modules were removed in jdk-9+150 [1][2]. We revisited the need for java.compact$N aggregator. Compact Profiles were useful in SE 8 but are legacy in 9, as intended from the outset. Highlighting them in the standard SE 9 module graph gives them unnecessary importance going forward. We want people who care about small systems to think about modules, not profiles. In addition, these aggregator modules are not a complete solution since the necessary service providers are still needed to be added manually for the Compact Profile builds. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8171201 [2] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010561.html > On Jan 16, 2017, at 1:35 PM, Sander Mak wrote: > > The build I'm currently using (jigsaw-b151) doesn't contain the java.compactN > modules anymore. Looks like this is a deliberate removal [1], although I > can't find any reason for it. Are they permanently gone? Why? They made for > such a nice example of implied readability in the JDK... They were also > useful in constructing small images with jlink. As I understood things at > JavaOne, jlink with the compact module definitions was supposed to be a > 'migration path' from Java SE 8 embedded. > > > Sander > > [1] > http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010603.html
Review request: JDK-8172870 test/tools/jmod/JmodTest.java fails on windows with AccessDeniedException
This test case attempts to verify that the temp file created by jmod is cleaned up and removed if jmod create command fails. It first cleans up any temp files matching the prefix and suffix created for this test case. The test fails because the fix for JDK-8160286 that creates the jmod temp file in the tmp directory and it was updated to use Files.walk that may fail to read the file attributes if the user doesn’t have access. The patch fixes the test to call Files.list and find only the writeable files matching the prefix and suffix. http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8172870/webrev.00/ Mandy
hg: jigsaw/jake/langtools: jdeps update per ModuleDescriptor/Builder cleanup
Changeset: 2eac819e2faa Author:mchung Date: 2017-01-17 14:42 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/2eac819e2faa jdeps update per ModuleDescriptor/Builder cleanup ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/Module.java ! src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ModuleInfoBuilder.java
Re: RFR 8168254: Detect duplicated resources in packaged modules
> On Jan 17, 2017, at 10:03 PM, Sundararajan Athijegannathan > wrote: > > Please review http://cr.openjdk.java.net/~sundar/8168254/webrev.00/ for > https://bugs.openjdk.java.net/browse/JDK-8168254 Looks good except the copyright end year. Mandy
Re: RFR [9] 8172982: tools/jlink/ResourceDuplicateCheckTest.java requires jdk.tools.jlink.plugin to be exported
+1 Mandy > On Jan 18, 2017, at 9:05 AM, Chris Hegarty wrote: > > Since the jlink plugin package is no longer exported this test should > add the appropriate @modules tag. > > diff -r 5b75946223fb test/tools/jlink/ResourceDuplicateCheckTest.java > --- a/test/tools/jlink/ResourceDuplicateCheckTest.javaWed Jan 18 > 13:56:50 2017 + > +++ b/test/tools/jlink/ResourceDuplicateCheckTest.javaWed Jan 18 > 17:05:11 2017 + > @@ -27,6 +27,7 @@ > * @summary Detect duplicated resources in packaged modules > * @modules jdk.jlink/jdk.tools.jlink.builder > * jdk.jlink/jdk.tools.jlink.internal > + * jdk.jlink/jdk.tools.jlink.plugin > * @run build ResourceDuplicateCheckTest > * @run main ResourceDuplicateCheckTest > */ > > -Chris.
Re: Review request: JDK-8172870 test/tools/jmod/JmodTest.java fails on windows with AccessDeniedException
> On Jan 18, 2017, at 1:25 AM, Chris Hegarty wrote: > > >> On 18 Jan 2017, at 07:38, Alan Bateman wrote: >> >> On 17/01/2017 21:23, Mandy Chung wrote: >> >>> This test case attempts to verify that the temp file created by jmod is >>> cleaned up and removed if jmod create command fails. It first cleans up >>> any temp files matching the prefix and suffix created for this test case. >>> The test fails because the fix for JDK-8160286 that creates the jmod temp >>> file in the tmp directory and it was updated to use Files.walk that may >>> fail to read the file attributes if the user doesn’t have access. >>> >>> The patch fixes the test to call Files.list and find only the writeable >>> files matching the prefix and suffix. >>> >>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8172870/webrev.00/ > > When this test was added originally jmod was creating the tmp file in > the current working directory. > > To avoid interference you could run with test with java.io.tmpdir set > to the current working directory, “testng/othervm -Djava.io.tmpdir=. …" Good suggestion. I updated JmodTest.java on top of the patch: * @run testng/othervm -Djava.io.tmpdir=. JmodTest Mandy
hg: jigsaw/jake/jdk: Minor update on Proxy spec to clarify open packages
Changeset: c1569ce11b78 Author:mchung Date: 2017-01-18 13:20 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/c1569ce11b78 Minor update on Proxy spec to clarify open packages ! src/java.base/share/classes/java/lang/reflect/Proxy.java
hg: jigsaw/jake/jdk: 2 new changesets
Changeset: 25d0881b2c0c Author:mchung Date: 2017-01-19 10:10 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/25d0881b2c0c Fix typo ! src/java.base/share/classes/java/lang/reflect/Proxy.java Changeset: e5e09b0e750b Author:mchung Date: 2017-01-19 10:12 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/e5e09b0e750b StackWalker::getCallerClass throws IllegalCallerException consistent with other @CS methods ! src/java.base/share/classes/java/lang/StackStreamFactory.java ! src/java.base/share/classes/java/lang/StackWalker.java ! test/java/lang/StackWalker/CallerFromMain.java
Re: RFR 8172527: Rename jdk.crypto.token to jdk.crypto.cryptoki
> On Jan 19, 2017, at 11:39 AM, Anthony Scarpino > wrote: > > Hi, > > I need a review to rename the jdk.crypto.token to jdk.crypto.cryptoki. This > is to change what 8171202 had done to the original jdk.crypto.pkcs11 module. > For those not familiar with discussions elsewhere, the term "token" is > confusing and unclear as it can mean many things cryptographically. The use > of cryptoki is more accurate to the original 'pkcs11' name and still > conforming to the new naming standard. > > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-root/ > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-jdk/ The webrev shows deleted/new files for src/jdk.crypto.token source files rather than rename. Did you use hg rename? Changes in other files looks good. Mandy
Re: RFR 8172659: PluginException("TargetPlatform attribute is missing ...") - should be ModuleTarget
+1 Mandy > On Jan 20, 2017, at 1:06 AM, Sundararajan Athijegannathan > wrote: > > Please review http://cr.openjdk.java.net/~sundar/8172659/webrev.00/ for > https://bugs.openjdk.java.net/browse/JDK-8172659 > > Thanks, > -Sundar
Review Request: JDK-8173096 jmod files are not world-readable
Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html A recent change in jmod tool [1] creates the JMOD file with no group and other readable permission as that’s the default permission when creating a temporary file with Files::createTempFile. This fixes the permission issue by creating the JMOD file in a temporary directory. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8173096
Re: Review Request: JDK-8173096 jmod files are not world-readable
> On Jan 20, 2017, at 12:37 PM, Peter Levart wrote: > > I guess the JmodTask is creating a temp file because it wants the file to not > be observed by other processes while it is being written to, right? At the > end when the tempTarget is fully written, it is renamed to its intended name > with: > > Files.move(tempTarget, target); > > > ...but that operation will not be atomic since you didn't pass ATOMIC_MOVE to > the CopyOption... options. Even if you did, the atomic move will succeed only > when both tempTarget and target point to the same filesystem. Since you are > creating tempTarget in default temporary files location (/tmp on UNIXes) this > might not be the case (depending on where the real target is located). > > So why don't you simply append a .tmp suffix to the intended target file name > when opening the JmodOutputStream. This would ensure the file location will > be on the same filesystem as the final target name as it will be created in > the same directory. Module path scanning should skip files with .tmp > extension then. Module path only ignores files starts with “.” or is hidden. This says making the temp file starting with “.” is a possibility. $ java -p mods -m hi Error occurred during initialization of VM java.lang.module.ResolutionException: Unrecognized module: mods/abc.tmp Mandy
Re: Review Request: JDK-8173096 jmod files are not world-readable
> On Jan 20, 2017, at 1:22 PM, Alan Bateman wrote: > > Mandy - a while back then Erik changed the build so that JMOD files are > created in JMODS_TEMPDIR rather than JMODS_DIR. I think this was to reduce > interference with concurrent execution of the `jmod` tool. This might mean > you can avoid using /tmp. The issue is jmod —-hash-modules resolves a module graph from a given module path which is the containing directory of the JMOD file being created: $ jmod create -—module-path dir —-hash-modules “.*” dir/m.jmod It seems one solution is to have the temporary file name be a hidden file. Mandy
Re: RFR 8172527: Rename jdk.crypto.token to jdk.crypto.cryptoki
> On Jan 20, 2017, at 10:22 AM, Anthony Scarpino > wrote: > > Good catch.. that'll teach me for trusting the graphical tool to rename a > directory when I used 'Rename'. > > Also I found Brad's issue as it was a new changeset that just showed up in > that file. > > I updated the webrev, which looks better now: > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-jdk.01/ +1 Mandy
Re: Review Request: JDK-8173096 jmod files are not world-readable
Updated webrev to put the temporary file in the same containing directory of the target file: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01 I want to fix this regression in jdk-9+154. We can revisit the approach after this fix if necessary. Mandy > On Jan 20, 2017, at 8:55 AM, Mandy Chung wrote: > > Webrev: > http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html > > A recent change in jmod tool [1] creates the JMOD file with no group > and other readable permission as that’s the default permission when > creating a temporary file with Files::createTempFile. This fixes > the permission issue by creating the JMOD file in a temporary directory. > > Mandy > [1] https://bugs.openjdk.java.net/browse/JDK-8173096
hg: jigsaw/jake/jdk: Minor clean up on StackWalker::getCallerClass spec
Changeset: cbf1e85f7c11 Author:mchung Date: 2017-01-20 19:58 -0800 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/cbf1e85f7c11 Minor clean up on StackWalker::getCallerClass spec ! src/java.base/share/classes/java/lang/StackWalker.java
Re: Review Request: JDK-8173096 jmod files are not world-readable
> On Jan 21, 2017, at 5:08 AM, Alan Bateman wrote: > > On 21/01/2017 02:32, Mandy Chung wrote: > >> Updated webrev to put the temporary file in the same containing directory of >> the target file: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01 >> >> I want to fix this regression in jdk-9+154. We can revisit the approach >> after this fix if necessary. >> > This looks okay, I guess an alternative fix would be for CreateJmods.gmk to > set the permissions. I prefer jmod tool to create a file with the proper permission in this fix. Thanks > There are a couple of issues that have come up here and I agree that we > should re-visit once this regression is out of the way. Thanks Mandy
Re: Review Request: JDK-8173096 jmod files are not world-readable
> On Jan 22, 2017, at 3:17 AM, Peter Levart wrote: > > Hi Mandy, > > On 01/21/2017 03:32 AM, Mandy Chung wrote: >> Updated webrev to put the temporary file in the same containing directory of >> the target file: >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01 >> <http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01> > That's ok. Maybe also specify ATOMIC_MOVE to CopyOption ... options in > Files.move() ? Currently the move will be atomic (rename in UNIX terminology) > because the source and target are in the same directory, but if renaming > fails for some reason, it will try the fallback to copy(src, dst) + > remove(src). If you add ATOMIC_MOVE to options, failure to rename will result > in exception. I don't know which one do you prefer? I file https://bugs.openjdk.java.net/browse/JDK-8173280 <https://bugs.openjdk.java.net/browse/JDK-8173280> I prefer to see how jmod is used and any one runs into this issue that the atomic move and fallback to copy/remove may be needed. The build could avoid the interference by creating a JMOD of the same name in a different directory. Mandy
Re: jlink / legal folder and LICENSE
> On Jan 25, 2017, at 3:08 AM, Alan Bateman wrote: > > On 25/01/2017 10:53, fo...@univ-mlv.fr wrote: > >> : >> I mean legal/java.base/LICENSE in jdk9. > > The EA builds that Oracle publishes on jdk9.java.net have a LICENSE file that > links to the BCL on oracle.com. AFAIK, this link has not changed, it's not > really something that we have any control of here. Before jdk-9+136, the LICENSE file was in the top level JDK directory with the following content: Please refer to http://java.com/license It was a bug. The EA builds are under a different click-through license [1] at download time and so it was removed in jdk-9+136. > > If you are building jdk9/dev yourself then I assume you get the LICENSE file > that you looking for. > > When using jlink to create your own run-time images then the legal notices > come from the packaged modules. > For OpenJDK build, legal/java.base/LICENSE contains OpenJDK license: http://openjdk.java.net/legal/gplv2+ce.html Mandy [1]] https://jdk9.java.net/download/
Review Request: 8173381: osName/osArch/osVersion is missing in ModuleDescriptor created by SystemModules
The ModuleTarget attribute is added to java.base and other JDK modules when JMOD file is created but it gets dropped when the system modules plugin reconstitutes the ModuleDescriptor for fast loading. The fix is straight-forward to add osName/osArch/osVersion if present. Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173381/webrev.00/ thanks Mandy
Re: Review Request: 8173381: osName/osArch/osVersion is missing in ModuleDescriptor created by SystemModules
> On Jan 26, 2017, at 12:33 AM, Alan Bateman wrote: > > The set of modules has to include java.base so if it has all three properties > then there is no need to call the builder methods for the other modules. That > will save a few bytecodes at startup. It's not critical of course, just a > small saving. It’s a nice small optimization. I revised the patch to drop the ModuleTarget attribute, if java.base has all three properties. I added a plugin option to retain the ModuleTarget attribute, primarily for testing purpose. > > Otherwise I think the change looks okay. At some point then the ModuleTarget > class file attribute needs to be re-examined to see if the values it records > are the right set. So it's possible it will change again. If there are more > then I assume we can use one method to emit the instructions as they are > builder method name / value pairs. > Updated webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173381/webrev.01/ thanks Mandy
Re: Review Request: 8173381: osName/osArch/osVersion is missing in ModuleDescriptor created by SystemModules
Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173381/webrev.02
> On Jan 28, 2017, at 10:01 AM, Alan Bateman wrote:
>
> On 27/01/2017 06:21, Mandy Chung wrote:
>
>> :
>> Updated webrev:
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173381/webrev.01/
>>
> This mostly looks good.
>
> I think it would cleaner if the plugin used rewriter.targetPlatform("", "",
> "") rather than adding filtering to the extender.
>
I agree that would be cleaner and made the change. That’s what I started with
but that’d leave ModuleTarget attribute in the class file.
> For the secret option for testing then I assume it should be
> "retainModuleTarget" rather than "retainTargetPlatform". There are a couple
> of places that use the old name in method names and maybe we should rename
> those too. One other rename is the plugin has "PackagesAttribute" when it
> should be "ModulePackages" attribute.
I fixed up a few places in ModuleInfoExtender as well.
Mandy
Re: Review Request: 8173381: osName/osArch/osVersion is missing in ModuleDescriptor created by SystemModules
> On Jan 29, 2017, at 1:56 PM, Alan Bateman wrote: > > On 28/01/2017 20:50, Mandy Chung wrote: > >> : >> I agree that would be cleaner and made the change. That’s what I started >> with but that’d leave ModuleTarget attribute in the class file. >> >>> For the secret option for testing then I assume it should be >>> "retainModuleTarget" rather than "retainTargetPlatform". There are a couple >>> of places that use the old name in method names and maybe we should rename >>> those too. One other rename is the plugin has "PackagesAttribute" when it >>> should be "ModulePackages" attribute. >> I fixed up a few places in ModuleInfoExtender as well. >> > The updated webrev looks okay and I think okay to keep the ModuleTarget > attribute with empty values. > > A minor comment on UserModuleTest.java is that > Object.class.getModule().getDescriptor() would be a simpler way to get the > descriptor for the base module. That’s an alternative. Layer::findModule is simple too. I will leave it as is. Thanks. Mandy
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
> On Jan 30, 2017, at 1:36 PM, Doug Simon wrote: > > >> On 30 Jan 2017, at 21:55, Mandy Chung wrote: >> >> >>> On Jan 30, 2017, at 10:38 AM, Doug Simon wrote: >>> >>> I’ve extended the webrev with that change - please re-review: >>> >>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev >>> >> >> +1 > > Thanks. Is that a “Reviewed”? > Sorry. I only noticed now that you added this to UPGRADEABLE_MODULE. Please add it only to PLATFORM_MODULES list instead. Making it an upgradeable module is a separate issue. I suggest you reopen JDK-8171448. Specifically, since upgradeable modules are not tied with java.base, our goal for JDK 9 is to eliminate qualified exports from JDK modules to upgradeable modules, e.g. JDK-8170116, JDK-8166745, JDK-8161549. > I think I should get at least one sign-off from the security team. > Hope Sean will review this one. Please send an updated webrev. > Also, since this is effectively making jdk.vm.compiler an upgradeable module, No it does not. > what’s the implication for it being a hash-checked module? When a module M is recorded in the ModuleHashes attribute of java.base, the runtime will check if module M resolved in the graph matches the one tied with java.base when created at build time; if not, it will fail. If an upgradeable module > It seems like these changes effectively achieve what I was requesting with > https://bugs.openjdk.java.net/browse/JDK-8171448. JDK-8145337 is about the security permission. It’s better to separate this review from JDK-8171448. Mandy > > -Doug > >> >>> Strangely, there was no existing declaration of jdk.vm.compiler in >>> Modules.gmk. >>> >> >> Default is to be defined by the application class loader. The build will >> find all modules from the source. There is no need to list all modules. >> >>> BTW, I never answered your question: >>> >>> "How does JVMCI call out to jdk.vm.compiler? does it load classes using >>> Class::forName with the system class loader?” >>> >>> It uses JVMCIServiceLocator[1] which is a mechanism built on the standard >>> ServiceLoader. >> >> Thanks for the pointer. That confirms my understanding that loads the >> service providers using the system class loader. >> >> Mandy >
