On Fri, 25 Oct 2024 19:59:47 GMT, Mandy Chung <mch...@openjdk.org> wrote:
> I see you add the last line in the help message.
>
> ```
> Capabilities: +run-time-image
> ```
>
> This needs some discussion/consideration how that information be conveyed.
OK. It's currently part of the JEP, though which should explain what it does.
Open for suggestions, though. I've taken inspiration from JVM features which
use a similar `+/-` model.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 526:
>
>> 524: }
>> 525: } catch (IOException e) {
>> 526: throw new InternalError("Failed to process run-time image
>> resources " +
>
> For unexpected exceptions, some code throws InternalError and some throws
> AssertionError - better to be consistent and I think implementation typically
> throws InternalError. For IOException, I think `UncheckedIOException` is
> appropriate.
I've changed it to AssertionError, but can change it to UncheckedIOException
instead. Will take another pass on this issue tomorrow.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JmodsReader.java
> line 41:
>
>> 39:
>> 40: @SuppressWarnings("try")
>> 41: public class JmodsReader implements JimageDiffGenerator.ImageResource {
>
> Is JmodsReader leftover from previous implementation? It can be removed?
Removed. As is ImageReader. They were both used when reading from jmods
directory a modules image respectively. Neither is the case any more and we
only use `ResourcePoolReader` now.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/RuntimeImageLinkException.java
> line 29:
>
>> 27:
>> 28: /**
>> 29: * Exception thrown for links without packaged modules. I.e. run-image
>> link.
>
> I found inconsistent terminologies "run-image" "run-time" "JDK run-time".
> Better to use consistent terminologies.
>
> For this one, maybe as simple as this:
>
> Suggestion:
>
> * Exception thrown for linking without packaged modules
Should be fixed.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/RuntimeImageLinkException.java
> line 38:
>
>> 36: private final IllegalArgumentException iae;
>> 37:
>> 38: public RuntimeImageLinkException(IllegalArgumentException cause) {
>
> Can you explain why `RuntimeImageLinkException` constructor takes IAE. The
> places throwing RILE has to create IAE. I can't see how IAE is necessary as
> other places would catch RILE and throw IAE or IOException instead.
>
> Can it simply take an exception message parameter? And possibly change
> `JlinkTask::run` in handling RILE like IAE?
>
>
> } catch (IllegalArgumentException | ResolutionException |
> RuntimeImageLinkException e) {
> log.println(taskHelper.getMessage("error.prefix") + " " +
> e.getMessage());
> if (DEBUG) {
> e.printStackTrace(log);
> }
> return EXIT_ERROR;
> ```
This was left-over from previous code. It's now just taking a string.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2448399505
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823404901
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823401864
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823400499
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823399864
