-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
8298266: "java.home property not set" error in Graal when sun.awt.fontconfig java property is set on Windows #11559
8298266: "java.home property not set" error in Graal when sun.awt.fontconfig java property is set on Windows #11559
Conversation
…ava property is set on Windows
👋 Welcome back alexsch! A progress list of the required criteria for merging this PR into |
@AlexanderScherbatiy The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
I can reproduce the bug and assume that this change will fix that exception. But after this change, we will skip initialization of the "javaLib" if the user config was passed, and as a result, we will skip initialization of the fallback fonts by the getInstalledFallbackFonts. I do not know should if fallback fonts are used if the user explicitly set config or not. |
Could you review the updated fix? The |
@AlexanderScherbatiy This change is no longer ready for integration - check the PR body for details. |
/reviewers 2 |
Why is it not set ? That seems wrong. |
There is the With comment: oracle/graal#2835 (comment)
|
Surely that's graal's problem to solve. |
The table in System.getProperties() lists the standard system properties. java.home is not optional. It's not hard to find code that reads the value of this property and it would be a major compatibility issue to change java.home to be optional. |
Graal uses a substitutor which replaces FontConfiguration.findFontConfigFile() method to empty on Linux. The idea of the fix was to allow to use |
FYI here's an almost 9 year old bug report where I make clear the unsupported nature of that property. https://bugs.openjdk.org/browse/JDK-8038987 Seems this PR should be withdrawn and the bug report closed as external or not an issue. |
If this property is not optional why do we have a null check for it? Can that check be removed as a part of refactoring? |
It is much better to have an error thrown which explains the problem rather than a random NPE or similar right afterwards. |
But I assume we should have a test somewhere that validates that this option is not null = "is not optional"?
We have an opportunity to clean up all that code. |
Maybe core libs could add such a test. The link doesn't work for me but I expect I'd need to look at each of those cases to understand what they are doing. It doesn't change that the reason for this proposed change isn't valid. |
It is tested by conformance tests but a test could be added to the OpenJDK jtreg tests if needed. |
Environment:
GraalVM 22.3.0 with jdk 19, Windows OS.
Description:
Create a native image of Swing application using GraalVM and run it with
-Dsun.awt.fontconfig=fontconfig.properties.src
java property.java.lang.Error: java.home property not set
error is thrown.For more details see oracle/graal#4875.
Solution:
The error is thrown by the code from FontConfiguration.findFontConfigFile() method.
GraalVM does not set
java.home
property, that is why thejava.lang.Error: java.home property not set
is thrown.FontConfiguration.findFontConfigFile()
method comparesjava.home
property to null before checking user providedsun.awt.fontconfig
java property.The proposed fix swaps the order of
java.home
andsun.awt.fontconfig
properties checking to avoid usingjava.home
property in casesun.awt.fontconfig
is set.Steps to reproduce:
SwingSample.java
samplegraalvm-ce-java19-22.3.0\lib\fontconfig.properties.src
file to the sample dir-Djava.awt.headless=false
,-Dsun.awt.fontconfig=fontconfig.properties.src
java properties-Dsun.awt.fontconfig=fontconfig.properties.src
java property.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11559/head:pull/11559
$ git checkout pull/11559
Update a local copy of the PR:
$ git checkout pull/11559
$ git pull https://git.openjdk.org/jdk pull/11559/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11559
View PR using the GUI difftool:
$ git pr show -t 11559
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11559.diff