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: allow cypress to run on m1 macs #3088

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

CooperBills
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Bazel fails to run Cypress tests with an M1 Mac (Darwin arm64) host, as it is unable to find a toolchain that doesn't limit the host to an x86 CPU.

Issue Number: #3085

What is the new behavior?

The CPU architecture compatibility restriction is removed in this PR, so Cypress will run using the x86 build under rosetta (note: Cypress' official take is: "the way to run Cypress on Apple M1 ARM chip is by using Intel emulation with Rosetta 2, Apple's translation layer."). As of opening this PR, Cypress still does not have a native arm64 build.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Discussion/Questions:

  • Is this fine, or should we be defining a separate toolchain_config for darwin_arm64 with another appropriately named toolchain and @cypress_*external which really just pulls the x86 build again under the hood?
  • Should we add any documentation stating that rosetta is required on the host? (it's very likely that any mac developer will already have it installed)

@alexeagle
Copy link
Collaborator

defining a separate toolchain_config for darwin_arm64 with another appropriately named toolchain and @cypress_*external which really just pulls the x86 build again under the hood

I think this is probably better, since it gives us the ability to warn about rosetta, and when/if they do release M1 binaries, it's a smaller change to modify that toolchain_config than to introduce a new one at that time.

Note that I'm hoping to spin out cypress to its own repository so that it could have independent maintainers, but probably not happening before 5.x.

Thanks!

@CooperBills CooperBills requested a review from mrmeku as a code owner November 19, 2021 18:59
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Thanks!

this toolchain needs some cleanup to avoid eager-evaluation of the RHS of the select() statement. I think right now it will download for all platforms? Maybe even an extra download since you have another http_archive here now.

@CooperBills
Copy link
Contributor Author

Yeah, I'm pretty sure it will end up downloading another archive... Is that acceptable or would you like me to take a look at lazy-loading the cypress toolchains before merging this? Do you know of documentation or a good example I could look at for lazy evaluation of select statements for toolchains? (Only thing I could find was rules_rust's "proxy repositories": https://github.com/bazelbuild/rules_rust/blob/main/rust/repositories.bzl#L263-L264)

@alexeagle
Copy link
Collaborator

https://github.com/aspect-build/bazel_rules_template/blob/main/mylang/private/toolchains_repo.bzl has some docs about that.

Pretty sure @mrmeku will make a new cypress rule from that template sometime soon, and we'll port over any fixes from here. So I think this is good to merge. Thanks!

@alexeagle alexeagle merged commit c1778f4 into bazel-contrib:stable Dec 7, 2021
@CooperBills CooperBills deleted the cbills/cypress-m1 branch December 8, 2021 19:48
alexeagle pushed a commit that referenced this pull request Dec 18, 2021
* fix: allow cypress to run on m1 macs

* fix: add additional cypress toolchain for darwin_arm64

* docs: update documentation for darwin_arm64 parameters in cypress_repositories
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants