Fix M1 Mac compatibiilty

Automattic / mShots

Website Thumbnail/Snapshot Service

GNU General Public License v2.0

100 stars 21 forks source link

Fix M1 Mac compatibiilty #57

Closed vishnugopal closed 2 years ago

vishnugopal commented 2 years ago

This fixes M1 Mac compatibiliity while retaining the behaviour in non-arm64 environments.

Fixes:

Upgrades sharp dependency (this is applicable to both envs)
Makes a separate Dockerfile.arm64 and docker-compose-overwrite-arm64.yaml applicable only when arch = arm64
Adds a missing npm run stop:docker to the stop npm task.

To test (perform all steps in both an M1 Mac and an Intel Mac):

npm ci
npm run start
Tail docker-compose logs -f
Go to http://localhost:8000/mshots/v1/vishnugopal.com and make sure a thumbnail comes up (after a few refrehes)

Also needs review from DevOps to make sure I haven't broken any production expectations!

vishnugopal commented 2 years ago

Ping for help reviewing! cc Francesco Bigiarini Jeroen Pfeil when you get time!

pullflow-com[bot] commented 2 years ago

From Francesco Bigiarini: @vishnugopal I'm taking a look now

vishnugopal commented 2 years ago

Check my comment regarding a possible multi-arch Dockerfile.

This definitely seems like the better way to approach this, when I first looked at multi-arch builds, I thought it was possible only with multiple Dockerfiles, but I dug in and found an example here that seems to work. I'll try adapting this to use TARGETARCH, thanks much for the review! :)

zaerl commented 2 years ago

@vishnugopal I think that it can be something like this:

RUN if [ "$TARGETARCH" = "arm64" ]; then \
  apt-get install -y chromium; \
  else \
  apt-get install -y google-chrome-unstable; \
  fi;

Or something similar. Maybe you can use an ARG where you store which chromium variant to install and override it at runtime when you are in an M1 chipset.

vishnugopal commented 2 years ago

@zaerl Can you review? I've made the changes based on your suggestions and that made the PR smaller :)

vishnugopal commented 2 years ago

Think I broke x86 somehow with the latest fixes. I'll need to get access to an x86 machine to test this there :-/

pullflow-com[bot] commented 2 years ago

From Francesco Bigiarini: I have a x86 machine if you want, I ran the tests a minute ago on my M1 without problems

zaerl commented 2 years ago

The code seems ok, I don't know why it timeout. Maybe we should just up jest timeout to 10 and try again.

vishnugopal commented 2 years ago

Francesco Bigiarini Yay! Thank you!