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

made --add-options and --vendor-* jlink plugins persistent #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dougxc
Copy link
Owner

@dougxc dougxc commented Sep 6, 2022

Changes in this PR:

  • The --add-options option value is persistent: A --add-options value specified when creating image1 is prepended to the --add-options value specified when running image1/bin/jlink.
  • The --vendor-* option values are persistent.

@AlanBateman
Copy link

At a high-level, having the --add-options and --vendor-XXX options persistent seems okay. A few things (like the protected fields) could be cleaned up but I think the main thing that will be important to include is tests to ensure that options persistent and are combined correctly.

@dougxc
Copy link
Owner Author

dougxc commented Sep 7, 2022

Thanks for the feedback. I can now proceed with adding tests given that you don't see any major issue with the solution.

@mlchung
Copy link

mlchung commented Sep 7, 2022

Making --add-options and --vendor-* options persistent also seems okay to me. As they are jlink options, I wonder if It should store these options in a /jdk.jlink/jdk/internal/jlink/options that will be read by jlink execution and will be overridden by the command line options.

@mlchung
Copy link

mlchung commented Sep 7, 2022

As for --add-options, I think some use cases may not always want all options specified in --add-options to carry to the custom image being created. It may be better to create a new jlink option --preserved-add-options=<options> such that jlink execution from the resulting image will also add --add-options=<options>.

@dougxc
Copy link
Owner Author

dougxc commented Sep 8, 2022

Does this capture your suggestion @mlchung ?

  --add-options <options>   Prepend the specified <options> string, which may
                            include whitespace, after --preserved-add-options but
                            before any other options when invoking the virtual machine
                            in the resulting image.

  --preserved-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 and in any further
                            image created by running jlink in the resulting image.

If not, could you please modify this help text to accurately capture your suggestion.

@mlchung
Copy link

mlchung commented Sep 9, 2022

Having more thought, a simpler solution may be to add --save-argfile that will store the content of argfile into jdk.jlink/jdk/internal/jlink/options that will be read and prepended to the jlink execution of the resulting image.

i.e. you can put --vendor-xxx and --add-options options in an argfile and invoke

$ jlink --module-path jmods --add-modules java.base,jdk.jlink --out image1 --save-argfile @argfile

@dougxc
Copy link
Owner Author

dougxc commented Sep 9, 2022

Are you suggesting that the contents of @argfile are processed by both the current jlink execution as well as a jlink execution in the target image?
What's more, how would a SaveArgfilePlugin or a SaveArgfileOption modify the arguments in situ being processed by jlink? Looking at the jlink option parsing loop, it's not clear how to do this. Maybe jdk.tools.jlink.internal.TaskHelper.Option.Processing.process(T, String, String) needs to be expanded to support updating the args array?

@mlchung
Copy link

mlchung commented Sep 9, 2022

Are you suggesting that the contents of @argfile are processed by both the current jlink execution as well as a jlink execution in the target image?

Yes. jlink currently supports @argfile; if specified, the content of the argfile is processed by the current jlink execution. If --save-argfile is specified, the contents of the argfile will be saved and prepended to the jlink execution in the target image.

@mlchung
Copy link

mlchung commented Sep 9, 2022

The argfile support is in the java launcher so jlink doesn't know anything about the argfiles, e.g. jlink @argfile will cause jlink to be invoked with the arguments obtained from @argfile. Need to investigate how to do that. The change will be more involved.

@mlchung
Copy link

mlchung commented Sep 13, 2022

@dougxc not sure if you are waiting for me. Two ideas to support this:

  1. have the native launcher to create VM with -Djdk.argfile.arguments=<...> and VM startup time will remove that system property. jlink can read that from jdk.internal.misc.VM::getSavedProperties.

  2. add an internal Java API to parse the @argfile (using the javac implementation) but that has some subtle behavioral difference. jlink can call that Java API to parse the argfile to get the parsed options.

@dougxc
Copy link
Owner Author

dougxc commented Sep 14, 2022

The -Djdk.argfile.arguments=<...> idea sounds reasonable. In that scenario I assume --save-argfile becomes a no-arg option?

@mlchung
Copy link

mlchung commented Sep 14, 2022

The -Djdk.argfile.arguments=<...> idea sounds reasonable. In that scenario I assume --save-argfile becomes a no-arg option?

yes and only if @file is specified.

@dougxc
Copy link
Owner Author

dougxc commented Sep 14, 2022

So this captures it?

  --save-argfiles           Saves all arguments specified in @argfiles and
                            prepends them to the command line when invoking
                            in the output image. If no argfiles are on the command
                            line, this option does nothing.

@mlchung
Copy link

mlchung commented Sep 14, 2022

Yes. Minor tweak.

  --save-argfiles           Saves all arguments specified in @argfiles and
                            prepends them to the jlink command line when invoking
                            in the output image. If no argfile is on the command
                            line, this option does nothing.

dougxc pushed a commit that referenced this pull request Oct 31, 2024
Reviewed-by: honkar, prr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants