On Sun, 12 Nov 2023 11:37:31 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Add a message when a run-image based link is being performed >> - Add hint to the modified file case >> - AddJmodResourcesPlugin => AddRunImageResourcesPlugin rename >> - --add-jmod-resources => --add-run-image-resources rename >> >> Also rename jmod_resources to runimage_resources > > I looked at the latest proposal and CSR. Overall I think the proposal is good > and you've got the right default to fail if the run-time image has been > modified. So I think we are down to a few lesser topics now. > > I'm wondering if the CLI option to override, meaning a first run-time image > containing the jdk.jlink module generates a second run-time image, and the > first run-time image has been modified, whether this is really needed. I'm > wondering about this because this CLI option is hard to explain, will > developers really understand it? If there is an CLI option then we'll need to > find a better name, I don't think "--unlock-run-time" works as a name (the > usage of options talk about "runtime image" for example, maybe > "-ignore-signing-information" can provide inspiration as an override too). > > Can --add-run-image-resources be dropped? Exposing this in the interface > feels like it's an attractive nuisance and not clear (to me anyway) what > developers would do with this. > > A few very minor things that I jotted down while looking at the current > proposal: > > - Adding a resource to serve as a marker that indicates it was created > without the packaged modules is fine. I think the name should be looked as > "runimage" is a bit consistent for this area. I'm also wondering if it would > be better to hide in jdk/internal somewhere to avoid any tooling assuming > it's a supported interface. > > - The error message exclaims "Please double check!", I think that error > message will need to be tweaked once we get down to details like this. @AlanBateman Thanks for having taken another look! > I looked at the latest proposal and CSR. Overall I think the proposal is good > and you've got the right default to fail if the run-time image has been > modified. So I think we are down to a few lesser topics now. Glad to hear it. Hopefully we can still get this into JDK 22. > I'm wondering if the CLI option to override, meaning a first run-time image > containing the jdk.jlink module generates a second run-time image, and the > first run-time image has been modified, whether this is really needed. I'm > wondering about this because this CLI option is hard to explain, will > developers really understand it? If there is an CLI option then we'll need to > find a better name, I don't think "--unlock-run-time" works as a name (the > usage of options talk about "runtime image" for example, maybe > "-ignore-signing-information" can provide inspiration as an override too). I think we need the option. Be it only for users wanting to validate links with and without packaged modules present (as one of the tests does). How about we call the option `--force-run-image-link`? Or maybe `--ignore-base-run-image-changes`. I'm not very good with coming up with names. > Can --add-run-image-resources be dropped? Exposing this in the interface > feels like it's an attractive nuisance and not clear (to me anyway) what > developers would do with this. It's not clear what you mean by that. Dropped from what? The CSR? Something else? The run-image based link needs the `--add-run-image-resources` plugin for it to work. > A few very minor things that I jotted down while looking at the current > proposal: > > * Adding a resource to serve as a marker that indicates it was created > without the packaged modules is fine. I think the name should be looked as > "runimage" is a bit consistent for this area. I'm also wondering if it would > be better to hide in jdk/internal somewhere to avoid any tooling assuming > it's a supported interface. How does `jdk/internal/runimage` as a name sound? > * The error message exclaims "Please double check!", I think that error > message will need to be tweaked once we get down to details like this. Sure. I can just drop that phrase. ------------- PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1808398661
