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

[7.x] Add support for arm64 macOS #869

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

sirdharma
Copy link
Contributor

This is a first pass at adding support for arm64 macOS.

I am opening this PR in order to start the discussion, alongside with the issue I opened earlier: #868.

On top of my head, this may raise a couple issues (but there may be more):

  • What if the arm64_m1 version of ChromeDriver is not available for the targeted Chrome version? Do we want to fail or fallback?
  • What if the user has a version of Chrome that is not an Universal app? I am unsure whether the arm64 version of ChromeDriver can start an Intel process.

I have tested this locally and it seems to work fine. The benefit is huge: running our Dusk test suite went from 7m 23s down to 3m 51s on my 13-inch M1 MacBook Pro.

Thank you!

@driesvints driesvints changed the title Add support for arm64 macOS [7.x] Add support for arm64 macOS Feb 11, 2021
@driesvints
Copy link
Member

@sirdharma perfect, thanks!

@driesvints
Copy link
Member

What if the arm64_m1 version of ChromeDriver is not available for the targeted Chrome version? Do we want to fail or fallback?

Fail

What if the user has a version of Chrome that is not an Universal app? I am unsure whether the arm64 version of ChromeDriver can start an Intel process.

Fail

@sirdharma
Copy link
Contributor Author

@driesvints Thanks for that. I'll confirm the behavior as soon as I have access to an Intel Mac.

@sirdharma
Copy link
Contributor Author

@driesvints I had all sorts of trouble installing an Intel version of Google Chrome, because of the way it enforces auto-updates on the user 🥲. I've also had my teammates (Intel Mac and Linux) try the branch and confirm that Dusk correctly installs the appropriate chromedriver, so I would say it's looking pretty safe 👍

Please let me know what you think. Thanks!

@sirdharma sirdharma marked this pull request as ready for review February 15, 2021 14:20
@driesvints driesvints marked this pull request as draft February 15, 2021 14:51
@driesvints
Copy link
Member

@sirdharma thanks for this. I'm going to put this one in draft for a bit longer until we've first merged #873

After that I'm going to have to ask you to rebase this PR with master.

@driesvints
Copy link
Member

@sirdharma that PR is merged now. Can you rebase this one?

@sirdharma
Copy link
Contributor Author

@driesvints Done.

@driesvints driesvints marked this pull request as ready for review February 15, 2021 15:53
@driesvints
Copy link
Member

Perfect. Thanks @sirdharma!

@taylorotwell taylorotwell merged commit 74a443d into laravel:master Feb 15, 2021
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.

3 participants