On Wed, 18 Aug 2021 21:21:06 GMT, Calvin Cheung <cche...@openjdk.org> wrote:
> Please review this change for adding a `jlink` command line option > `--generate-cds-archive` for generating CDS archives as a post processing > step during the creation of a custom JDK image. > > Details can be found in the corresponding CSR > [JDK-8269178](https://bugs.openjdk.java.net/browse/JDK-8269178). > > Testing: > > - [x] tiers 1,2 (including the new test) src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java line 89: > 87: private final List<String> args; > 88: private final Set<String> modules; > 89: private Platform platform; Can `DefaultExecutableImage` constructor take an additional `platform` argument and make this `platform` field final? When the `DefaultExecutableImage` is constructed, it already has the target platform information. In the constructor, it can check if the `platform` parameter must not be `UNKNOWN`; otherwise throw IAE. src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java line 134: > 132: > 133: @Override > 134: public void setTargetPlatform(Platform p) { This setter is not needed if it's passed to the constructor. src/jdk.jlink/share/classes/jdk/tools/jlink/builder/ImageBuilder.java line 82: > 80: * Gets the target image platform. > 81: * > 82: * @return Platform Suggestion: * @return {@code Platform} object representing the platform of the runtime image src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 70: > 68: * @param p > 69: */ > 70: public void setTargetPlatform(Platform p); This method should not be needed. The platform should be known when an executable image is created. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 75: > 73: * The Platform of the image. > 74: * > 75: * @return Platform Suggestion: * @return {@code Platform} object representing the platform of the runtime image src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 82: > 80: * Checks if the image is 64-bit. > 81: * > 82: * @return boolean Suggestion: * @return true if it's a 64-bit platform src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 84: > 82: * @return boolean > 83: */ > 84: public boolean is64Bit(); This `is64Bit` method should belong to `Platform` class src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginStack.java line 195: > 193: public void operate(ImageProvider provider) throws Exception { > 194: ExecutableImage img = provider.retrieve(this); > 195: img.setTargetPlatform(imageBuilder.getTargetPlatform()); See above comment - the ExecutableImage should know the platform when it's constructed. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 55: > 53: * Returns the {@code Platform} based on the platformString of the > form <operating system>-<arch>. > 54: */ > 55: public static Platform parseTargetPlatform(String platformString) { This method can be renamed back to `parsePlatform` (an earlier version). My suggestion to name this method as `parseTargetPlatform` no longer apply in this version. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CDSPlugin.java line 80: > 78: status = p.waitFor(); > 79: } catch (Exception ex) { > 80: ex.printStackTrace(); This plugin should fail with any exception. jlink will handle `PluginException` and `UncheckedIOException` and print the error message. You can simply wrap IOException with `UncheckedIOException` try { Process p = builder.inheritIO().start(); status = p.waitFor(); } catch (IOException e) { throw new UncheckedIOException(e); } src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CDSPlugin.java line 83: > 81: } > 82: if (status == 0) { > 83: System.out.println("Created " + archiveMsg + " archive > successfully"); Is it significant to print two lines on 64-bit platform? Users may not have any idea what `NOCOOPS` means? Created CDS archive successfully Created NOCOOPS CDS archive successfully It seems adequate to me as a user to get one output: Created CDS archive successfully src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CDSPlugin.java line 121: > 119: @Override > 120: public ResourcePool transform(ResourcePool in, ResourcePoolBuilder > out) { > 121: in.transformAndCopy((file) -> { This method should not be called in a post-processor. Plugin API needs some re-thinking to support post-processor plugin. As `Plugin::transform` is abstract method, for now this method should simply throw `UnsupportedOperationException`. src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 90: > 88: \ the runtime image. > 89: > 90: main.opt.generate-cds-archive=\ This should not be needed. Leftover from an early version. src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties line 143: > 141: > 142: generate-cds-archive.description=\ > 143: CDS plugin: generate cds archives (classes.jsa, classes_nocoops.jsa).\n\ Is it necessary to show `classes.jsa, classes_nocoops.jsa` the archive file name? Simply drop them? src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties line 148: > 146: generate-cds-archive.usage=\ > 147: \ --generate-cds-archive Generate CDS archives (classes.jsa, > classes_nocoops.jsa).\n\ > 148: \ Applicable to JDK images built with the CDS > feature. Suggestion: \ --generate-cds-archive Generate CDS archives if the runtime image supports CDS feature.\n\ Does this match what you want to say? test/jdk/tools/jlink/plugins/CDSPluginTest.java line 66: > 64: String sep = File.separator; > 65: String osName = System.getProperty("os.name"); > 66: if (System.getProperty("os.name").startsWith("Windows")) { Since this test already depends on `jdk.tools.jlink.internal`, maybe it can use `Platform::runtime` to get the runtime platform. test/jdk/tools/jlink/plugins/CDSPluginTest.java line 80: > 78: String jlinkPath = JDKToolFinder.getJDKTool("jlink"); > 79: String[] cmd = {jlinkPath, "--add-modules", > "java.base,java.logging", > 80: "-J-Dos.name=windows", > "--generate-cds-archive", Is there a better way of setting `os.name` system property on the command line? Maybe override `ModuleTarget` attribute in module-info.class of java.base to be a different platform? ------------- PR: https://git.openjdk.java.net/jdk/pull/5174