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

8247872: Upgrade HarfBuzz to the latest 2.7.2 #993

Closed
wants to merge 3 commits into from
Closed

8247872: Upgrade HarfBuzz to the latest 2.7.2 #993

wants to merge 3 commits into from

Conversation

@prrace
Copy link
Contributor

@prrace prrace commented Nov 2, 2020

This upgrades JDK to import the current 2.7.2 version of harfbuzz - an OpenType text shaping library

https://bugs.openjdk.java.net/browse/JDK-8247872

This has passed building and headless and headful automated tests on all platforms.


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 2, 2020

👋 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.

@prrace
Copy link
Contributor Author

@prrace prrace commented Nov 2, 2020

Aside from the code change there is a small build change is to remove some obsolete defines

@openjdk openjdk bot added the rfr label Nov 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2020

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

  • 2d
  • build
  • compiler

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2020

Webrevs

magicus
magicus approved these changes Nov 2, 2020
Copy link
Member

@magicus magicus left a comment

Build changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2020

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

8247872: Upgrade HarfBuzz to the latest 2.7.2

Reviewed-by: serb

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

  • c5462bb: 8255989: Remove explicitly unascribed authorship from Java source files
  • 358f5d2: 6422025: ThemeReader.cpp can be updated for VC7
  • a9dff94: 8254864: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java timed out
  • 0b7fba7: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c
  • f5d36e6: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
  • 688b10b: 8255561: add tests to check binary compatibility rules for records
  • 727a69f: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags
  • 952abea: 8254920: Application launched with jpackage produced .exe crashes JVM
  • 5dfb42f: 8255563: Missing NULL checks after JDK-8233624
  • e730e8b: 8241806: The sun/awt/shell/FileSystemViewMemoryLeak.java is unstable
  • ... and 123 more: https://git.openjdk.java.net/jdk/compare/d0867578344380cf9424937acd8a3382966b1995...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.

Copy link
Member

@magicus magicus left a comment

I'm just a bit curious about the added, empty, src/java.desktop/share/native/libharfbuzz/abc.txt...

If it really is in upstream source, I'm not saying you should remove it. It just looks very odd. It's not a merge artifact?

(I could not even add review comments to an empty file in github! 😮)

@openjdk openjdk bot added ready and removed ready labels Nov 2, 2020
@prrace
Copy link
Contributor Author

@prrace prrace commented Nov 2, 2020

I'm just a bit curious about the added, empty, src/java.desktop/share/native/libharfbuzz/abc.txt...

If it really is in upstream source, I'm not saying you should remove it. It just looks very odd. It's not a merge artifact?

(I could not even add review comments to an empty file in github! 😮)

oops. that should not have been in that folder. It was me practicing a different white space removal script.
I thought I checked for any extraneous files. I'll get rid of it now.

@openjdk openjdk bot removed the compiler label Nov 2, 2020
@mrserb
Copy link
Member

@mrserb mrserb commented Nov 5, 2020

Looks like the build on macOS via GitHub will be broken by this fix?

@magicus
Copy link
Member

@magicus magicus commented Nov 5, 2020

@mrserb Good catch! Yes indeed, it looks like range-loop-analysis needs to be disabled for clang for Xcode 12 to not complain.

@prrace
Copy link
Contributor Author

@prrace prrace commented Nov 5, 2020

I added that to the disabled warnings .. now I suppose the actions will re-run but I don't see any sign of it yet.
Just that the results that were there are now gone.
A few thoughts on this

  • github sends so many messages that it is easy to miss the important ones
  • using the default for github we are at the whim of when ever they switch and if this change
    had been pushed 6 weeks ago likely it would have passed until the day github upgraded.
  • it also means passing a github build does not guarantee it will pass on the "official" toolchains.

@magicus
Copy link
Member

@magicus magicus commented Nov 5, 2020

@prrace I agree, the github actions are a double-edged sword -- at best. At worst, it's a clear regression in terms of how we work. :-(

We might need to take a step back and rethink this.

Anyway, your changes looks good from a build PoV now.

mrserb
mrserb approved these changes Nov 8, 2020
@openjdk openjdk bot added the ready label Nov 8, 2020
@prrace
Copy link
Contributor Author

@prrace prrace commented Nov 8, 2020

/integrate

@openjdk openjdk bot closed this Nov 8, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2020

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

  • 6a183fb: 8255562: delete UseRDPCForConstantTableBase
  • c5462bb: 8255989: Remove explicitly unascribed authorship from Java source files
  • 358f5d2: 6422025: ThemeReader.cpp can be updated for VC7
  • a9dff94: 8254864: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java timed out
  • 0b7fba7: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c
  • f5d36e6: 8246741: NetworkInterface/UniqueMacAddressesTest: mac address uniqueness test failed
  • 688b10b: 8255561: add tests to check binary compatibility rules for records
  • 727a69f: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags
  • 952abea: 8254920: Application launched with jpackage produced .exe crashes JVM
  • 5dfb42f: 8255563: Missing NULL checks after JDK-8233624
  • ... and 124 more: https://git.openjdk.java.net/jdk/compare/d0867578344380cf9424937acd8a3382966b1995...master

Your commit was automatically rebased without conflicts.

Pushed as commit ed7526a.

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

@prrace prrace deleted the hb272 branch Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants