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

Update Gradle config for ARM architectures, initially MacOS. #1100

Closed
wants to merge 5 commits into from

Conversation

prbprbprb
Copy link
Collaborator

Still to do: update build docs.

Overall change:

  • Upgraded to Gradle 6.9.2
  • Added explicit architectures (CPU+OS) to build for, i.e. don't assume everything is x86 and just worry about 32bit vs 64bit.
  • Explicitly drop 32bit Windows support, we effectively had done already anyway.
  • Building on either MacOS architecture will build for both arches, but only run tests for the current one.

Builds and tests on:
MacOS x86_64
MacOS ARM
Linux x86_64
Windows x86_64

@prbprbprb prbprbprb requested a review from borisf January 12, 2023 16:15
Copy link

@borisf borisf left a comment

Choose a reason for hiding this comment

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

Thanks

I didn't review the functionality, but the build practices, LGTM

@prbprbprb
Copy link
Collaborator Author

Thanks, Boris!

I forgot to mention in the commit message that this should mostly resolve #1034 and go some way towards resolving #1051, but there's still a long way to go in modernising the Gradle config and I'll probably ping you during the week if that's oK.

@borisf
Copy link

borisf commented Jan 15, 2023 via email

@prbprbprb
Copy link
Collaborator Author

Those CI failures are real.... Not sure about the testJar one on Linux, will try and reproduce tomorrow, but the MacOS link error is because our CI config doesn't match the changes I made to compile for both x86 and ARM on MacOS. Will fix.

@prbprbprb
Copy link
Collaborator Author

Can't fix the Mac CI for this until upstream https://boringssl-review.googlesource.com/c/boringssl/+/56065 is resolved.

@prbprbprb
Copy link
Collaborator Author

Two more small commits to come:

  • Update build instructions for MacOS
  • I got the Maven native classifier wrong for ARM, so I'll fix that before merging this

Still to do: update build docs.

Overall change:
* Upgraded to Gradle 6.9.2
* Added explicit architectures (CPU+OS) to build for, i.e. don't assume
everything is x86 and just worry about 32bit vs 64bit.
* Explicitly drop 32bit Windows support, we effectively had done already anyway.
* Building on either MacOS architecture will build for both
arches, but only run tests for the current one.

Builds and tests on:
MacOS x86_64
MacOS ARM
Linux x86_64
Windows x86_64
project.configurations.testRuntimeOnly isn't resolvable, use
runtimeClasspath instead.

Repro'd on MacOS but should fix Linux build.
prbprbprb added a commit to prbprbprb/conscrypt that referenced this pull request Feb 14, 2023
Much yak shaving:
* BoringSSL HEAD requires a recent-ish cmake.
* Conscrypt was using Android Gradle plugiun 4.x which defaults to cmake 10 and crashes with later versions
* Upgrading to plugin version 7.x requires upgrading to Gradle 7.x

This is a minimum viable upgrade to Gradle 7.5 plus plugin 7.4,
i.e. just enough to build and remove most warnings. Some sub-projects
are broken, but I believe they were broken before (e.g. conscrypt-android-platform)
and there is still much scope for optimisation.

This update is orthogonal to the ARM changes in google#1100 and once
this lands I'll rebase that PR on top of it.

Once google#1100 lands we can consider Gradle 8.  :)
prbprbprb added a commit to prbprbprb/conscrypt that referenced this pull request Feb 14, 2023
Much yak shaving:
* BoringSSL HEAD requires a recent-ish cmake.
* Conscrypt was using Android Gradle plugiun 4.x which defaults to cmake 3.10 and crashes with later versions
* Upgrading to plugin version 7.x requires upgrading to Gradle 7.x

This is a minimum viable upgrade to Gradle 7.5 plus plugin 7.4,
i.e. just enough to build and remove most warnings. Some sub-projects
are broken, but I believe they were broken before (e.g. conscrypt-android-platform)
and there is still much scope for optimisation.

This update is orthogonal to the ARM changes in google#1100 and once
this lands I'll rebase that PR on top of it.

Once google#1100 lands we can consider Gradle 8.  :)
@prbprbprb prbprbprb mentioned this pull request Feb 14, 2023
@prbprbprb
Copy link
Collaborator Author

Due to overlapping changes with the Gradle version upgrade, it's non-trivial to rebase this PR against master so I have a new branch that merges the CLs from this change and ignores Gradle changes which I will upload presently.

@fvasco
Copy link

fvasco commented Feb 23, 2023

Is it possible to fix #858, too?

@prbprbprb
Copy link
Collaborator Author

Is it possible to fix #858, too?

We should definitely do that! From a release point of view the question would be what hardware baseline we support as I understand ARM is a bit more fragmented in terms of things like AES hardware support, but we should certainly add some build support ASAP and iterate from there.

@AlessandroAlinone
Copy link

Is it possible to fix #858, too?

We should definitely do that! From a release point of view the question would be what hardware baseline we support as I understand ARM is a bit more fragmented in terms of things like AES hardware support, but we should certainly add some build support ASAP and iterate from there.

It would be great to have support for AWS Graviton processors. Thanks!

@OliverKoo
Copy link

+1 we are trying to build on AWS Graviton and is running in

java.lang.UnsatisfiedLinkError: no conscrypt_openjdk_jni-linux-aarch_64 in java.library.path: [/usr/java/packages/lib, /lib, /usr/lib, /usr/lib64, /lib64]

@AlessandroAlinone
Copy link

We should definitely do that! From a release point of view the question would be what hardware baseline we support as I understand ARM is a bit more fragmented in terms of things like AES hardware support, but we should certainly add some build support ASAP and iterate from there.

Pete, do you have any news on this? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants