jdk8u-dev icon indicating copy to clipboard operation
jdk8u-dev copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8193017: Import freetype sources into OpenJDK source tree

Open gdams opened this issue 2 years ago • 27 comments
trafficstars

Imports the Freetype source to be consistent with JDK11+

As discussed in https://github.com/ibmruntimes/openj9-openjdk-jdk8/pull/631 CC @gnu-andrew

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] JDK-8193017 needs maintainer approval

Issue

  • JDK-8193017: Import freetype sources into OpenJDK source tree (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/318/head:pull/318
$ git checkout pull/318

Update a local copy of the PR:
$ git checkout pull/318
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/318/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 318

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/318.diff

Webrev

Link to Webrev Comment

gdams avatar May 24 '23 10:05 gdams

:wave: Welcome back gdams! 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.

bridgekeeper[bot] avatar May 24 '23 10:05 bridgekeeper[bot]

This backport pull request has now been updated with issue from the original commit.

openjdk[bot] avatar May 24 '23 10:05 openjdk[bot]

Webrevs

mlbridge[bot] avatar May 24 '23 10:05 mlbridge[bot]

Good to see this being proposed. I was wondering what had happened to it.

How close is this to JDK-8193017? Were other changes necessary on top?

I'll do a comparison of the two patches when I get a chance.

gnu-andrew avatar May 31 '23 17:05 gnu-andrew

Mailing list message from Andrew Hughes on jdk8u-dev:

On 20:03 Wed 31 May , Thorsten Glaser wrote:

On Wed, 31 May 2023, Andrew John Hughes wrote:

Good to see this being proposed. I was wondering what had happened to it.

As long as there is a flag to still use the system library, and I can figure out how and where to add it in the build process?

bye, //mirabilos -- Infrastrukturexperte ? tarent solutions GmbH Am Dickobskreuz 10, D-53121 Bonn ? http://www.tarent.de/ Telephon +49 228 54881-393 ? Fax: +49 228 54881-235 HRB AG Bonn 5168 ? USt-ID (VAT): DE122264941 Gesch?ftsf?hrer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

                    \*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*

/?\ The UTF-8 Ribbon ??? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen: ??? HTML eMail! Also, https://www.tarent.de/newsletter ??? header encryption! ****************************************************

Given 8u is a stable release, I don't intend for any defaults to change, so you shouldn't have to add any new options.

This is primarily aimed at Windows, where FreeType is not readily available on the system and FreeType sources have to be supplied manually to the JDK build.

Thanks, -- Andrew :) Pronouns: he / him or they / them Principal Free Java Software Engineer OpenJDK Package Owner Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222

Please contact via e-mail, not proprietary chat networks -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <https://mail.openjdk.org/pipermail/jdk8u-dev/attachments/20230601/18c9724d/signature-0001.asc>

mlbridge[bot] avatar Jun 02 '23 01:06 mlbridge[bot]

Good to see this being proposed. I was wondering what had happened to it.

How close is this to JDK-8193017? Were other changes necessary on top?

I'll do a comparison of the two patches when I get a chance.

it's reasonably close, the main difference is that the freetype source is located in jdk/src/share/native/sun/awt/libfreetype/ rather than jdk/src/share/native/libfreetype to be consistent with other JDK8u sources. I also removed freetype task from the GitHub actions workflow.

gdams avatar Jun 06 '23 14:06 gdams

@gdams This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jul 04 '23 15:07 bridgekeeper[bot]

Please keep this open

gdams avatar Jul 04 '23 20:07 gdams

@gdams This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Aug 01 '23 22:08 bridgekeeper[bot]

@gdams This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar Aug 30 '23 03:08 bridgekeeper[bot]

probably we should keep it open?

mrserb avatar Aug 30 '23 05:08 mrserb

/open

gdams avatar Jul 12 '24 11:07 gdams

@gdams This pull request is now open

openjdk[bot] avatar Jul 12 '24 11:07 openjdk[bot]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

openjdk[bot] avatar Jul 12 '24 11:07 openjdk[bot]

@gdams this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout freetype
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Jul 12 '24 11:07 openjdk[bot]

@gdams This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Aug 09 '24 14:08 bridgekeeper[bot]

@gdams do you have a chance to look at the merge-conflict?

mrserb avatar Aug 12 '24 17:08 mrserb

Yup @mrserb I'll try and get to it this week. I must stress that it would be good to make a decision on this as it's very painful to keep such a large changeset up to date

gdams avatar Aug 12 '24 17:08 gdams

@mrserb I've resolved the merge conflicts. I'm slightly worried about the changes to common/autoconf/generated-configure.sh though. I've got [email protected] on my mac (installed using brew) but there might be a more specific version required?

gdams avatar Aug 13 '24 11:08 gdams

@mrserb I've resolved the merge conflicts. I'm slightly worried about the changes to common/autoconf/generated-configure.sh though. I've got [email protected] on my mac (installed using brew) but there might be a more specific version required?

I had a similar issue some time ago https://github.com/openjdk/jdk8u-dev/pull/413#issuecomment-1951600088, at that time I also run 2.69 on macos. Solved that by running autoconf on some linux hosts.

mrserb avatar Aug 13 '24 17:08 mrserb

@mrserb I've resolved the merge conflicts. I'm slightly worried about the changes to common/autoconf/generated-configure.sh though. I've got [email protected] on my mac (installed using brew) but there might be a more specific version required?

I had a similar issue some time ago #413 (comment), at that time I also run 2.69 on macos. Solved that by running autoconf on some linux hosts.

that looks better, I've compiled it locally with the patch applied and the generated configure looks correct now

gdams avatar Aug 13 '24 20:08 gdams

I did a quick test on macOS. If the "libfreetype.dylib" is deleted from some old jdk8 build(or jdk-21) the next exception is occurred for the font demo:

./jdk-8/bin/java -jar ./build/macosx-x86_64-normal-server-release/images/j2sdk-image/demo/jfc/Font2DTest/Font2DTest.jar Exception in thread "main" java.lang.UnsatisfiedLinkError: /Users/openjdk/jdk8u-dev/jdk-8/jre/lib/libfontmanager.dylib: dlopen(/Users/openjdk/jdk8u-dev/jdk-8/jre/lib/libfontmanager.dylib, 0x0001): Library not loaded: @rpath/libfreetype.dylib.6 Referenced from: <E45B410C-8F6D-3350-90ED-7D918CD703B6> /Users/openjdk/jdk8u-dev/jdk-8/jre/lib/libfontmanager.dylib Reason: tried: '/Users/openjdk/jdk8u-dev/jdk-8/jre/lib/./libfreetype.dylib.6' (no such file), '/Users/openjdk/jdk8u-dev/jdk-8/jre/lib/./libfreetype.dylib.6' (no such file), '/Users/openjdk/jdk8u-dev/jdk-8/jre/lib/server/./libfreetype.dylib.6' (no such file), '/Users/openjdk/jdk8u-dev/jdk-8/jre/lib/server/../libfreetype.dylib.6' (no such file), '/Users/openjdk/jdk8u-dev/jdk-8/bin/./libfreetype.dylib.6' (no such file), '/usr/lib/libfreetype.dylib.6' (no such file, not in dyld cache) at java.lang.ClassLoader$NativeLibrary.load(Native Method) at java.lang.ClassLoader.loadLibrary0(ClassLoader.java:1934) at java.lang.ClassLoader.loadLibrary(ClassLoader.java:1838) at java.lang.Runtime.loadLibrary0(Runtime.java:871) at java.lang.System.loadLibrary(System.java:1124) at sun.lwawt.macosx.LWCToolkit$1.run(LWCToolkit.java:93) at sun.lwawt.macosx.LWCToolkit$1.run(LWCToolkit.java:80) at java.security.AccessController.doPrivileged(Native Method) at sun.lwawt.macosx.LWCToolkit.(LWCToolkit.java:79) .... at java.awt.Toolkit.getDefaultToolkit(Toolkit.java:854) .... at Font2DTest.main(Font2DTest.java:1031)

But If I delete the "libfreetype.dylib" from the build after this patch the demo works fine.

mrserb avatar Aug 15 '24 00:08 mrserb

But If I delete the "libfreetype.dylib" from the build after this patch the demo works fine.

Using "DYLD_PRINT_LIBRARIES=YES" traced that the "Homebrew" version of freetype is loaded. it is unclear why this version is not loaded in case of jdk-21. @rpath looks the same.

mrserb avatar Aug 15 '24 01:08 mrserb

Using "DYLD_PRINT_LIBRARIES=YES" traced that the "Homebrew" version of freetype is loaded. it is unclear why this version is not loaded in case of jdk-21. @rpath looks the same.

nevermind seems that in case of "macos+Homebrew" the jdk-21 is affected as well but only if I build jdk locally. tested on windows and works fine.

mrserb avatar Aug 15 '24 02:08 mrserb

it will be good to test it on solartis and aix.

mrserb avatar Aug 15 '24 23:08 mrserb

This one is outside the usual rules for 8u, which is closed for enhancements. For that reason, it must be accompanied by a very strong justification, and evidence that it carries no risk. By now, I'd have thought that everyone building JDK 8 on Windows etc. had workflows in place. We are at the (long) tail of the JDK 8 life cycle.

theRealAph avatar Aug 16 '24 13:08 theRealAph

This one is outside the usual rules for 8u, which is closed for enhancements. For that reason, it must be accompanied by a very strong justification, and evidence that it carries no risk. By now, I'd have thought that everyone building JDK 8 on Windows etc. had workflows in place. We are at the (long) tail of the JDK 8 life cycle.

I'm not sure I'd call this an enhancement. The lack of in-tree sources for FreeType means that there is no reference version of FreeType to build and test against, as there is with other libraries used by the AWT classes (libjpeg, libpng, giflib, LCMS).

Historically, this is because Oracle did not use FreeType and its maintenance was largely left to others. There is evidence of this in this patch with the only JDK code changes being to allow FreeType to be used on non-OpenJDK builds (largely irrelevant to use I would expect)

Yes, those on Windows & Mac likely have some way of building against FreeType in place, but it is an ongoing burden to maintain that version of FreeType, and it means every person building OpenJDK potentially uses some different version of FreeType.

It's also an issue on the GNU/Linux side as there is no option not to use a system FreeType, which potentially reduces the portability of such builds. A build of OpenJDK may take place against one system installation of FreeType and then be used against a completely different one.

The reason these issues have not been a bigger problem up to now is FreeType is a pretty stable library. I think the risk of introducing this now is still very low (as I say, there are very few code changes at all) and it improves the maintainability of 8u for everyone.

gnu-andrew avatar Aug 21 '24 13:08 gnu-andrew

@gdams This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Sep 27 '24 06:09 bridgekeeper[bot]

Keep open. Would be good to get this in.

gnu-andrew avatar Oct 07 '24 16:10 gnu-andrew

@gdams This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Nov 04 '24 22:11 bridgekeeper[bot]