-
-
Notifications
You must be signed in to change notification settings - Fork 275
adaptation of JDK-8 to build with in-tree freetype #4287
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
base: master
Are you sure you want to change the base?
Conversation
|
For completeness it would be good to detect jdk8u_+ |
You mean to distinguish the freshly released "jdk8 5b3" as it went out 2014 or to distinguish the particular u472 and older x future ones? |
|
being able to still support older jdk8 would be useful for completeness. |
|
That may be easier then to fully support the custom build of free type. If the tag will be present, which usually is, then it can be decided. If not, the future in-tree approach should take place. Or maybe add switch? However to fully remove the custom freetype, would simplify the scripts a lot. I'm still just brainstorming. Am not sure which direction to take. Thanx a lot for inputs! |
the if is to long, will refactor
@karianna WDYT now? It is based on tag, so pretty limited, but had not found a better way. This do not work wit |
db1626c to
09d18b8
Compare
09d18b8 to
ee63d39
Compare
|
I would like to use temurin-build/sbin/prepareWorkspace.sh Line 592 in f4bcbeb
|
|
The openjdk/jdk8u-dev#709 was integrated |
sbin/build.sh
Outdated
| local majorBuildVersion=$(echo "${BUILD_CONFIG[TAG]}" | sed "s/jdk8u//" | sed "s/-.*//") | ||
| if [ 0${majorBuildVersion} -lt 482 ] ; then | ||
| echo "old" | ||
| elif [ 0${majorBuildVersion} -gt 482 ] ; then |
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.
Should this be gt and equal to?
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.
Can we just simplify the logic here to capture 8u<version> and see if <version> is >=482 and then apply the freetype from source? There seems to be excess logic here
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.
Should this be gt and equal to?
Nope. The "eq" is handled in final else. More readable it is:
if majorBuildVersion< 482 ; then //always old
return "old"
elif majorBuildVersion > 482] ; then //always new
return "new"
else // that means ==
decideByMinorBuildNumber()
fi
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.
Can we just simplify the logic here to capture
8u<version>and see if<version>is >=482 and then apply the freetype from source? There seems to be excess logic here
Of course we can. I made it to the most precise granularity, and if "b" is unimportant (which I Actually doubt) then of course. But it was done in early stage of 482, so maybe there can be done one exact match to 482-b01 and 482-b02 to avodi math check on b's XY.
I would like to use
temurin-build/sbin/prepareWorkspace.sh
Line 592 in f4bcbeb
| if bash ./configure --help | grep "with-png"; then |
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.
@karianna Thanx for eyball, highly appreciated!
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.
@judovana Is there a way to tell from the actual "source" what it is doing/supports? I don't know... but say if (some make file exists?!)
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.
although not ideal, you could gate this logic on the existance or not of .github/workflows/freetype.vcxproj ,
which has been deleted as the submit workflow does not support building freetype src with this update?
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.
hi! I would love to, But jdk sources do not exists in this moment. Otherwise check like
temurin-build/sbin/prepareWorkspace.sh
Line 592 in f4bcbeb
| if bash ./configure --help | grep "with-png"; then |
The only other way is to deference all this after jdk sources are built, which may not be trivial, as in "legacy" mode, the certificates are prepared long before jdk.
Tbh I would love to get rid of tag check, because the tag do not cover eg the build from tarball and maybe more.
I'm proposing to merge this, so jdk8 remain buildable, but would elaborate, on change of order.
That woudl mean to get rid of tag parsing, and try to reorder cloning the jdk, and building of cacerts. I already looked into it, and it seems doable. Only I got scared of such big change.
Or I will try to do so asap.
Co-authored-by: Martijn Verburg <[email protected]>
|
@andrew-m-leonard @sxa thoughts please? |
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.
Noting that I have managed to build the 8u482-b02 version with the original build scripts (not from the temurin tags) without any problem so it's not necessarily clear (unless I made a mistake in my build) what error I should expect.
Noting also that if we change the version of freetype in the build the SBOM validation script will also need to be updated to match what gets produced.
|
fyi, jdk8u482-b02_adopt will be appearing shortly once i've resolved : adoptium/mirror-scripts#76 |
|
@sxa - Does removing/changing the "--with-freetype=/usr/local/" bit in solaris.sh fall under the scope of this PR? |
Hi @sxa thats exceptionally weird, it can not work via temurin scripts, unless you are setting up freetype manually, you will always get After the fix: |
Thats it. The patch is adjusting exactly this. Tahnx @adamfarley |
Thats a lot of |
I did some reading, and some experiments. Can I deffere this to separate PR? It do not seem correct to me. I thought, that everywhere except |
|
hi! I was partially wrong. Or better confused myself. Jdk exists in the moment of those checks, so I will switch to somehting like That will simplify the whole code a lot. Howver, waht I want to do, and what made me to write that "jdk sources do nto exists in that moment" is this Without it, the freetype would be downloaded, and build, but later not used. Thats why I moved to the cryptic tag parsing. I will now elaborate on this, and will stop building and downloading (and of course using) the freetype if |
Yeah Solaris is only built on jdk8. Adoptium doesn't have a suitable compiler for building 11 which is the only other LTS version that works on Solaris from the upstream codebase. |
|
@andrew-m-leonard Are there any implications of us having EDIT: It looks like when we run through our instead of what I'm getting from which we might need to do some checking of before merging this |
|
Running checks in the following jobs (Solaris ones needed this playbook fix too) so we can see what the build output looks like on our systems with the full build Adoptium scripts:
Solaris build failed with: |
|
Also noting that the for the regular temurin builds of Most of those failed the build step (not always for the same reason!) but AIX failed the SBOM validation step: |
It should still be valid for older jdk8 (and possibly 9 and 10, although they do not build anyway), or not? Am moreover trying to understand what all I could miss.
That script do not seem to adjust freetype:( Unless
THat would be major divergence which should not be there. Thanx for noticing it. |
The logs throws 404. I would b ver interested in final arguments. The sbom failure is expected, as the build should pick up bundled 2.9. But the fact that build passed keeps me wondering where is the catch. Were there any built artifacts at all? I guess yes, but false positivum may be also the reason. I'm still very nervous from swapping the two work-flow lines. |
Can you clarify which link is giving a 404 - all the ones in my previous comment should be publicly accessible 🤔
From Linux? Yep! https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk8u/job/jdk8u-linux-aarch64-temurin/521/artifact/workspace/target/ is an example of the job with artifacts from a successful jdk8u482-b02 on Linux/aarch64 I haven't gone through it in detail but noting that we set
|
https://ci.adoptium.net/job/build-scripts/job/openjdk8-pipeline/3040/console => 404 others works
Good :)
Investigating. Thanx! |
Thanks - raised adoptium/ci-jenkins-pipelines#1327 to investigate |
Ok, the conclusion is quite interesting. When you use no switch; old jdk:configure run with --skip-freetype switch; old jdkconfigure run without any freetype, so run to defaults, which is no switch; new jdkconfigure run with --skip-freetype switch; new jdkconfigure run without any freetype, so run to defaults, which is, however where new jdk refers to anything newer then 482-b01 So although it looks scary, when summed up:
It seems, that this behavior is correct, as for jdk21 with Jdk21 without it, I can see: So - in my opinion - no action is needed for this patch, but every wrapper of build.sh (eventually ./makejdk-any-platform.sh/prepareWorkspace.sh) should drop the build-farm/platform-specific-configurations/mac.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/alpine-linux.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/solaris.sh:export BUILD_ARGS="${BUILD_ARGS} --skip-freetype --make-args SHELL=/bin/bash"
build-farm/platform-specific-configurations/aix.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/windows.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/windows.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/windows.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/windows.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
build-farm/platform-specific-configurations/linux.sh: export BUILD_ARGS="${BUILD_ARGS} --skip-freetype"
CHANGELOG.md:1. `-sf` changes to `-F`, (long form `--skip-freetype` stays the same).
makejdk-any-platform.1:.BR \-F ", " \-\-skip-freetype
README.md:-F, --skip-freetype
sbin/common/config_init.sh: "--skip-freetype" | "-F" )
cyclonedx-lib/build.xml: <arg value="--clean-git-repo --jdk-boot-dir /usr/lib/jvm/jdk-16 --configure-args --disable-warnings-as-errors --enable-ccache --enable-dtrace --target-file-name OpenJDK17-jdk_x64_linux_hotspot_17_35.tar.gz --release --clean-libs --tag jdk-17+35_adopt --skip-freetype --use-jep319-certs --create-debug-image --build-variant hotspot jdk17"/>
cyclonedx-lib/build.xml: <arg value="--clean-git-repo --jdk-boot-dir /usr/lib/jvm/jdk-16 --configure-args --disable-warnings-as-errors --enable-ccache --enable-dtrace --target-file-name OpenJDK17-jdk_x64_linux_hotspot_17_35.tar.gz --release --clean-libs --tag jdk-17+35_adopt --skip-freetype --use-jep319-certs --create-debug-image --build-variant hotspot jdk17"/>
In addition the WDYT? |
While studying above, I had noticed one more possible issue. The
When
My proposal is to terminate build asap, once Unlike various |
Since jdk8u482.b02 all maintained JDKs have in tree freetype. All jdks should use it unless there is necessary reason to do so. See adoptium#4287 (comment)
For tuning, initiated: #4320 |
See adoptium#4287 (comment) * On "old-jdk8" the docs man/readme lies - as using the --skip-freetype leads to use system freetype, the --freetype-dir is ignored (which is in ooposite to docs) * When --skip-freetype is omitted, the --freetype-dir works as expected. * On "new-jdk8" and on 11+the usage of -freetype-dir is leading to imminent configure error * with added --skip-freetype its useless as before, as system is used.
hints welcomed! #4321 |
I would tentatively agree with all of your analysis :-) I note that it looks like we use I feel we should definitely adjust the |
Thanx for the info! So I think this change should be global. But there may of course be a reasons I'm not awaere off:(
I had opened it in separate PR (#4320) , but i have no objections to includ eit here. I put it separately, because it is affecting also 11 and 17. |
|
The move to "bundled" I did back in 2023, was to support reproducible builds, so as to ensure deterministic freetype binary. |
Is there a particular reason that change wasn't done across all versions (even though they wouldn't have been reproducible)? That would have seemed the obvious thing to do which makes me nervous that there was some specific problem that was potentially going to be caused. |
From the PMC minutes discussing this back in Nov 22nd 2023: |
|
So the plan originally was to backport to earlier versions... but that was not done yet... So seems reasonable to do that now |
|
Thanx a lot for all that insight. Highly appreciated! |
3c67327 to
a0836d8
Compare
a0836d8 to
24726ac
Compare
|
hi! Did run quite a few tests around the freetype switches on both freetype bundled and external-needed jdks, and it seems it behave as it should now. Ok t go by me ;) Please, state your wish how to handle all related PRs: #4306 (comment) I do not mind to include them in this, or as separate. Being separate have afaik many benefits, but it is up to you. W can even discuss them in separate PRs, and then move to this one. Maybe even some logic (liek the checks may move elsewhere (eg to the man/reasme one, but for that I'm more against, then for) . Note, that the configuration one, probably can not pass until this one is finished. Similarly the sbom one can not pass before 2.13.3 reaches jdk8u (already under review) |
I'm OK with the multi-PR approach :-) Thanks for all your work on this. |
Hello!
There is openjdk/jdk8u-dev#709 agreed to go to jdk8, so a change in temurin-build is necessary to continue tp support future jdk8/
This is minimalist change, which is allowing to build jdk8 as temurin, but I do not belive it is correct, enough, nor final.
What is supposed to be supported in this script? Older jdk8? Jdk9+10? And so on. If none, maybe whole freetype download and build can be remove? Due to nature of tis commit, the jdk8 build in CI is most liekly going to fail before openjdk/jdk8u-dev#709 is merged and used.
Any guidance or thoughts highly appreciated