dougxc / jdk

GNU General Public License v2.0
0 stars 0 forks source link

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

Open dougxc opened 2 years ago

dougxc commented 2 years ago

Changes in this PR:

AlanBateman commented 2 years ago

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 commented 2 years ago

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

mlchung commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

mlchung commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.