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 .../core-js@3.21.1/node_modules/core-js postinstall: Done
#9 20.66 .../deasync@0.1.24/node_modules/deasync install: gyp info it worked if it ends with ok
#9 20.66 .../deasync@0.1.24/node_modules/deasync install: gyp info using node-gyp@8.4.1
#9 20.66 .../deasync@0.1.24/node_modules/deasync install: gyp info using node@16.15.0 | linux | arm64
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python Python is not set from command line or npm configuration
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python Python is not set from environment variable PYTHON
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python checking if "python3" can be used
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python - "python3" is not in PATH or produced an error
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python checking if "python" can be used
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python - "python" is not in PATH or produced an error
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python **********************************************************
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python You need to install the latest version of Python.
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python Node-gyp should be able to find and use Python. If not,
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python you can try one of the following options:
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python - Use the switch --python="/path/to/pythonexecutable"
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python (accepted by both node-gyp and npm)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python - Set the environment variable PYTHON
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python - Set the npm configuration variable python:
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python npm config set python "/path/to/pythonexecutable"
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python For more information consult the documentation at:
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python https://github.com/nodejs/node-gyp#installation
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python **********************************************************
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! find Python
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! configure error
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack Error: Could not find any Python installation to use
#9 20.70 .../deasync@0.1.24/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 .../deasync@0.1.24/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 .../deasync@0.1.24/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 .../deasync@0.1.24/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 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at exithandler (node:child_process:406:5)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at ChildProcess.errorhandler (node:child_process:418:5)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at ChildProcess.emit (node:events:527:28)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at Process.ChildProcess._handle.onexit (node:internal/child_process:289:12)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at onErrorNT (node:internal/child_process:478:16)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! stack at processTicksAndRejections (node:internal/process/task_queues:83:21)
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! System Linux 5.10.104-linuxkit
#9 20.70 .../deasync@0.1.24/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 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! cwd /home/node/app/node_modules/.pnpm/deasync@0.1.24/node_modules/deasync
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! node -v v16.15.0
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! node-gyp -v v8.4.1
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: gyp ERR! not ok
#9 20.70 .../deasync@0.1.24/node_modules/deasync install: Build failed
#9 20.71 Progress: resolved 2277, reused 0, downloaded 2180, added 2277, done
#9 20.71 .../deasync@0.1.24/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? 🤔 |
krysal
left a comment
krysal
left a comment
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 |
dhruvkb
left a comment
dhruvkb
left a comment
There was a problem hiding this comment.
I like the changes, especially the part where we drop Docker Compose usage in local development. I can't imagine any scenario where we'd want to use it.
|
@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. |
5 participants
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