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

8255790: GTKL&F: Java 16 crashes on initialising GTKL&F on Manjaro Linux #2982

Closed
wants to merge 3 commits into from
Closed

Conversation

prrace
Copy link
Contributor

@prrace prrace commented Mar 13, 2021

From a build perspective this partially reverts https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps
the harfbuzz sources separate and still supports building and running against a system harfbuzz which is only of interest or relevance on Linux.

I ended up having to go this way because its is the least unsatisfactory solution.
I did not want us to build a devkit to link against a system linux only to find we couldn't use it at runtime
because too many systems have to old a version of harfbuzz.

This solves the Manjaro Linux problem and I've manually verified building against a system hardbuxz on Ubuntu 20.10

There are couple of incidental fixes in here too

  • "libharfbuzz" should not have been in the EXTRA_HEADERS var when building against a system version
  • harfbuzz/hb-ucdn is gone and should not be listed as a header directory needed to build the bundled copy
  • I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8255790: GTKL&F: Java 16 crashes on initialising GTKL&F on Manjaro Linux

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2982/head:pull/2982
$ git checkout pull/2982

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 13, 2021

👋 Welcome back prr! 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 label Mar 13, 2021
@openjdk
Copy link

openjdk bot commented Mar 13, 2021

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

  • 2d
  • build

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 2d build labels Mar 13, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 13, 2021

Webrevs

mrserb
mrserb approved these changes Mar 13, 2021
@openjdk
Copy link

openjdk bot commented Mar 13, 2021

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

8255790: GTKL&F: Java 16 crashes on initialising GTKL&F on Manjaro Linux

Reviewed-by: serb, ihse, azvegint

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

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 Mar 13, 2021
@magicus
Copy link
Member

magicus commented Mar 15, 2021

It feels a bit unfortunate that we had to end up this way. :-( I've read through the bug report and unnderstand that this might be the least bad way... but still.

Just thinking out loud, you don't think it would be better to build harfbuzz separately, but as a static library, that is then included in libfontmanager? The main win of doing that, I think, is the ability to have all the disabled warnings confined to the lower-quality upstream source. The resulting code would be the same. And from a build performance perspective I don't think any way of doing it matters.

Copy link
Contributor Author

@prrace prrace left a comment

Just thinking out loud, you don't think it would be better to build harfbuzz separately, but as a static library, that is then included in libfontmanager? The main win of doing that, I think, is the ability to have all the disabled warnings confined to the lower-quality upstream source. The resulting code would be the same. And from a build performance perspective I don't think any way of doing it matters.

I don't know that this would be worth such an effort. I've clearly separated the warnings we are disabling for HARFBUZZ and there's really not a lot of likelihood in my mind that these warnings will suddenly also start to apply to the small amount of JDK code that is in libfontmanager which is also much simpler code.

BTW I noticed we are still disabling all warnings for sunFont.c and after this is done I'll likely see if I can make a source code change to resolve that.

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved
@magicus
Copy link
Member

magicus commented Mar 15, 2021

I was actually thinking that such a change would be simpler; more or less amounting to changing from LIBRARY to STATIC_LIBRARY. But if you feel that it is too much work, fine...

@prrace
Copy link
Contributor Author

prrace commented Mar 15, 2021

I was actually thinking that such a change would be simpler; more or less amounting to changing from LIBRARY to STATIC_LIBRARY. But if you feel that it is too much work, fine...

If you say so .. I have no idea what build changes would be needed. I'm just more familiar and comfortable with this and leaving aside the graal stuff I don't know if it is used like that anywhere by the main JDK build.

@prrace
Copy link
Contributor Author

prrace commented Mar 16, 2021

/integrate

@openjdk openjdk bot closed this Mar 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 16, 2021
@openjdk
Copy link

openjdk bot commented Mar 16, 2021

@prrace Since your change was applied there have been 102 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 05fe06a.

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

@prrace prrace deleted the hb_bld branch Apr 30, 2021
@gnu-andrew
Copy link
Member

gnu-andrew commented Sep 15, 2021

You mention that "too many systems have to old a version of harfbuzz".
Is the required version defined somewhere? There's no check in configure for either a version or a required symbol:

PKG_CHECK_MODULES([HARFBUZZ], [harfbuzz], [HARFBUZZ_FOUND=yes], [HARFBUZZ_FOUND=no])

With undefined symbols also being left to runtime (hence why JDK-8272332 doesn't show up during build), it seems this could lead to sporadic runtime failures if OpenJDK is unknowingly built against a harfbuzz that is too old.

@mlbridge
Copy link

mlbridge bot commented Sep 15, 2021

Mailing list message from Philip Race on build-dev:

You are right there's no check. One could be added by a motivated party ..
The minimum for Linux may be as old as 1.2.3? but safer is 2.3.1 since
we rely on that for AAT font support.
I can't (quickly) speak to any important bug fixes in later releases we
may need, just API / functionality.

-phi.

On 9/15/21 12:47 PM, Andrew John Hughes wrote:

@magicus
Copy link
Member

magicus commented Sep 23, 2021

@gnu-andrew Maybe I'm misunderstanding here, but if symbol lookup is done at runtime, then the failure would occur if the JDK is run on a system with a too old harfbuzz, not if it is built on it. So it seems you want runtime checks, not build-time checks, right?

@gnu-andrew
Copy link
Member

gnu-andrew commented Oct 6, 2021

Yes, it would fail at runtime, perhaps in subtle ways if the issue is not a missing function but a difference in behaviour.

In the scenario I'm considering, the build and runtime environments are effectively the same, as we're building packages for Fedora or RHEL on a specific version of that OS, which is then provided for that OS alone. We tend to use system libraries where possible as we know the library is going to be the same or newer than what we built against, and we benefit from fixes to the system library without rebuilding OpenJDK.

Rather than relying on failures being found in an older environment at runtime, I'd like the build to check it is being built against is suitable. With that early build failure, we can then flip those builds to using the in-tree library.

You do, however, make a good point that this check can be overridden at runtime if the build is run on a different system. So, if possible, it would be good to also have a runtime check if there is some clear entry point to add one.

I'll try and look into a fix for both once the October security update cycle is out of the way. That's taking up most of my time right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d build integrated
5 participants