Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8218685: jlink --list-plugins needs to be readable and tidy #305

Closed
wants to merge 6 commits into from

Conversation

@igraves
Copy link
Member

@igraves igraves commented Sep 22, 2020

These changes update the jlink plugin command line documentation to tidy them up into a more canonical form.

The output generated by jlink from this change appears as follows:

> build/macosx-x64/images/jdk/bin/jlink --list-plugins

List of available plugins:
  --add-options <options>   Prepend the specified <options> string, which may
                            include whitespace, before any other options when
                            invoking the virtual machine in the resulting image.
  --compress <0|1|2>[:filter=<pattern-list>]
                            Compress all resources in the output image.
                            Level 0: No compression
                            Level 1: Constant string sharing
                            Level 2: ZIP.
                            An optional <pattern-list> filter can be
                            specified to list the pattern of 
                            files to be included.
  --dedup-legal-notices [error-if-not-same-content]
                            De-duplicate all legal notices.
                            If error-if-not-same-content is specified then
                            it will be an error if two files of the same
                            filename are different.
  --exclude-files <pattern-list>
                            Specify files to exclude.
                            e.g.: **.java,glob:/java.base/lib/client/**
  --exclude-jmod-section <section-name>
                            Specify a JMOD section to exclude.
                            Where <section-name> is "man" or "headers".
  --exclude-resources <pattern-list>
                            Specify resources to exclude.
                            e.g.: **.jcov,glob:**/META-INF/**
  --generate-jli-classes @filename
                            Specify a file listing the java.lang.invoke
                            classes to pre-generate. By default, this plugin
                            may use a builtin list of classes to pre-generate.
                            If this plugin runs on a different runtime version
                            than the image being created then code generation
                            will be disabled by default to guarantee 
                            correctness add ignore-version=true
                            to override this.
  --include-locales <langtag>[,<langtag>]*
                            BCP 47 language tags separated by a comma,
                            allowing
                            locale matching defined in RFC 4647.
                            e.g.: en,ja,*-IN
  --order-resources <pattern-list>
                            Order resources. 
                            e.g.: **/module-info.class,@classlist,
                            /java.base/java/lang/**
  --release-info <file>|add:<key1>=<value1>:<key2>=<value2>:...|del:<key list>
                            <file> option is to load release properties from
                            the supplied file.
                            add: is to add properties to the release file.
                            Any number of <key>=<value> pairs can be passed.
                            del: is to delete the list of keys in release file.
  --strip-debug             Strip debug information from the output image
  --strip-java-debug-attributes 
                            Strip Java debug attributes from
                            classes in the output image
  --strip-native-commands   Exclude native commands (such as java/java.exe)
                            from the image.
  --system-modules retainModuleTarget
                            Fast loading of module descriptors (always enabled)
  --vendor-bug-url <vendor-bug-url>
                            Override the vendor bug URL baked into the build.
                            The value of the system property
                            "java.vendor.url.bug" will be <vendor-url-bug>.
  --vendor-version <vendor-version>
                            Override the vendor version string baked into the
                            build,if any. The value of the system property
                            "java.vendor.version" will be <vendor-version>.
  --vendor-vm-bug-url <vendor-vm-bug-url>
                            Override the vendor VM bug URL baked 
                            into the build.  The URL displayed in VM error
                            logs will be <vendor-vm-bug-url>.
  --vm <client|server|minimal|all>
                            Select the HotSpot VM in the output image.
                            Default is all

For options requiring a <pattern-list>, the value will be a comma separated
list of elements each using one the following forms:
  <glob-pattern>
  glob:<glob-pattern>
  regex:<regex-pattern>
  @<filename> where filename is the name of a file containing patterns to be
              used, one pattern per line


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8218685: jlink --list-plugins needs to be readable and tidy

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/305/head:pull/305
$ git checkout pull/305

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 22, 2020

👋 Welcome back igraves! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@igraves The following label will be automatically applied to this pull request: core-libs.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added the core-libs label Sep 22, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 22, 2020

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 23, 2020

The output looks much better. Have you considered to sort them in alphabetical order of the plugin name?

@igraves
Copy link
Member Author

@igraves igraves commented Sep 23, 2020

Yes! I had intended to but it looks like I got hung up making sure non-documented plugins came last. Will push a change for this.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 23, 2020

Can we put a base abstract class in jdk.tools.jlink.internal.plugins that implements getUsage. That would avoid needing to have the same getUsage method in all the plugins.

The text in the value of some of the .usage keys looks like it will wrap significantly, can the values of release-info.usage and strip-native-debug-symbols.usage be re-formatted so that the usage is readable with a default terminal?

@igraves
Copy link
Member Author

@igraves igraves commented Sep 23, 2020

I'll shrink narrow those descriptions down to 80 characters.

As to the abstract class, I think it'd make sense to have that implement getUsage, getName, and getDescription -- essentially combine all of those very similar things in the same place.

@igraves
Copy link
Member Author

@igraves igraves commented Sep 23, 2020

Updated to consolidate documentation/resource-related methods into an abstract base class and shortened the mentioned usage bodies to 80 chars or less.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 24, 2020

AbstractPlugin might be a better name for the abstract class as it will likely define methods beyond access to name, description and usage. It also needs a copyright header.
StripNativeDebugSymbolsPlugin may have been missed in the update, it can extend the abstract class too.
AddResourcePlugin: the name field can be removed, also should be good to rename "n" to "name" so it's a bit clearer.
TaskHelper - the IDE may have messed up the imports when it was edited.

Copy link
Member

@mlchung mlchung left a comment

I suggest to update the PR description with the new jlink --show-plugins output.

import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.*;
Copy link
Member

@mlchung mlchung Sep 24, 2020

I assume this change is unintentional. Probably IDE converted the single-class imports to this.


import jdk.tools.jlink.plugin.Plugin;

public abstract class DocumentedPlugin implements Plugin {
Copy link
Member

@mlchung mlchung Sep 24, 2020

I agree with Alan that AbstractPlugin is better.

This should also provide the default implementation of getArgumentsDescription() such that the plugins don't need to implement.


public abstract class DocumentedPlugin implements Plugin {

private final String NAME;
Copy link
Member

@mlchung mlchung Sep 24, 2020

Nit: use lowercase for final instance field name.

@@ -46,23 +46,19 @@
*
* Order Resources plugin
*/
public final class OrderResourcesPlugin implements Plugin {
public final class OrderResourcesPlugin extends DocumentedPlugin {
public static final String NAME = "order-resources";
Copy link
Member

@mlchung mlchung Sep 24, 2020

This is not related to your change. It can be private static final field. In my past observation, this static final NAME field is defined in every plugin which isn't really a need for it (at least in the current implementation). It might be good to consider the clean up to remove this static final field and instead call getName() to get the name. Just a thought.

Copy link
Member

@mlchung mlchung left a comment

Looks good to me. It may be useful to have a regression test to verify the plugin order of the output if possible.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2020

@igraves This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8218685: jlink --list-plugins needs to be readable and tidy

Reviewed-by: mchung, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 109 new commits pushed to the master branch:

  • 2fe0a5d: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities
  • fb20690: 8253637: Update EC removal
  • b1ce6bd: 8253548: jvmFlagAccess.cpp: clang 9.0.0 format specifier error
  • ff6843c: 8253714: [cgroups v2] Soft memory limit incorrectly using memory.high
  • d5be829: 8253770: Test tools/javac/parser/JavacParserTest.java fails on Windows after JDK-8253584
  • 6e5d4f3: 8253607: [mlvm] meth/func/jdi/breakpointOtherStratum: un-problemlist and add randomness keyword
  • 3ed960e: 8253640: Make MEMFLAGS an enum class
  • 86491a5: 8253584: Redunant errors for partial member selects
  • ebf443a: 8253590: java/foreign tests are still failing on x86_32 after foreign-memaccess integration
  • 431338b: 8212107: VMThread issues and cleanup
  • ... and 99 more: https://git.openjdk.java.net/jdk/compare/2e30ff61b0394793655d6c6a593a5de8eb9c07b9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung, @AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Sep 28, 2020
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 29, 2020

The new AbstractPlugin and each of the implementations extending it look good. The new usages messages look good too. A few minor formatting nits in a few places but otherwise I think good to go.
/approve

@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@AlanBateman Unknown command approve - for a list of valid commands use /help.

@igraves
Copy link
Member Author

@igraves igraves commented Sep 29, 2020

Updated JLinkTest to assert alphabetical order. With that I will /integrate .

@igraves
Copy link
Member Author

@igraves igraves commented Sep 29, 2020

/integrate

@openjdk openjdk bot added the sponsor label Sep 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@igraves
Your change (at version 5372b15) is now ready to be sponsored by a Committer.

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 29, 2020

/sponsor

@openjdk openjdk bot closed this Sep 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@mlchung @igraves Since your change was applied there have been 109 commits pushed to the master branch:

  • 2fe0a5d: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities
  • fb20690: 8253637: Update EC removal
  • b1ce6bd: 8253548: jvmFlagAccess.cpp: clang 9.0.0 format specifier error
  • ff6843c: 8253714: [cgroups v2] Soft memory limit incorrectly using memory.high
  • d5be829: 8253770: Test tools/javac/parser/JavacParserTest.java fails on Windows after JDK-8253584
  • 6e5d4f3: 8253607: [mlvm] meth/func/jdi/breakpointOtherStratum: un-problemlist and add randomness keyword
  • 3ed960e: 8253640: Make MEMFLAGS an enum class
  • 86491a5: 8253584: Redunant errors for partial member selects
  • ebf443a: 8253590: java/foreign tests are still failing on x86_32 after foreign-memaccess integration
  • 431338b: 8212107: VMThread issues and cleanup
  • ... and 99 more: https://git.openjdk.java.net/jdk/compare/2e30ff61b0394793655d6c6a593a5de8eb9c07b9...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8df3e72.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants