On Wed, 18 Aug 2021 23:55:25 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @mlchung comments
>
> 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.
I've added the `platform` argument and made the `platform` field final.
However, as we've discussed offline, there's a code path via the
`--post-process-path` option where the platform may not be available. So we
can't throw IAE on an `UNKNOWN` platform in the constructor.
> 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.
Fixed.
> 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
Fixed.
> 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.
Fixed.
> 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
Done.
> 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
Fixed.
> 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
Done. I've moved the method to the `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.
Removed the code.
> 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.
Changed the name back to `parsePlatform`.
> 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);
> }
I've changed it to the following since we also need to catch the
`InterruptedException`:
try {
Process p = builder.inheritIO().start();
status = p.waitFor();
} catch (InterruptedException | IOException e) {
throw new PluginException(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
I've made the change; the plugin will print only one successful message.
> 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`.
It is being called. The minimal implementation is:
@Override
public ResourcePool transform(ResourcePool in, ResourcePoolBuilder out) {
return in;
}
I've filed [JDK-8272734](https://bugs.openjdk.java.net/browse/JDK-8272743) to
follow-up the issue.
> 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.
Removed.
> 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?
Removed the file names.
> 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?
Yes.
> 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.
I'm using the `jdk.test.lib.Platform` so that `if (Platform.isWindows())` can
be used to check if the platform is Windows.
> 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?
I'd prefer to leave it as is for now since it's in a test case and I don't
think the code is complex.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5174
