-
Notifications
You must be signed in to change notification settings - Fork 173
8193017: Import freetype sources into OpenJDK source tree #709
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
Conversation
|
👋 Welcome back jvanek! A progress list of the required criteria for merging this PR into |
|
@judovana This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: 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 no new commits pushed to the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@gnu-andrew) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
8657ac1 to
c2a41ed
Compare
|
@judovana Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
This backport pull request has now been updated with issue from the original commit. |
|
|
|
Good. The builds passed. I somehow do not see the issue in the tier1. I had run them on many OSes and many platforms, and all looks good. intel: Noni-ntels may look wilder, but there is no new failure. And they are mostly not running GUI anyway... |
|
Also all the full jtregs finished on all arches x osses I have handy. The |
|
Would be nice to have in-tree freetype as in later jdks As when person is not careful bundling freetype, it can end up linking against system hafrbuzz (which, in turn, has cyclic dependency on freetype). This may, in better case, fail the build. In worst case, build may seemingly look ok, but cause runtime linking error (in description), when run on newer OS (newer than build one). For reference, issue and workaround in temurin-build: Also (and older freetype such as |
Are the failing jtregs. Do not seem to be related though Am also digging in 32windows time-outs, and can not see culprit. @zzambers any ideas? |
There are some unstable tests on x86 (32-bit) and probably unrelated to your PR. Segfault in |
|
btw. there is also issue with Windows x86 build jobs, where VS2010 installation hangs and jobs get cancelled. I have created PR for that. |
gnu-andrew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this over. In like manner, I have leveraged my previous review and just compared the diff for your PR with the one for #318. I see building.html is now updated. I also think your choice of keeping the example and removing the freetype part is a better idea than removing the entire thing, even if it deviates from the backported change.
I do think we should test this on Windows & VS2010 before merging, so I will approve this once #712 & #713 are merged and this PR is updated with those changes. In the meantime, please use the contributor command to credit @gdams for his work here.
Thanks for analysiing the test results and debugging this stalling build job. I observed it when merging the security update in, but had no idea what was causing it. I've reviewed #712 & #713 so we should be able to get them in shortly and then Jiri can merge them into his branch. |
|
/contributor add @gdams |
|
@judovana |
I had been testing it on windows, but sure, no objection to wait. Thank you!
done! |
On VS2010? Patches are now merged so just merge into your tree and push, please. |
|
nope Sorry. Mergin in progress |
c2a41ed to
0c7f5b2
Compare
|
@judovana Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
x86 windows still do not look healthy |
|
It seems to have resolved itself now and is working on my PR, #719 |
Co-authored-by: George Adams <[email protected]>
0c7f5b2 to
c6a920c
Compare
|
thanx! Rerunning. |
|
@judovana Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@gnu-andrew it is green!-) @zzambers ty! |
|
@judovana Wow, completely green, even security infra stuff and all 32-bit tests. Have not seen such for a long time here. Lucky run :) |
|
You know.. great PR :). But I think luck had nothing to do with it. Thank you for fixing them. |
gnu-andrew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing looks good.
Clearly GHA is eager for FreeType support too ;) |
|
/approve yes |
|
@gnu-andrew |
|
/integrate |
|
Note that I'm not commiter, so would be nice somebody sponsors this. ty! Looking forward to see this in! |
|
/sponsor |
|
Going to push as commit 335fce5. |
|
@judovana, are you planning to work on the followup issues to bump the freetype version to match the one used in jdk11+? |
|
@mrserb hi! I have no objections to do so, and test that. if it is wanted, which I guess it is. That would be directly to https://bugs.openjdk.org/browse/JDK-8351538 ? Or one by one? Generally any gudidance to save both mine and reviewers cycles Welcomed. Thanx for pointing it out. I wanted to do it as part of the rebase, but Andrew advised against, and that was good intermediate step. Is it supposed to target before January CPU,or rather later? |
|
@mrserb Unless you advice differently, I will be backporting one by one, not necessasrily doing PR with each minor (eg 8203367, 2.9.1 is CVE IIReadC) , but will be testing build-ability on old systems. |
|
@mrserb Despite from build system, only " is present in 2.9.1, it requires openjdk/jdk11u@f74d0ae#diff-a634b106e57d9efbd1270664b8b79b2048b035a109b6f5220c4d39a4293d4458 thoughts? I guess I will bring it on jdk8u mailing list... Started here: #720 |
Hi!
I had recently hit an issue with jdk8, which, if build on old system - eg el7, it can not run GUI application on newest system like f42 or similar:
As per investigations the issue is in duality of freetype, and is solved by
8193017: Import freetype sources into OpenJDK source treeThis is fully based on unfinished #318 , with smal lexception of rebase, and docs adaptation. The change in
doc/building.htmlis unluckily done manually, asmake update-build-docsis regenerating half of the universum. Is it desired? Is there some recomended version of pandoc to use?I had tested builds on windows and several linuxes, run full jtregs and checked the GUI works again. I can link full set of tests if needed (actually many of them are still running).
For record, I built by temurin build wrapper, which needed an change to adapt to this: adoptium/temurin-build#4287
Progress
Issue
Reviewers
Contributors
<[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/709/head:pull/709$ git checkout pull/709Update a local copy of the PR:
$ git checkout pull/709$ git pull https://git.openjdk.org/jdk8u-dev.git pull/709/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 709View PR using the GUI difftool:
$ git pr show -t 709Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/709.diff
Using Webrev
Link to Webrev Comment