support for building inside docker#5630
Conversation
WalkthroughAdds containerized build support: ChangesDocker Build Environment
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Dockerfile (1)
1-16: ⚖️ Poor tradeoffConsider adding a non-root USER directive for defense-in-depth.
Static analysis flagged that the container runs as root. While this is often acceptable for build-only containers that don't run services, adding a non-root user provides defense-in-depth security. However, given that this is strictly a build container and not a runtime service, this is optional.
🛡️ Example of adding a non-root user
FROM ubuntu:latest RUN apt-get update \ && apt-get install -y --no-install-recommends nodejs npm git ca-certificates python3-pip \ && rm -rf /var/lib/apt/lists/* +RUN useradd -m -u 1000 builder +USER builder + WORKDIR /workdir COPY . .Note: This may require adjusting file ownership and permissions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 1 - 16, Add a non-root user in the Dockerfile and run the build steps as that user: create a user/group and set ownership of WORKDIR (/workdir) and any copied files (after COPY . .) so subsequent RUN steps (npm ci / pip install) and the final CMD execute as the non-root account; update the Dockerfile to include the USER directive referencing that created user (and adjust file permissions via chown on /workdir) before CMD so the container no longer runs as root by default..dockerignore (1)
1-6: ⚡ Quick winConsider excluding additional build artifacts and dependencies.
The current exclusions cover development metadata, but
node_modules/andbuild_output/should also be excluded. Localnode_modules/will be reinstalled duringnpm ci(line 11 of Dockerfile), and including it in the build context unnecessarily bloats the context. Similarly,build_output/is mounted at runtime (compose.yaml line 6) and doesn't need to be in the image.📦 Proposed additions to reduce build context size
.git .github .vscode .devcontainer +node_modules/ +build_output/ *.log *.tmp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dockerignore around lines 1 - 6, Update the .dockerignore to exclude local build artifacts and dependencies by adding node_modules/ and build_output/ so the Docker build context isn't bloated; specifically, modify the .dockerignore (the file shown in the diff) to include entries for node_modules/ and build_output/ alongside the existing patterns so local modules (reinstalled via npm ci) and runtime-mounted build_output are not sent to the daemon.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compose.yaml`:
- Line 7: The compose command references the shell variable PIO_ENV but it isn't
defined; either add an environment entry for PIO_ENV on the service (so
PlatformIO gets a concrete value) or document that users must supply PIO_ENV
when invoking compose (e.g., PIO_ENV=esp32dev docker compose up); update the
compose.yaml service that contains command ["bash", "-c", "npm run build && pio
run -e $PIO_ENV"] to include an env declaration for PIO_ENV or add a comment in
the file explaining the required external variable and an example invocation.
In `@Dockerfile`:
- Line 11: The RUN instruction currently falls back from "npm ci" to "npm
install" which defeats reproducible installs; remove the "|| npm install"
fallback so the Dockerfile runs only "npm ci" (i.e., replace the RUN line that
contains "npm ci || npm install" with a single "npm ci"), and ensure CI/build
tooling provides a correct package-lock.json and Node/npm in the base image so
the build fails loudly if "npm ci" cannot run.
- Line 1: The Dockerfile currently uses an unpinned base image "FROM
ubuntu:latest"; change this to a specific, immutable image tag or digest (for
example a release tag like ubuntu:22.04 or an image@sha256:<digest>) to ensure
reproducible builds—update the FROM line in the Dockerfile to use the chosen
pinned tag or digest and document the selected Ubuntu version.
---
Nitpick comments:
In @.dockerignore:
- Around line 1-6: Update the .dockerignore to exclude local build artifacts and
dependencies by adding node_modules/ and build_output/ so the Docker build
context isn't bloated; specifically, modify the .dockerignore (the file shown in
the diff) to include entries for node_modules/ and build_output/ alongside the
existing patterns so local modules (reinstalled via npm ci) and runtime-mounted
build_output are not sent to the daemon.
In `@Dockerfile`:
- Around line 1-16: Add a non-root user in the Dockerfile and run the build
steps as that user: create a user/group and set ownership of WORKDIR (/workdir)
and any copied files (after COPY . .) so subsequent RUN steps (npm ci / pip
install) and the final CMD execute as the non-root account; update the
Dockerfile to include the USER directive referencing that created user (and
adjust file permissions via chown on /workdir) before CMD so the container no
longer runs as root by default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a077bac-8a70-4d2c-bf98-a349b1db915c
📒 Files selected for processing (3)
.dockerignoreDockerfilecompose.yaml
| @@ -0,0 +1,16 @@ | |||
| FROM ubuntu:latest | |||
There was a problem hiding this comment.
Pin the base image to a specific version.
Using ubuntu:latest leads to non-reproducible builds because the underlying image can change at any time. For a containerized build environment, reproducibility is essential.
🔒 Proposed fix to pin Ubuntu version
-FROM ubuntu:latest
+FROM ubuntu:24.04📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM ubuntu:latest | |
| FROM ubuntu:24.04 |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` at line 1, The Dockerfile currently uses an unpinned base image
"FROM ubuntu:latest"; change this to a specific, immutable image tag or digest
(for example a release tag like ubuntu:22.04 or an image@sha256:<digest>) to
ensure reproducible builds—update the FROM line in the Dockerfile to use the
chosen pinned tag or digest and document the selected Ubuntu version.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Dockerfile (1)
7-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a non-root runtime user to the image.
The container runs as root (no
USERinstruction), which weakens isolation and matches the DS-0002 finding. Add a non-root user before theCMDinstruction:Suggested fix
ENV WLED_RELEASE=True +RUN useradd --create-home --shell /bin/bash builder \ + && chown -R builder:builder /workdir +USER builder + CMD ["bash"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 7 - 16, Add a non-root runtime user to the Dockerfile to avoid running containers as root: create a user and group (e.g., myuser) and home dir after files are copied, chown /workdir (the WORKDIR used) to that user, and add a USER instruction before CMD so subsequent runtime processes (started by CMD ["bash"]) run as the non-root user; ensure any build-time steps that need root still run earlier (e.g., RUN npm ci and RUN pip install) and that file ownership for the workspace is transferred to the new user.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Dockerfile`:
- Around line 7-16: Add a non-root runtime user to the Dockerfile to avoid
running containers as root: create a user and group (e.g., myuser) and home dir
after files are copied, chown /workdir (the WORKDIR used) to that user, and add
a USER instruction before CMD so subsequent runtime processes (started by CMD
["bash"]) run as the non-root user; ensure any build-time steps that need root
still run earlier (e.g., RUN npm ci and RUN pip install) and that file ownership
for the workspace is transferred to the new user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 769c5d94-c68a-4f38-bf59-9fd9bb6d9c06
📒 Files selected for processing (3)
.dockerignoreDockerfilecompose.yaml
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
🚧 Files skipped from review as they are similar to previous changes (1)
- compose.yaml
| RUN npm ci | ||
| RUN pip install --break-system-packages --ignore-installed -r requirements.txt | ||
|
|
||
| ENV WLED_RELEASE=True |
There was a problem hiding this comment.
what is WLED_RELEASE needed for?
Its not checked by platformio or pio-scripts, neither referenced anywhere in our nightly CI jobs. SO maybe just remove it, to avoid future problems.
There was a problem hiding this comment.
Will remove it then. This was suggested in the intially drafted Dockerfile in the Discord conversation, I just copypasted it.
| *.log | ||
| *.tmp | ||
| node_modules/ | ||
| build_output/ |
There was a problem hiding this comment.
does this mean docker will ignore any firmware.bin files created by pio?
There was a problem hiding this comment.
Well, they will not be copied inside the image (during the image build process).
But they will happen to be available (accidentally) during the actual firmware build phase, because the compose.yaml file bind-mounts the local build_output into the container (because we want to be able to retrieve those .bin files afterwards).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Dockerfile (1)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a non-root runtime user before the final
CMD.The Dockerfile currently lacks a USER directive, causing the container to run as root. This creates a problem with the bind-mounted
build_outputdirectory in compose.yaml: root-owned files will be written to the host filesystem.Add the following before line 14 (
CMD ["bash"]):Proposed fix
RUN npm ci RUN pip install --break-system-packages --ignore-installed -r requirements.txt + +RUN useradd --create-home --uid 1000 builder \ + && chown -R builder:builder /workdir +USER builder CMD ["bash"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 7, Container currently runs as root because there is no USER directive; create a non-root runtime user (e.g., "appuser"), ensure ownership of the working directory set by WORKDIR /workdir is transferred to that user (chown), and add USER appuser immediately before the final CMD ["bash"] so the process runs without root privileges; reference WORKDIR /workdir and CMD ["bash"] to locate where to add the CREATE USER/CHOWN/USER steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 3-5: The RUN layer currently installs python3-pip and may
encourage use of pip flags like --break-system-packages; instead create and use
an isolated virtualenv and install Python deps from there: in the Dockerfile
replace direct pip usage with steps to install python3-venv (if needed), run
python3 -m venv /opt/venv, upgrade pip via /opt/venv/bin/pip, install your
requirements via /opt/venv/bin/pip install -r requirements.txt (without
--break-system-packages or --ignore-installed), and update PATH (e.g. ENV
PATH="/opt/venv/bin:$PATH") so the rest of the image uses the venv-managed
Python; remove any global pip flags and avoid altering system packages.
---
Duplicate comments:
In `@Dockerfile`:
- Line 7: Container currently runs as root because there is no USER directive;
create a non-root runtime user (e.g., "appuser"), ensure ownership of the
working directory set by WORKDIR /workdir is transferred to that user (chown),
and add USER appuser immediately before the final CMD ["bash"] so the process
runs without root privileges; reference WORKDIR /workdir and CMD ["bash"] to
locate where to add the CREATE USER/CHOWN/USER steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| RUN apt-get update \ | ||
| && apt-get install -y --no-install-recommends nodejs npm git ca-certificates python3-pip \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Dockerfile uses venv-based Python deps and no system-break flags.
rg -n 'break-system-packages|ignore-installed|python3-venv|python3 -m venv|ENV PATH="/opt/venv/bin:\$PATH"' DockerfileRepository: wled/WLED
Length of output: 135
Avoid system-level pip override flags; install Python deps in a venv instead.
--break-system-packages and --ignore-installed reduce determinism and can hide dependency conflicts. Use an explicit virtual environment in the image.
Proposed fix
RUN apt-get update \
- && apt-get install -y --no-install-recommends nodejs npm git ca-certificates python3-pip \
+ && apt-get install -y --no-install-recommends nodejs npm git ca-certificates python3-pip python3-venv \
&& rm -rf /var/lib/apt/lists/*
@@
RUN npm ci
-RUN pip install --break-system-packages --ignore-installed -r requirements.txt
+RUN python3 -m venv /opt/venv \
+ && /opt/venv/bin/pip install --no-cache-dir -r requirements.txt
+ENV PATH="/opt/venv/bin:$PATH"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 3 - 5, The RUN layer currently installs python3-pip
and may encourage use of pip flags like --break-system-packages; instead create
and use an isolated virtualenv and install Python deps from there: in the
Dockerfile replace direct pip usage with steps to install python3-venv (if
needed), run python3 -m venv /opt/venv, upgrade pip via /opt/venv/bin/pip,
install your requirements via /opt/venv/bin/pip install -r requirements.txt
(without --break-system-packages or --ignore-installed), and update PATH (e.g.
ENV PATH="/opt/venv/bin:$PATH") so the rest of the image uses the venv-managed
Python; remove any global pip flags and avoid altering system packages.
|
Why do we need a second container definition? There's already a Dockerfile in the |
|
This is redundant duplication, we already have a devcontainer |
|
Uhm... may I ask for some explanation here? The Dockerfile in question is basically empty, not suitable for building and/or running via the Perhaps let me re-iterate the motivation for the PR: in my opinion, it is useful to have a way to build the project (the binary firmware file) without having to clutter the system with necessary prerequisities -- namely NodeJS, all npm dependencies, Python, PlatformIO and more. Docker is an ideal way to do so. So I would expect a straightforward way to use Docker in order to build the firmware; my Dockerfile (+compose.yaml) achieves that goal. The currently existing one does not. Also, the |
|
The devcontainer is a vscode extension, but I should give everything needed to run all of your pio setup without the need for any extra tooling. Please try your compose with the existing Dockerfile to confirm. Maybe VSCode calls something you bootstrap the container? |
Trying to build & run the Apparently, this is just a plain image with a Python REPL interpreter. This corresponds to the fact that the Dockerfile is mostly empty. Also, it is somewhat suspicious that the image itself comes from Microsoft -- I guess that this particular file is strongly bound to some particlar VS Code extension (a piece of software that I do not use). All in all, I do not think that this Dockerfile is sufficient/suitable for independent firmware building. Perhaps someone with a deeper knowledge of that extension can explain how is this supposed to work? |
|
Yeah it looks like vscode must be doing some bootstrapping to install the dependencies automatically. We should still try and standardise on a single Dockerfile as it's the same requirement - an environment to do your builds in I think putting a docker-compose.yml into the root of the project will confuse more users than it would help The approach of baking the source into the build image it also flawed, adding .gitignore doesn't resolve the issue - the source should be mounted into the builder image |
I have no problem with moving that file to other location.
Okay then -- what do you propose should be in the image? We can only copy the definition of dependencies (e.g. I am not sure if this is a suitable approach, though: bind-mounting the source makes the image "always up to date", but changing a dependency breaks the build completely (as the container will attempt to build the mounted source with out-of-the-date dependencies). |
|
Giving this some more thought: apparently for the purpose of "building a firmware binary in an isolated environment", the difference between a build phase and a runtime phase is very small. To summarize our options, we can:
From the point of view of a "one-shot build", I do not see any differences between these approaches. But if we consider the lifecycle of a container (data created at runtime are stored in a temporary writable FS layer that overlays the image), IMHO it is useful to think of the container as of something easily disposable. And for repeated builds (e. g. for multiple targets), we probably do not want to install the deps every time we build. So from my point of view, the approach 3 makes the most sense (as it allows persistent caching of build dependencies -- something that does not change often when compared to the source code itself). |
Yes, it's an attempt to create a standard place for containerized software development with a composable featureset of tools. The tooling is homed here: https://containers.dev/ -- while it was created to smooth the experience for VSCode, using VSCode is not required for using the devcontainer standard. The
That's also correct - "image": "mcr.microsoft.com/devcontainers/python:3",
"features": {
"ghcr.io/ar90n/devcontainer-features/platformio:1": {}
}(Note that the PlatformIO VSCode extension will pretty much never use a pre-installed version -- it always wants to install its own captive version. The option to enable using an external PlatformIO install seems generally broken.) Same could be done for our npm install I think. To sum up: I think it's definitely true that our |
I think that we are in full agreement: the ability to build binary firmware in an isolated environment is the goal; individual tools that enable it are not that important. I am okay with using the |
|
I tried reading more about the whole devcontainer concept and honestly I am really puzzled about it. The official website does not seem to answer the most basic questions -- why is this abstraction necessary, why they decided not to use regular Dockerfiles, why they decided not to use (and/or extend) |
The key limitation with pure Docker tooling is composability -- you can't trivially mix+match layers to construct a final environment. The devcontainer tooling brings a registry of "features" -- essentially, publicly maintained script snippets to install a certain tool -- that can be stacked arbitrarily to compose the final image. It makes it easy to have this project include (nodejs + platformio), that project (nodejs + aws), the next one (aws + conda), and so forth. And best of all, I don't have to maintain copies of those snippets in every one of my projects; they're implemented as references to public repositories. Essentially the devcontainer spec is a way of constructing a unique project-specific dockerfile from publicly available fragments that do not have a rigid layer ordering. I sketched a more-complete devcontainer at https://github.com/willmmiles/WLED/tree/devcontainer-completeness -- you're welcome to give it a try. |
Well, I tried -- it did something: What exactly is the next step? I tried |
Hm - that's pretty much exactly what I did? wmiles@Alcyone:~/Projects/WLED4$ git checkout devcontainer-completeness
branch 'devcontainer-completeness' set up to track 'origin/devcontainer-completeness'.
Switched to a new branch 'devcontainer-completeness'
wmiles@Alcyone:~/Projects/WLED4$ devcontainer up
[1 ms] @devcontainers/cli 0.87.0. Node.js v22.18.0. linux 6.6.87.2-microsoft-standard-WSL2 x64.
[865 ms] Resolving Feature dependencies for 'ghcr.io/ar90n/devcontainer-features/platformio:1'...
[1513 ms] Resolving Feature dependencies for 'ghcr.io/devcontainers/features/node:2'...
<output snipped>
wmiles@Alcyone:~/Projects/WLED4$ devcontainer exec bash
vscode ➜ /workspaces/WLED4 (devcontainer-completeness) $ pio run -e nodemcuv2
Processing nodemcuv2 (board: nodemcuv2; platform: espressif8266@4.2.1; framework: arduino)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
<output snipped>The CLI tool doesn't seem clever enough to notice that the |
|
Sorry, my bad! I forgot that your fix lives in a branch and tried the whole stuff in The only small non-fatal problem I encountered when running Nevertheless, the goal of having a working isolated build environment has been achieved 👍 and I would welcome merging the updated devcontainer definition into the mainline WLED. Now that my PR has been obsoleted, I can at least provide a new PR with updated documentation for those devs (me included) who would benefit from the new build env. Is this something desirable? |
4 participants
Added a
Dockerfileto create an image with building infrastructure.Added a
compose.yamlto build+run that image in order to create a binary in a clean environment.Summary by CodeRabbit