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

8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option #8747

Closed
wants to merge 3 commits into from

Conversation

asotona
Copy link
Member

@asotona asotona commented May 17, 2022

Problem description

Minimal jdk image created by jlink with the only jdk.compiler module and its dependencies
fails to run java source launcher correctly (for example when --source N is specified).
Failing source launcher is only one the expressions of internal jdk.compiler malfunction, another example is failure to run javac with --release option.

Root cause

Module jdk.compiler requires zip filesystem (jdk.zipfs module) to parse ct.sym file and so to provide full functionality.
Module jdk.zipfs is not included in the minimal JDK image, because module jdk.compiler does not declare it as "requires" in its module info.

Alternative patch

The problem can be alternatively resolved by complex code checks in jdk.compiler to provide user with valid error message, however that solution would be just a workaround for jdk.compiler dual functionality (with or without presence of jdk.zipfs module). Compiler functionality without access to ct.sym through jdk.zipfs is very limited.

Proposed fix

This patch fixes the problem by explicit declaration of jdk.compiler dependency on jdk.zipfs.
Plus added specific test case.

Please review the patch or raise objections against declaration of jdk.compiler dependent on jdk.zipfs.

Thanks,
Adam


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8747/head:pull/8747
$ git checkout pull/8747

Update a local copy of the PR:
$ git checkout pull/8747
$ git pull https://git.openjdk.java.net/jdk pull/8747/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8747

View PR using the GUI difftool:
$ git pr show -t 8747

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8747.diff

….compiler fails with --enable-preview option
@bridgekeeper
Copy link

bridgekeeper bot commented May 17, 2022

👋 Welcome back asotona! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option May 17, 2022
@openjdk openjdk bot added the rfr label May 17, 2022
@openjdk
Copy link

openjdk bot commented May 17, 2022

@asotona The following labels will be automatically applied to this pull request:

  • compiler
  • core-libs
  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot-runtime core-libs compiler labels May 17, 2022
@mlbridge
Copy link

mlbridge bot commented May 17, 2022

Webrevs

@jaikiran
Copy link
Member

jaikiran commented May 18, 2022

Hello Adam, I don't have necessary knowledge of this area, so I don't know what approach would be preferred.

But a couple of questions:

  • If we do decide to add the jdk.zipfs dependency to jdk.compiler module, do you think the LimitedImage test is still relevant? Or should it be removed? The comment on that test states:

@summary Verify javac behaves properly in absence of zip/jar FileSystemProvider

and it does so by using --limit-modules. But now with the jdk.zipfs being a dependency, the zip/jar FileSystemProvider will not be absent and the test then would seem unnecessary.

  • In the JBS issue corresponding to this PR, you noted that:

There are actually two different issues:

First in JDKPlatformProvider, which silently swallows ProviderNotFoundException.

Should something be done there? I see that the catch block happens to reside in the static block of that class (which also happens to be a implementation of a Java service), so I'm unsure what if anything can be done there.

@dholmes-ora
Copy link
Member

dholmes-ora commented May 18, 2022

/label remove hotspot-runtime

Nothing hotspot-runtime related here

@openjdk openjdk bot removed the hotspot-runtime label May 18, 2022
@openjdk
Copy link

openjdk bot commented May 18, 2022

@dholmes-ora
The hotspot-runtime label was successfully removed.

@asotona
Copy link
Member Author

asotona commented May 18, 2022

Hello Adam, I don't have necessary knowledge of this area, so I don't know what approach would be preferred.

But a couple of questions:

  • If we do decide to add the jdk.zipfs dependency to jdk.compiler module, do you think the LimitedImage test is still relevant? Or should it be removed? The comment on that test states:

@summary Verify javac behaves properly in absence of zip/jar FileSystemProvider

and it does so by using --limit-modules. But now with the jdk.zipfs being a dependency, the zip/jar FileSystemProvider will not be absent and the test then would seem unnecessary.

You are right, the test description should change. The test name is LimitedImage and it verifies that javac behaves properly in limited JDK image. I think the purpose of the test is still the same, just results are different (now it pass compilation instead of failing with specific error).

Thanks for pointing it out.

  • In the JBS issue corresponding to this PR, you noted that:

There are actually two different issues:
First in JDKPlatformProvider, which silently swallows ProviderNotFoundException.

Should something be done there? I see that the catch block happens to reside in the static block of that class (which also happens to be a implementation of a Java service), so I'm unsure what if anything can be done there.

Silent swallow of an exception in static initializer of a service is not very good practice and it has significant effect on this issue right now. However it will express itself only in corner cases (like for example broken ct.sym file) after this patch is integrated.

@lahodaj
Copy link
Contributor

lahodaj commented May 20, 2022

FWIW, if adding this dependency would be seen unacceptable, we could try to tweak jlink to at least produce a warning when building an image with jdk.compiler but without jdk.zipfs.

@jaikiran
Copy link
Member

jaikiran commented May 21, 2022

Thank you for changing the description of the test. That part now looks fine to me.

@plevart
Copy link
Contributor

plevart commented May 22, 2022

My humble opinion: if java.compiler needs jdk.zipfs for full functionality (i.e. using --source N or --release X), does that mean java.compiler is useless without jdk.zipfs? If there is a meaningful functionality left in java.compiler even without jdk.zipfs, then someone might want to use that limited subset of functionality and might not want to include jdk.zipfs in its image.
So what is possible to do with java.compiler without jdk.zipfs?

@plevart
Copy link
Contributor

plevart commented May 22, 2022

My humble opinion: if java.compiler needs jdk.zipfs for full functionality....

Sorry, I wrongly assumed it was java.compiler (the library module), but this is about jdk.compiler (the tool)... Nevertheless, the same question applies: Does jdk.compiler have any meaningful functionality even without jdk.zipfs ?

@asotona
Copy link
Member Author

asotona commented May 23, 2022

My humble opinion: if java.compiler needs jdk.zipfs for full functionality....

Sorry, I wrongly assumed it was java.compiler (the library module), but this is about jdk.compiler (the tool)... Nevertheless, the same question applies: Does jdk.compiler have any meaningful functionality even without jdk.zipfs ?

Basic compilation works even without presence of jdk.zipfs, however such stripped JDK is for example unable to read jar files on classpath.

@AlanBateman
Copy link
Contributor

AlanBateman commented May 23, 2022

Sorry, I wrongly assumed it was java.compiler (the library module), but this is about jdk.compiler (the tool)... Nevertheless, the same question applies: Does jdk.compiler have any meaningful functionality even without jdk.zipfs ?

This came up in JDK 9 and the decision at the time was not to require jdk.zipfs as it's a service provider module. The addition of the source launcher, the addition of --enable-preview, and more use of --release N means more ct.sym usage. It probably isn't too interesting to have a small runtime image that contains jdk.compiler and not jdk.zipfs so Adam's proposal mightn't be too bad, it's just a bit weird and not something that we'd want other libraries in the eco system to do.

@asotona
Copy link
Member Author

asotona commented May 31, 2022

Followup issues to narrow the situation with optional module dependencies have been filled:

Please review if you agree with actual patch adding jdk.compiler dependency on jdk.zips.

lahodaj
lahodaj approved these changes Jun 7, 2022
Copy link
Contributor

@lahodaj lahodaj left a comment

Looks like a reasonable compromise to me.

@openjdk
Copy link

openjdk bot commented Jun 7, 2022

@asotona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option

Reviewed-by: jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 316 new commits pushed to the master branch:

  • 8d28734: 8287741: Fix of JDK-8287107 (unused cgv1 freezer controller) was incomplete
  • f1dd559: 8287896: PropertiesTest.sh fail on msys2
  • 4fe0ca9: 8287860: Revise usage of volatile in j.u.Locale
  • bde7a7a: 8287236: Reorganize AST related to pattern matching for switch
  • 2d8c649: 8287663: Add a regression test for JDK-8287073
  • b647a12: 8286940: [IR Framework] Allow IR tests to build and use Whitebox without -DSkipWhiteBoxInstall=true
  • dbf0905: 8286967: Unproblemlist compiler/c2/irTests/TestSkeletonPredicates.java and add additional test for JDK-8286638
  • 39fa52b: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature
  • 42261d7: 8287785: Reduce runtime of java.lang.invoke microbenchmarks
  • b6c6cc5: 8286360: ARM32: Fix crashes after JDK-8284161 (Virtual Threads)
  • ... and 306 more: https://git.openjdk.java.net/jdk/compare/5e5500cbd79b40a32c20547ea0cdb81ef6904a3d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 7, 2022
@asotona
Copy link
Member Author

asotona commented Jun 7, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jun 7, 2022

Going to push as commit 905bcbe.
Since your change was applied there have been 316 commits pushed to the master branch:

  • 8d28734: 8287741: Fix of JDK-8287107 (unused cgv1 freezer controller) was incomplete
  • f1dd559: 8287896: PropertiesTest.sh fail on msys2
  • 4fe0ca9: 8287860: Revise usage of volatile in j.u.Locale
  • bde7a7a: 8287236: Reorganize AST related to pattern matching for switch
  • 2d8c649: 8287663: Add a regression test for JDK-8287073
  • b647a12: 8286940: [IR Framework] Allow IR tests to build and use Whitebox without -DSkipWhiteBoxInstall=true
  • dbf0905: 8286967: Unproblemlist compiler/c2/irTests/TestSkeletonPredicates.java and add additional test for JDK-8286638
  • 39fa52b: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature
  • 42261d7: 8287785: Reduce runtime of java.lang.invoke microbenchmarks
  • b6c6cc5: 8286360: ARM32: Fix crashes after JDK-8284161 (Virtual Threads)
  • ... and 306 more: https://git.openjdk.java.net/jdk/compare/5e5500cbd79b40a32c20547ea0cdb81ef6904a3d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jun 7, 2022
@openjdk openjdk bot closed this Jun 7, 2022
@openjdk openjdk bot removed ready rfr labels Jun 7, 2022
@openjdk
Copy link

openjdk bot commented Jun 7, 2022

@asotona Pushed as commit 905bcbe.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler core-libs integrated
6 participants