Fix pnpm + docker #1385
Fix pnpm + docker #1385
Conversation
sarayourfriend
added
🟧 priority: high
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1385 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
.github/workflows/ghcr.yml
Outdated
| - name: Set PNPM_VERSION | ||
| run: | | ||
| export PNPM_VERSION=$(cat .pnpm-version) |
There was a problem hiding this comment.
I'm a bit surprised that this works. GitHub Actions docs recommend echo-ing to $GITHUB_ENV.
There was a problem hiding this comment.
Ah, I will do that!
@obulat that is probably why it failed 🙂
|
There appears to still be a build error in the CI: |
sarayourfriend
force-pushed
the
fix/pin-pnpm-dockerfile
branch
from
17291f3 to
8d06968
Compare
|
Something we might want to document is that They also discuss corepack, a Node first-party "experimental tool to help with managing versions of your package managers." that could be stable in the near future, and would theoretically replace volta for us. |
Where would you like to document this? The base README.md already mentions that volta doesn't fully support pnpm and links to the issue that the RFC is addressing. |
|
@sarayourfriend I didn't realize that, seems totally sufficient 👍 |
|
I can't build the image running "docker build" requires exactly 1 argument.
See 'docker build --help'.
Usage: docker build [OPTIONS] PATH | URL | -
Build an image from a DockerfilePerhaps the last part is for tagging? I tried this way: PNPM_VERSION=$(cat .pnpm-version) docker build --tag openverse-frontend:latest .but I got an error that Node-gyp needs Python and is not found 😟 Error output#9 20.63 .../[email protected]/node_modules/core-js postinstall: Done
#9 20.66 .../[email protected]/node_modules/deasync install: gyp info it worked if it ends with ok
#9 20.66 .../[email protected]/node_modules/deasync install: gyp info using [email protected]
#9 20.66 .../[email protected]/node_modules/deasync install: gyp info using [email protected] | linux | arm64
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python Python is not set from command line or npm configuration
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python Python is not set from environment variable PYTHON
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python checking if "python3" can be used
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python - "python3" is not in PATH or produced an error
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python checking if "python" can be used
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python - "python" is not in PATH or produced an error
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python **********************************************************
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python You need to install the latest version of Python.
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python Node-gyp should be able to find and use Python. If not,
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python you can try one of the following options:
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python - Use the switch --python="/path/to/pythonexecutable"
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python (accepted by both node-gyp and npm)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python - Set the environment variable PYTHON
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python - Set the npm configuration variable python:
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python npm config set python "/path/to/pythonexecutable"
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python For more information consult the documentation at:
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python https://github.com/nodejs/node-gyp#installation
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python **********************************************************
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! configure error
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack Error: Could not find any Python installation to use
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at PythonFinder.fail (/usr/local/lib/node_modules/pnpm/dist/node_modules/node-gyp/lib/find-python.js:330:47)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at PythonFinder.runChecks (/usr/local/lib/node_modules/pnpm/dist/node_modules/node-gyp/lib/find-python.js:159:21)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at PythonFinder.<anonymous> (/usr/local/lib/node_modules/pnpm/dist/node_modules/node-gyp/lib/find-python.js:202:16)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at PythonFinder.execFileCallback (/usr/local/lib/node_modules/pnpm/dist/node_modules/node-gyp/lib/find-python.js:294:16)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at exithandler (node:child_process:406:5)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at ChildProcess.errorhandler (node:child_process:418:5)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at ChildProcess.emit (node:events:527:28)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at Process.ChildProcess._handle.onexit (node:internal/child_process:289:12)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at onErrorNT (node:internal/child_process:478:16)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! stack at processTicksAndRejections (node:internal/process/task_queues:83:21)
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! System Linux 5.10.104-linuxkit
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/pnpm/dist/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! cwd /home/node/app/node_modules/.pnpm/[email protected]/node_modules/deasync
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! node -v v16.15.0
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! node-gyp -v v8.4.1
#9 20.70 .../[email protected]/node_modules/deasync install: gyp ERR! not ok
#9 20.70 .../[email protected]/node_modules/deasync install: Build failed
#9 20.71 Progress: resolved 2277, reused 0, downloaded 2180, added 2277, done
#9 20.71 .../[email protected]/node_modules/deasync install: Failed
#9 20.71 ELIFECYCLE Command failed with exit code 1.
------
executor failed running [/bin/sh -c pnpm fetch]: exit code: 1Aside from that, everything looks quite good in this PR. |
|
Sorry about leaving out the But also, I got the build command totally wrong 🤦 I don't know why it worked locally for me 🤔 Here is the correct command to run to pass the build arg: The PR description is updated as well. I will push an updated for the README now. |
|
I get an error about python not being found by node-gyp for sentry. Is it looking for it on my computer? |
|
Inside the docker build it would be looking for it elsewhere in the image. I have no idea why y'all get that and I don't though 🤔 It didn't happen in the CI either, it built successfully there. I ran It looks a lot like this issue and we could resolve it by installing Python or maybe making a multi-arch build or something and only including Python in the arm build? 🤔 |
There was a problem hiding this comment.
Thanks for updating the PR description and the README @sarayourfriend. The problem is indeed on macOS, I think we can omit it for now since it works in the CI, and we can do that as a separate improvement.
I tried in Debian and I got to build the image and run the app successfully, with some changes I mention here.
| # == | ||
| # application for development purposes | ||
| FROM node:16 AS dev | ||
| COPY --chmod=777 . /home/node/app |
There was a problem hiding this comment.
The --chmod=777 option requires installing BuildKit, this conflicts with the use of a non-root user, can we omit it?.
| COPY --chmod=777 . /home/node/app | |
| COPY . /home/node/app |
There was a problem hiding this comment.
How does BuildKit conflict with using a non-root user? Do you mean non-root user in the image or running rootless docker?
There was a problem hiding this comment.
I meant the option to change permissions with 777 as the value and run it with a non-root user.
There was a problem hiding this comment.
It seems like chmod can be replaced with chown without requiring BuildKit.
| USER node | ||
|
|
There was a problem hiding this comment.
With the user here it fails in my case due to "EACCES: permission denied". We can move this sentence just before the ENTRYPOINT.
| USER node |
There was a problem hiding this comment.
If we move it before the entrypoint without chmoding the copied files, how would the node user have access to the files copied into its home directory?
There was a problem hiding this comment.
Why wouldn't it have access to its own directory? I tried these changes and the app runs normally as far as I've seen. Otherwise, I think it needs to be pointed out in the Docker section of the README that BuildKit is required, it did not work for me as it is currently.
|
I meant to comment before approving but this is really close to being completed anyway :) |
There was a problem hiding this comment.
I managed to run this on my Mac only by adding RUN apk add --no-cache python3 make g++ on line 4 of the Docker file. I'm OK with it running only on Linux, we only need to document that.
Edited to add: I got this line from https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#node-gyp-alpine
|
@obulat I'll document that. Could y'all try an experiment for me and try to run the built image from ghcr locally on your macs? Running like this should do it (you may need to pull before but I think docker run does that automatically for you). As long as it runs on mac then I'm okay with it not being able to build on mac... mostly because this particular build is meant as a CI artifact anyway but being able to get someone quickly running a specific version of the frontend on any computer is ostensibly useful. We could for example add another comment to PRs with the |
|
I get a warning: But it runs fine!!! 🎉 And it starts really fast. |
|
Oh, I also wanted to say that I've been using |
|
Same as @obulat, I ran it and it works! ✨ (with the warning). What do you think of the suggested changes? @obulat @dhruvkb @zackkrida |
|
@krysal I'm going to merge this as is so that it stops failing on PRs but I will open a follow up to use chown instead of chmod. |
Fixes
Fixes https://github.com/WordPress/openverse-frontend/runs/6283206618
Description
This PR does the following:
pnpmto a specific version and updates instructions and installations ofpnpmto indicate how to use the "pinned" version. We need a custom solution because volta does not yet support pinningpnpmand we don't use volta to install it in the docker builds anyway.pnpm fetchto populate the store in the docker build frompnpm-lock.yamlinstead ofpnpm install. Details on how this works available on the documentation page for it here. I've cross referenced the API and usage with pnpm 7 and there are no differences from pnpm 6, so the "experimental" warning at the top of that documentation page can be safely ignored.nodeuser rather than root. This is a basic docker security best practice that we've been ignoring for a while. It's a simple fix here, especially without the multi-stage build, so why not.Testing Instructions
Checkout this branch and run
docker build . --build-arg PNPM_VERSION=$(cat .pnpm-version) -t openverse-frontend:latestand confirm that it builds a working image that you can run usingdocker run -it -p 127.0.0.1:8443:8443/tcp openverse-frontend:latestatlocalhost:8443.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin