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

Enable cross-compilation from Linux to Mac #13537

Merged
merged 32 commits into from
Jul 7, 2022

Conversation

mherrmann
Copy link
Collaborator

@mherrmann mherrmann commented May 30, 2022

The following commands can be used to test the new implementation:

git clone git@github.com:brave/brave-browser.git
cd brave-browser
git clone git@github.com:brave/brave-core.git src/brave

... check out a revision of b-c with the changes from this PR...

npm install
npm config set target_os=mac
npm config set target_arch=x64
npm run init
npm run build -- Static

Resolves https://github.com/brave/devops/issues/7551. Depends on brave/Sparkle#19 and https://github.com/brave/devops/pull/7615.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This change could affect Brave's auto-update functionality on macOS. Please test the scenarios 1.-4. specified under "Test Plan" in #12335. It should be enough to do this on one or two macOS versions, say the most recent one and the oldest one supported by Brave.

@mherrmann mherrmann requested review from a team and bridiver as code owners May 30, 2022 17:09
@mherrmann mherrmann self-assigned this May 30, 2022
@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from ee220ce to f647e8c Compare May 30, 2022 17:13
@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from f647e8c to 0f2b97d Compare May 31, 2022 06:18
@goodov
Copy link
Member

goodov commented May 31, 2022

What issue we currently have that will be solved by cross-compiling MacOS on Linux? Won't this arise another issues that we will have to deal with? Will Linux be able to compile arm64 binaries?

@simonhong
Copy link
Member

simonhong commented May 31, 2022

With this change, sparkle will be built separatly from b-c?

@mherrmann
Copy link
Collaborator Author

@simonhong yes. See brave/Sparkle#19.

simonhong
simonhong previously approved these changes Jun 2, 2022
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM for sparkle related changes.

@mherrmann
Copy link
Collaborator Author

mherrmann commented Jun 2, 2022

We would like to merge this. Even if it doesn't end up getting used in CI, it speeds up builds a little and can help developers (like me) whose primary OS is Linux. @goodov @bridiver can I ask for your review?

build/mac/download_hermetic_xcode.py Outdated Show resolved Hide resolved
patches/chrome-tools-build-mac-BUILD.gn.patch Outdated Show resolved Hide resolved
script/download_rust_deps.py Outdated Show resolved Hide resolved
build/commands/lib/config.js Outdated Show resolved Hide resolved
@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from ab80c3b to a1bb2ad Compare June 2, 2022 15:01
@mherrmann mherrmann added the QA/No label Jun 2, 2022
@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch 2 times, most recently from 5a4ea3b to 1d6124b Compare June 3, 2022 09:02
@goodov
Copy link
Member

goodov commented Jun 3, 2022

can you pls build release builds on this branch (x64 and arm64) to make sure everything's working fine?

@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from 1d6124b to 0be98bf Compare June 6, 2022 14:01
@mherrmann
Copy link
Collaborator Author

@goodov sure. There you go:

@goodov
Copy link
Member

goodov commented Jun 7, 2022

@goodov sure. There you go:

Thanks. Looks good. Sorry for being too picky, but Release builds don't use Goma. Can you please check that these jobs also work with USE_GOMA disabled?

@mherrmann
Copy link
Collaborator Author

mherrmann commented Jun 7, 2022

goodov
goodov previously approved these changes Jun 7, 2022
@mherrmann
Copy link
Collaborator Author

@bridiver could you also review or point me to someone else who can unlock this PR?

@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from 0be98bf to c5e05e0 Compare June 15, 2022 08:44
@mherrmann
Copy link
Collaborator Author

@brave/sec-team we are downloading two new Rust dependencies as part of Brave's build process. There are already several others which we are downloading. Do we need a security review for this?

(This was raised by @bridiver.)

Previous commits decoupled it from this repository, instead building its
binaries in CI. However, this goes against our recent push to keep all
necessary code in this repository. This commit therefore adds Sparkle
back. The implementation still pulls pre-built binaries from CI by
default. However, it now also supports building locally. This is then
used by CI to generate the pre-built binaries.
This adds the necessary ninja dependencies for:

    npm run build Static -- \
        --build_sparkle
        --target=brave/vendor/sparkle:tar_sparkle_binaries
It can be passed to `npm run build` via `--gn`.
@mherrmann mherrmann force-pushed the enable-cross-compilation-linux-to-mac branch from 8681617 to 3794fa2 Compare July 7, 2022 07:31
@mherrmann
Copy link
Collaborator Author

Just a quick update that I would merge this but there have been CI problems with Windows and I want a green build before merging. I expect to get it and merge after my PTO next week.

@mihaiplesa mihaiplesa merged commit 32ff5ab into master Jul 7, 2022
@mihaiplesa mihaiplesa deleted the enable-cross-compilation-linux-to-mac branch July 7, 2022 20:28
@github-actions github-actions bot added this to the 1.43.x - Nightly milestone Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants