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

feat: add support for Apple Silicon chromium builds #7546

Merged
merged 17 commits into from May 5, 2022

Conversation

7rulnik
Copy link
Contributor

@7rulnik 7rulnik commented Sep 5, 2021

Google has published Chromium builds for Apple Silicon so we can fetch it now

Related to #6622

@google-cla
Copy link

google-cla bot commented Sep 5, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@7rulnik
Copy link
Contributor Author

7rulnik commented Sep 5, 2021

I got 3 tests failed, but not sure that it's related to this PR

  1) Accessibility
       roledescription:
     Error: expect(received).toEqual(expected) // deep equality

Expected: "foo"
Received: undefined
      at Context.<anonymous> (test/accessibility.spec.ts:171:50)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

  2) network
       Response.fromCache
         should work:
     Error: expect(received).toBe(expected) // Object.is equality

Expected: 304
Received: 200
      at Context.<anonymous> (test/network.spec.ts:166:56)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

  3) network
       Page.authenticate
         should not disable caching:
     Error: expect(received).toBe(expected) // Object.is equality

Expected: 304
Received: 200
      at Context.<anonymous> (test/network.spec.ts:606:56)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

if (platform === 'darwin') this._platform = 'mac';
if (platform === 'darwin' && productFromOptions === 'chrome')
this._platform = os.arch() === 'arm64' ? 'mac_arm' : 'mac';
else if (productFromOptions === 'firefox') this._platform = 'mac';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't feel right for me, but firefox ships universal build, so in downloadURLs we can't use the separate url for firefox for MacOS ARM because of string interpolation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as workaround we can pass platform as query parameter in firefox case and remove this hack

@7rulnik 7rulnik marked this pull request as ready for review September 5, 2021 18:08
@jschfflr
Copy link
Contributor

jschfflr commented Sep 9, 2021

I can confirm that the three test failures you described are related to Chromium changes that we haven't rolled into Puppeteer yet.

@jschfflr
Copy link
Contributor

The change looks good to me from a technical standpoint but we'll have to wait for the next Chromium roll to land it.
That one is currently blocked on some network related changes but I'll make sure to include this change with the next roll.
Thanks so much for your contribution!

@7rulnik
Copy link
Contributor Author

7rulnik commented Sep 10, 2021

Sure! Feel free to ping me. Thanks

@maksimr
Copy link

maksimr commented Oct 18, 2021

Any chance to merge this PR?

Related issue - #7632

@joshbranham
Copy link

Would love to see this land. As an alternative approach, would anyone be opposed to this function searching PATH for chromium? In our case brew install chromium drops it in /opt/homebrew/bin/chromium per Homebrew on Apple Silicon.

@SpawnAtis
Copy link

any progress ?

@7rulnik
Copy link
Contributor Author

7rulnik commented Dec 8, 2021

@7rulnik
Copy link
Contributor Author

7rulnik commented Dec 8, 2021

Also, as I understand, it would be nice to test against MacOS ARM? But it's currently blocked by actions/runner-images#2187

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 11, 2022

Let's wait for GitHub Actions to support it or we can we land it as an experimental opt-in feature?

@7rulnik
Copy link
Contributor Author

7rulnik commented Feb 11, 2022

@OrKoN honestly I prefer to release it as an opt-in because we don't really have any timeline for GitHub Actions release. Maybe we can download the ARM version only if env variables set? Something like PUPPETEER_CHROMIUM_MAC_ARM?

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 11, 2022

yes, I think an env var would work. Perhaps PUPPETEER_EXPERIMENTAL_CHROMIUM_MAC_ARM?

@7rulnik
Copy link
Contributor Author

7rulnik commented Feb 11, 2022

I will update PR on this weekend or next week

@7rulnik
Copy link
Contributor Author

7rulnik commented May 4, 2022

@OrKoN added some info to docs/api.md and to contributing.md Feel free to change wordings if they are not correct

@cdeutsch
Copy link
Contributor

cdeutsch commented May 4, 2022

Great to see this close to landing.

I'm guessing this won't help people running puppeteer in Docker on Apple Silicon?
garris/BackstopJS#1300

Are there Arm builds of Chromium for Debian running in Docker?

@OrKoN
Copy link
Collaborator

OrKoN commented May 5, 2022

@cdeutsch I am not aware of Arm builds for Debian that are available for download (but I might just not know)

@7rulnik thanks for the PR, it looks good to me!

@OrKoN OrKoN enabled auto-merge (squash) May 5, 2022 07:12
@OrKoN OrKoN disabled auto-merge May 5, 2022 07:12
@OrKoN OrKoN enabled auto-merge (squash) May 5, 2022 07:12
@OrKoN OrKoN merged commit baa017d into puppeteer:main May 5, 2022
@7rulnik 7rulnik deleted the chromium-arm64 branch May 5, 2022 13:26
olaven added a commit to olaven/paperpod that referenced this pull request May 12, 2022
Pupeteer does not play well with M1 and ARM in general.
While they [just released](puppeteer/puppeteer#7546) experimental
support for ARM macs, they still don't support [ARM machines for Debian/Docker](puppeteer/puppeteer#7546 (comment)).

This PR switches from puppeteer to [zombie](https://github.com/assaf/zombie). Ideally
I would like to move back, as Zombie does not appear to be actively maintained.
olaven added a commit to olaven/paperpod that referenced this pull request May 13, 2022
* Dev environment runs on M1 mac

Pupeteer does not play well with M1 and ARM in general.
While they [just released](puppeteer/puppeteer#7546) experimental
support for ARM macs, they still don't support [ARM machines for Debian/Docker](puppeteer/puppeteer#7546 (comment)).

This PR switches from puppeteer to [zombie](https://github.com/assaf/zombie). Ideally
I would like to move back, as Zombie does not appear to be actively maintained.

* Replaced puppeteer mock with zombie + removed commented out puppeteer code
@artyil
Copy link

artyil commented May 21, 2022

Was this one supposed to fix #7916 ?

@7rulnik
Copy link
Contributor Author

7rulnik commented May 22, 2022

@artyil nope. It's not related to any Linux (inside docker too) at all. Just native M1.

@artyil
Copy link

artyil commented May 22, 2022

@7rulnik the referred issue is related to m1 only - “[Bug]: Docker M1 Mac fails to open new page (Target.createTarget) #7916
Can you elaborate what am I missing there? You mean this fix completely unrelated to Docker issues, only when running on M1 (eg: executing node.js script in terminal) 🙄 ?

@7rulnik
Copy link
Contributor Author

7rulnik commented May 22, 2022

@artyil this PR is related only to M1. This #7546 issue is about docker so it's about chromium and puppeteer on Linux. Also I see this comment #7916 (comment) and looks like in this case it's about native run (without docker) too, but I'm not sure. WIll try to run example from it.

This was referenced May 30, 2022
This was referenced May 30, 2022
garris added a commit to garris/BackstopJS that referenced this pull request Jun 4, 2022
- puppeteer/puppeteer#7546 fixes chrome build issue on apple silicon machines   Included in v14.0.0
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.

None yet

8 participants