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

Fix gradle builds for Apple Silicon running arm64 JDKs #4792

Merged
merged 7 commits into from
Feb 3, 2022

Conversation

skhamis
Copy link
Contributor

@skhamis skhamis commented Jan 22, 2022

fixes #4665

To Test (Intel or M1 Mac)

  1. Delete your libs/desktop folder (should be only darwin in there)
  2. If you have any rust.targets in your local.properties update (or just comment out for now):
  • Remove any darwin targets
  • update to one of the new darwin-specific architectures
  1. Run ./libs/verify-desktop-environment.sh
  2. also run ./build-all.sh android (shouldn't need rebuild but just incase)
  3. you should now see libs/desktop/darwin-{your-arch}
  4. in the top level of the project, run ./gradlew assembleDebug
  5. using local-published flow, ensure android-components can also build successfully
  6. using local-published flow, ensure fenix can also build successfully
  • Need to confirm it still works on Intel (will need to clear out libs/desktop and rebuild NSS/SQLCipher)
  • Need to be able to successfully build android-components -> fenix with no issues

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@codecov-commenter
Copy link

Codecov Report

Merging #4792 (6c6260a) into main (d817251) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4792   +/-   ##
=======================================
  Coverage   81.53%   81.53%           
=======================================
  Files          49       49           
  Lines        5650     5650           
=======================================
  Hits         4607     4607           
  Misses       1043     1043           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d817251...6c6260a. Read the comment docs.

@skhamis skhamis requested a review from a team January 25, 2022 20:05
@tarikeshaq tarikeshaq self-requested a review January 26, 2022 07:26
@skhamis
Copy link
Contributor Author

skhamis commented Jan 26, 2022

@tarikeshaq would you mind following the instructions to test that this works on Intel?
@MatthewTighe would you mind also following the instructions to test that this works on your M1 (will need to ensure your JDK is arm64)

Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Tested it on my Intel machine! app services builds all the way across the local development flow to fenix/android components. As we debugged yesterday, I had a rust.targets in my local.properties and I need to replace darwin with darwin-x86-64.

We should probably the docs in https://github.com/mozilla/application-services/blob/main/docs/howtos/locally-published-components-in-fenix.md to reference the new darwin-x86-64 and darwin-aarch64 targets instead of darwin

@skhamis
Copy link
Contributor Author

skhamis commented Jan 28, 2022

This is blocked until:

land as any M1s hoping to build a-s and then use the locally published flow in a-c will run into issues with the kotlin library room. While the first PR is much smaller, there is issues raised on the a-c team being that it's a pre-release version and moving to the next stable release will require kotlin 1.6 (second PR).

@skhamis
Copy link
Contributor Author

skhamis commented Feb 3, 2022

While this fixes it in a-s, there will be an issue in a-c with the room library so I won't announce that M1s can use native arm64 JDKs, however given that this is still needed on our side -- I'll land it once all tests pass and let a-c chart the path to fixing it on their side based on their timetables.

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.

Add support for M1 Macbook desktop testing
3 participants