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

Support for Loongson jdk build #3134

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Panxuefeng-loongson
Copy link

@Panxuefeng-loongson Panxuefeng-loongson commented Oct 26, 2022

Hi,

I'm a member of the openjdk development team from Loongson. We intend to distribute binaries of JDK via adoptium. LoongArch code was based on openjdk 8, here is repo on github:https://github.com/loongson/jdk8u. LoongArch is a risc architecture, developed by Loongson Technology. Currently, the freetype version needs to be updated, see: #3120. So this pr can't be merged yet, it's just used to ask for your opinion in advance.

We intend to use jdk8 as a trial and look forward to participating in more adoptium community work.

Thanks

@github-actions github-actions bot added documentation Issues that request updates to our documentation ghActions jenkins Issues that enhance or fix our jenkins server labels Oct 26, 2022
@karianna
Copy link
Contributor

Hi @Panxuefeng-loongson - Are you able to sign the Eclipse CLA?

@karianna
Copy link
Contributor

@Panxuefeng-loongson
Copy link
Author

@Panxuefeng-loongson the Loongsoon build itself seems to be failing: https://github.com/adoptium/temurin-build/actions/runs/3327315068/jobs/5515528475

I only tested it on my local machine, I will fix the build failure in docker.

@Panxuefeng-loongson Panxuefeng-loongson force-pushed the supportForLoongsonJdk branch 2 times, most recently from c7bc562 to 0d69b07 Compare October 27, 2022 02:56
@Panxuefeng-loongson
Copy link
Author

Hi @Panxuefeng-loongson - Are you able to sign the Eclipse CLA?

I clicked on this link: https://accounts.eclipse.org/user/register?destination=legal/eca/validation/125041 and want to register an account, an error occurred while submitting information: The answer you entered for the CAPTCHA was not correct. Could you please tell me where to sign the cla agreement?

@smlambert
Copy link
Contributor

This has some guidance for ECA signing, hopefully it can help: https://adoptium.net/docs/eca-sign-off

@Panxuefeng-loongson
Copy link
Author

Hi @Panxuefeng-loongson - Are you able to sign the Eclipse CLA?

I have signed the Eclipse CLA. Thanks for your help @karianna @smlambert

@smlambert
Copy link
Contributor

ECA validation now passes 👍

Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes, broadly looks good.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
build-farm/make-adopt-build-farm.sh Outdated Show resolved Hide resolved
build-farm/platform-specific-configurations/linux.sh Outdated Show resolved Hide resolved
configureBuild.sh Outdated Show resolved Hide resolved
sbin/build.sh Outdated Show resolved Hide resolved
sbin/build.sh Outdated Show resolved Hide resolved
@Panxuefeng-loongson
Copy link
Author

Panxuefeng-loongson commented Nov 1, 2022

@theaoqi is our team leader, you can also ask him if you have any questions. Before merge this changes, I would like to ask @theaoqi to confirm the changes again.

sbin/build.sh Outdated Show resolved Hide resolved
sbin/build.sh Outdated Show resolved Hide resolved
Co-authored-by: Martijn Verburg <martijnverburg@gmail.com>
Co-authored-by: Martijn Verburg <martijnverburg@gmail.com>
Co-authored-by: Martijn Verburg <martijnverburg@gmail.com>
@zdtsw
Copy link
Contributor

zdtsw commented Nov 8, 2022

Assume after we enable this feature (do nightly+weekly jdk8 build to support loongson), we need update ci-jenkins-pipeline as well

@Panxuefeng-loongson Panxuefeng-loongson requested review from karianna and theaoqi and removed request for karianna and theaoqi November 8, 2022 12:46
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Please note there is a separate decision whether to add build agents at Adoptium, but at least this should enable folks to build locally.

Copy link

@theaoqi theaoqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@theaoqi theaoqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the patch broke the builds? @Panxuefeng-loongson please have a look.

@Panxuefeng-loongson
Copy link
Author

Some errors have occurred, i will check it

Comment on lines 145 to 147
if [ "${ARCHITECTURE}" == "loongarch64" ]; then
echo Loongson jdk8 requires a Loongson boot JDK - downloading one ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the architecture check required? Doesn't it require a Loongson JVM to build the Loonson codebase every time?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the architecture check required? Doesn't it require a Loongson JVM to build the Loonson codebase every time?

I found that in jenkins jobs, Loongson codebase will build on x86 machine. If not architecture check, x86 machine will use the LoongArch jdk binary, and an error will occur when VARIANT is BUILD_VARIANT_LOONGSON. Maybe my ideas are wrong and welcome to give your opinion.

build-farm/platform-specific-configurations/linux.sh Outdated Show resolved Hide resolved
@Panxuefeng-loongson
Copy link
Author

Please review the change again and welcome to give your opinion @tellison @karianna

Copy link
Contributor

@andrew-m-leonard andrew-m-leonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks

@karianna
Copy link
Contributor

Couple of shellcheck warnings:

...
2022-11-26 06:18:35 [INFO]   File:[/github/workspace/sbin/build.sh]
2022-11-26 06:18:37 [ERROR]   Found errors in [shellcheck] linter!
2022-11-26 06:18:37 [ERROR]   Error code: 1. Command output:
------

In /github/workspace/sbin/build.sh line 250:
        local updateNum="$(cat "$loongsonVerFile" | awk -F - '{print $1}' | awk -F u '{print $2}')"
                               ^----------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In /github/workspace/sbin/build.sh line 251:
        local buildNum="$(cat "$loongsonVerFile" | awk -F - '{print $2}')"
                              ^----------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

For more information:
  https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
------
...

@Panxuefeng-loongson
Copy link
Author

Couple of shellcheck warnings:

...
2022-11-26 06:18:35 [INFO]   File:[/github/workspace/sbin/build.sh]
2022-11-26 06:18:37 [ERROR]   Found errors in [shellcheck] linter!
2022-11-26 06:18:37 [ERROR]   Error code: 1. Command output:
------

In /github/workspace/sbin/build.sh line 250:
        local updateNum="$(cat "$loongsonVerFile" | awk -F - '{print $1}' | awk -F u '{print $2}')"
                               ^----------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In /github/workspace/sbin/build.sh line 251:
        local buildNum="$(cat "$loongsonVerFile" | awk -F - '{print $2}')"
                              ^----------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

For more information:
  https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
------
...

I will fix this problem, thanks

@Panxuefeng-loongson Panxuefeng-loongson requested review from karianna and removed request for tellison November 28, 2022 02:57
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Panxuefeng-loongson
Copy link
Author

ping

@gdams
Copy link
Member

gdams commented Dec 22, 2022

@Panxuefeng-loongson this looks good to me, on the topic of FreeType, I propose that we merge #3188 first which would then allow us to specify a different FreeType tag for Loongson JDK. What do you think?

@Panxuefeng-loongson
Copy link
Author

@Panxuefeng-loongson this looks good to me, on the topic of FreeType, I propose that we merge #3188 first which would then allow us to specify a different FreeType tag for Loongson JDK. What do you think?

This option looks great and I agree with your solution

@gdams
Copy link
Member

gdams commented Dec 22, 2022

Out of interest, what FreeType version does Loongson JDK need @Panxuefeng-loongson?

@Panxuefeng-loongson
Copy link
Author

Out of interest, what FreeType version does Loongson JDK need @Panxuefeng-loongson?

Recent versions of freetype have met the Loongson JDK build, because config.guess and config.sub support LoongArch from this point onwards: http://git.savannah.gnu.org/gitweb/?p=config.git;a=commit;h=c8ddc8472f8efcadafc1ef53ca1d863415fddd5f, It's been two years since. I have checked the last few versions of freetype and they all meet our needs. Thanks @gdams

@andrew-m-leonard
Copy link
Contributor

this has been stale for over a year, converting to "draft", if no longer needed please close.

@andrew-m-leonard andrew-m-leonard marked this pull request as draft April 3, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues that request updates to our documentation ghActions jenkins Issues that enhance or fix our jenkins server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants