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

8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distribution #12260

Closed

Conversation

AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Jan 27, 2023

JDK-8278549 UNIX sun/font coding misses SUSE distro detection on recent distro SUSE 15 adds SuSE detection by checking SLES os name property in /etc/os-release file.

opensuse/leap:15.4 docker defines os name property as "openSUSE Leap" in /etc/os-release file which is not recognized as SuSE.

The issue is reproduced with Oracle jdk-19.0.2 with custom fontconfig.SuSE.properties file copied to jdk-19.0.2/lib directory.

The fix checks if os name property from /etc/os-release contains SUSE substring.

Steps to reproduce.

  • Download Oracle jdk-19.0.2
  • Copy custom fontconfig.SuSE.properties file to jdk-19.0.2/lib directory.
  • Run the opensuse/leap:15.4 docker and install freetype and dejavu fonts (do not install fontconfig)
docker run --rm --security-opt seccomp=unconfined -it opensuse/leap:15.4 bash
zypper install -y dejavu-fonts
zypper install -y freetype2
  • Run HelloImage java sample in the docker
import javax.imageio.ImageIO;
import java.awt.*;
import java.awt.image.BufferedImage;
import java.io.File;

public class HelloImage {

    public static void main(String[] args) throws Exception {

        BufferedImage buff = new BufferedImage(300, 200, BufferedImage.TYPE_INT_RGB);
        Graphics2D g = buff.createGraphics();
        g.setColor(Color.WHITE);
        g.fillRect(0, 0, buff.getWidth(), buff.getHeight());

        g.setColor(Color.BLUE);
        g.setFont(g.getFont().deriveFont(32f));
        g.drawString("Hello, Image!", 50, 50);
        g.dispose();

        File file = new File("hello-image.png");
        ImageIO.write(buff, "png", file);
    }
}
./jdk-19.0.2/bin/javac HelloImage.java
./jdk-19.0.2/bin/java HelloImage
Exception in thread "main" java.lang.NullPointerException: Cannot load from short array because "sun.awt.FontConfiguration.head" is null
	at java.desktop/sun.awt.FontConfiguration.getVersion(FontConfiguration.java:1261)
	at java.desktop/sun.awt.FontConfiguration.readFontConfigFile(FontConfiguration.java:221)
	at java.desktop/sun.awt.FontConfiguration.init(FontConfiguration.java:105)
	at java.desktop/sun.awt.X11FontManager.createFontConfiguration(X11FontManager.java:706)
	at java.desktop/sun.font.SunFontManager$2.run(SunFontManager.java:352)
	at java.desktop/sun.font.SunFontManager$2.run(SunFontManager.java:309)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
	at java.desktop/sun.font.SunFontManager.<init>(SunFontManager.java:309)
	at java.desktop/sun.awt.FcFontManager.<init>(FcFontManager.java:35)
	at java.desktop/sun.awt.X11FontManager.<init>(X11FontManager.java:56)
	at java.desktop/sun.font.PlatformFontInfo.createFontManager(PlatformFontInfo.java:37)
	at java.desktop/sun.font.FontManagerFactory.getInstance(FontManagerFactory.java:51)
	at java.desktop/java.awt.Font.getFont2D(Font.java:526)
	at java.desktop/java.awt.Font$FontAccessImpl.getFont2D(Font.java:265)
	at java.desktop/sun.font.FontUtilities.getFont2D(FontUtilities.java:151)
	at java.desktop/sun.java2d.SunGraphics2D.checkFontInfo(SunGraphics2D.java:671)
	at java.desktop/sun.java2d.SunGraphics2D.getFontInfo(SunGraphics2D.java:837)
	at java.desktop/sun.java2d.pipe.GlyphListPipe.drawString(GlyphListPipe.java:46)
	at java.desktop/sun.java2d.SunGraphics2D.drawString(SunGraphics2D.java:2931)
	at HelloImage.main(HelloImage.java:17)

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-8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distribution

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12260/head:pull/12260
$ git checkout pull/12260

Update a local copy of the PR:
$ git checkout pull/12260
$ git pull https://git.openjdk.org/jdk pull/12260/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12260

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12260.diff

@AlexanderScherbatiy AlexanderScherbatiy changed the title UNIX sun/font coding does not detect SuSE in openSUSE Leap distributive 8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distributive Jan 27, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 27, 2023

👋 Welcome back alexsch! 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 added the rfr Pull request is ready for review label Jan 27, 2023
@openjdk
Copy link

openjdk bot commented Jan 27, 2023

@AlexanderScherbatiy The following label will be automatically applied to this pull request:

  • client

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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Jan 27, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 27, 2023

Webrevs

@prrace
Copy link
Contributor

prrace commented Jan 30, 2023

it is distribution not distributive.

And I made a suggestion in the bug that what I'd prefer to see is that
this be done by parsing the /etc/release and using the exact name there rather
than remapping it.
This means fontconfig files would need to use SLES in the name not SuSE but
that seems a small price to pay for not having to keep editing OpenJDK code.

@AlexanderScherbatiy AlexanderScherbatiy changed the title 8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distributive 8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distribution Jan 31, 2023
@AlexanderScherbatiy
Copy link
Author

AlexanderScherbatiy commented Jan 31, 2023

A table with ID and Name values for some docker images:

Docker image ID NAME
opensuse/leap:15.4 opensuse-leap openSUSE Leap
opensuse/tumbleweed opensuse-tumbleweed openSUSE Tumbleweed
fedora:35 fedora Fedora Linux
ubuntu:20.04 ubuntu Ubuntu
photon:4.0 photon VMware Photon OS
archlinux arch Arch Linux
centos:8 centos CentOS Linux
alpine:3.17 alpine Alpine Linux
mstormo/suse sles SLES

@AlexanderScherbatiy
Copy link
Author

May be it has sense to add a property file which maps OS name used in fontconfig files to OS names from 'os-release` file.
Something like this:

SuSE=SLES,opensuse-leap,opensuse-tumbleweed

@prrace
Copy link
Contributor

prrace commented Jan 31, 2023

uSE=SLES,opensuse-leap,opensuse-tumbleweed

I am not sure that would even be right.
Fedora and RHEL might need different mappings and so might these too.
So if you want to provide support for both of these, provide 2 files.
I'd like it simple as in "you look at /etc/os-release, and that tells you the name to use", without any more indirections than that.

@AlexanderScherbatiy
Copy link
Author

The proposal is to add one os-mapping.properties file which contains os mapping for all new platforms:

SuSE=SLES,opensuse-leap,opensuse-tumbleweed
Ubuntu=ubuntu
Fedora=fedora
RedHat=rhel

It can be put to the same directory where fontconfig.OS.properties files are placed.

JDK reads /etc/os-release file, gets ID property and finds corresponding mapping in os-mapping.properties file (SuSE for SLES and opensuse-leap, RedHat for rhel, and etc).
What user needs for new OS name is to add one more mapping into os-mapping.properties property file.
It allows to use only one fontconfig.OS.properties file for different OSes from the same family and reuse the old ones.

@mlbridge
Copy link

mlbridge bot commented Feb 1, 2023

Mailing list message from Patrick Chen on client-libs-dev:

anyone can unsubscribe or ban me ? spam

Le mar. 31 janv. 2023 ? 10:21, Alexander Scherbatiy <alexsch at openjdk.org> a
?crit :

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230131/bed8ad57/attachment.htm>

@AlexanderScherbatiy
Copy link
Author

AlexanderScherbatiy commented Feb 7, 2023

Could you review the updated fix?

  • ID property from /etc/os-release file is used for os name when the /etc/os-release file is handled.
  • NAME property for SUSE check is left as is to not break the JDK-8278549 fix.
  • extractOsInfo() method is renamed to extractInfo(). It now checks the input string to null and replaces white characters to underscores.
  • There are operation systems that have both /etc/lsb-release and /etc/os-release files. For example, Photon OS have:

/etc/os-release

NAME="VMware Photon OS"
ID=photon
VERSION_ID=4.0

/etc/lsb-release

DISTRIB_ID="VMware Photon OS"
DISTRIB_RELEASE="4.0"

The extractInfo() method is added for os name/version parsing during /etc/lsb-release file handling as well.

Some improvements can be added.

  • May be it has sense to allow to configure os name for fontconfig.properties file by a Java property. It allows to use a simple custom os name in the fontconfig.properties and reuse the same property file on several systems like SLES, openSUSE Leap, and openSUSE Tumbleweed.
  • Running a java program with -Dsun.java2d.debugfonts=true property lists checked fontconfig.properties files:
INFO: Looking for text fontconfig file : /build/jdk/images/jdk/lib/fontconfig.opensuse-leap.15.4.properties
Feb 07, 2023 5:43:30 PM sun.awt.FontConfiguration findImpl
INFO: Looking for binary fontconfig file : /build/jdk/images/jdk/lib/fontconfig.opensuse-leap.15.4.bfc
Feb 07, 2023 5:43:30 PM sun.awt.FontConfiguration findImpl

It could be a good idea to add logging for os name and version in setOsNameAndVersion() method as well.

  • The code for os name/version detection is duplicated in FcFontConfiguration and MFontConfiguration classes. It would be good to unify them. Though they have different files parsing order:

FcFontConfiguration: lsb-release, redhat-release, SuSE-release, turbolinux-release, fedora-release, os-release
MFontConfiguration: fedora-release, redhat-release, turbolinux-release, SuSE-release, lsb-release, os-release

@prrace
Copy link
Contributor

prrace commented Feb 23, 2023

If I read the code right, what you have now is good enough.

If the code really is duplicated 100% - just with different orders - then unifying seems like a good thing.
Up to you if you do it.
For the ordering I would need to research to be sure, but I'd be inclined towards the order in MFontConfiguration
unless research shows that the names like fedora-release are going out of fashion and everyone is using lsb-release these days.
If they are then I'd move that to the front ..

More logging is also fine.

A mapping file is something I'd leave until there's a demonstrated need for more than the suse family

@openjdk
Copy link

openjdk bot commented Feb 23, 2023

@AlexanderScherbatiy 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:

8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distribution

Reviewed-by: prr

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 432 new commits pushed to the master branch:

  • 1a07871: 8302173: Button border overlaps with button icon on macOS system LaF
  • 796cdd5: 8302677: JFR: Cache label and contentType in EventType and ValueDescriptor
  • 6b24b4a: 8302821: JFR: Periodic task thread spins after recording has stopped
  • 4d33fbd: 8303089: [jittester] Add time limit to IRTree generation
  • f612dcf: 8302512: Update IANA Language Subtag Registry to Version 2023-02-14
  • 6397cb6: 8301200: Don't scale timeout stress with timeout factor
  • 71dd7ea: 8303085: Runtime problem list cleanup
  • 4b6acad: 8302883: JFR: Improve periodic events
  • a2471b3: 8303125: ProblemList vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java
  • 58ca711: 8303078: Reduce allocations when pretty printing JCTree during compilation
  • ... and 422 more: https://git.openjdk.org/jdk/compare/e4252bb91478e9c2f0a5bbdae7cd663838d91b1b...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 Pull request is ready to be integrated label Feb 23, 2023
@AlexanderScherbatiy
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 6, 2023

Going to push as commit 15c76e4.
Since your change was applied there have been 540 commits pushed to the master branch:

  • 3eff1a0: 8303630: Move nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java back to general problem list
  • 10d6a8e: 8299518: HotSpotVirtualMachine shared code across different platforms
  • 148900c: 8303562: Remove obsolete comments in os::pd_attempt_reserve_memory_at
  • 1bb39a9: 8302027: Port fdlibm trig functions (sin, cos, tan) to Java
  • 9fdbf3c: 8303588: [JVMCI] make JVMCI source directories conform with standard layout
  • 629a905: 8303242: ThreadMXBean issues with virtual threads
  • 5b2e2e4: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
  • cd4b88d: 8292269: Replace FileMapInfo::fail_continue() with Unified Logging
  • ae29054: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal
  • a04b104: 8303413: (fs) Ignore polling interval sensitivity modifiers in PollingWatchService
  • ... and 530 more: https://git.openjdk.org/jdk/compare/e4252bb91478e9c2f0a5bbdae7cd663838d91b1b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 6, 2023
@openjdk openjdk bot closed this Mar 6, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 6, 2023
@openjdk
Copy link

openjdk bot commented Mar 6, 2023

@AlexanderScherbatiy Pushed as commit 15c76e4.

💡 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
client client-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants