On Tue, 19 Sep 2023 17:39:04 GMT, Alan Bateman <al...@openjdk.org> wrote:
> I did a pass over this to see where this proposal is currently at. At a > high-level I think good progress since the discussion on leyden-dev some time > ago. A few comments this from this pass: Thanks, Alan! > If I read it correctly, the default behavior is now to fail if jlink is > executed from a run-time image that has been modified from the original and > the packaged modules are not observable on the module path (this includes the > default module path effectively appended by jlink). That seems the right > policy as it avoids modifications by plugins or conf changes silently > propagating. Correct. > If I read the code correctly, the error when a hash doesn't match is a very > terse "Run image links only allow single-hop" so that probably needs to be > looked to make sure the message says that the run-time image has been > modified and maybe include one of the files/resources that has changed so > there is something real to go with the error. Not quite. See the example I've added in the [previous comment](https://github.com/openjdk/jdk/pull/14787#issuecomment-1673592876). It already includes that info. It looks like this: Error: java.lang.IllegalArgumentException: /disk/openjdk/upstream-sources/git/jdk-jdk/build/jmodless-copy-jmods-removed/conf/net.properties has been modified. Please double check! The message can of course be modified what we think is best. > The command line options to override and change the error to a warning or > silently ignore seems to be "-run-time-ignore-single-hop" and > "--run-image-only-warnings". Maybe it should be reduced to just one option > and maybe it should contain the word "clone" to something that makes it more > obvious that it's just copying or cloning the contents of the modules in the > run-time image (I suspect "single hop" won't be understood). I believe keeping them both makes sense. Or at least make it so that it takes an argument to be able to distinguish between the two. The reason why I think this is useful is in the container case, where say - the JDK got installed by an installer - which didn't include packaged modules, but is otherwise a full JDK (with all modules). The installer already does integrity checking of the files and it might be desirable to do multi-hop evolutions, while still wanting to get warnings/errors on modified *configuration* files, for example. Happy to change the name of that flag/flags to `--clone-run-image=ignore-multi,warn-only` or something similar. > Adding a new jdk.tools.jlink.internal.Archive implementation where the > resources come from the run-time image seems a sensible approach. I don't > particularly like the name "JmodLessArchive" because the original module may > not have been packaged as a JMOD file, it may have been a modular JAR. Same > comment on the BOM file "jmod_resources" added to the top-level directory of > each module. OK. How about `RunimageArchive` and `runimage_resources` or `clone_resources`? > I think it's clever to add this to each module in the container image but > part of me wonders if it should be hidden in some way to avoid tools > depending on it. I don't have a concrete suggestion except to wonder if jrtfs > should filter it out, same thing in the SystemModuleReader code as > ModuleReader.find("jmod_resources") will find the resource, could be an > attractive nuisance. That seems fine. I can add code to filter those resources out. On the other hand, no special handling is being made for `jdk/internal/vm/options`. Perhaps that should be handled uniformly? ------------- PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1741195090
