Pin the packages the images install, and keep them pinned - #19
Merged
Conversation
`pip install pytest …` carried no version specifier, so the harness was decided by the day the image was built: one tag has already shipped pytest-embedded 2.2.1 / pytest 8.4.2 and 2.8.1 / 9.1.1 across two rebuilds. The drift reached past the test harness. pytest-embedded-serial-esp 2.x requires esptool>=5.2 and nothing holds esptool back, so pip overwrote the copy ESP-IDF ships - and components/esptool_py/esptool/esptool.py is a shim around `python -m esptool`, which makes the harness decide what `idf.py flash` runs. esptool is therefore pinned explicitly, and the image runs 5.3.1 against ESP-IDF 5.5's own esptool~=4.12. That divergence is deliberate (issue #18): the alternative is a 1.x harness frozen since November 2025, while ESP-IDF 6.0's constraints already pair esptool 5.x with pytest-embedded 2.x. `python -m pip check` closes the layer, and the verification RUN now prints esptool and writes a freeze to /opt/esp/python-packages.txt - esptool being exactly what changed underneath without anyone noticing. scripts/check-pins.sh keeps it that way. It gates pip and pio, leaves apt advisory (Debian rewrites its pool at every point release; an Ubuntu release-pocket pin rolls back CVE fixes), and compares specs after ARG expansion - which is how `Unity@^${UNITY_VERSION}` turns out to be a range shipping 2.6.1 under an ARG saying 2.6.0. It runs in lint.yml rather than in `prepare`: a Dockerfile belongs to one image, so an unpinned package in one must not withhold another image's tags. platformio's three unpinned packages carry `# pin-allow:` with a reason - pinning them belongs to platformio's own change, and protobuf/jinja2 need "keep or drop" answered first. An allowance that covers nothing fails the check, so they cannot rot silently. Verified on both IDF lines (v5.5.5 and v5.4.1): the image builds, `pip check` is clean, `idf.py --version` still exits 0, and `pytest --help` still carries --embedded-services.
The gate shipped in the previous commit passed things it exists to catch and failed things it does not. Both directions are now covered by fixtures. Passed when it should not have: a `-r`/`-c` argument made the checker skip the rest of the command, so `pip install -c c.txt pytest` was green - and a constraints file bounds only the names it lists, so that pytest really did re-resolve. A requirements file is now followed into the build context and its own lines held to the same rule, a constraints file pins nothing by itself, and neither excuses the packages named beside it. A `git+…` URL counted as pinned even on `@main`; it now needs a full commit, and a plain URL a `#sha256=`. `pytest==9.*` counted as pinned, though a PEP 440 prefix match re-resolves like any range. A `RUN --mount=…` shifted the first token and turned the check off for that whole layer. Failed when it should not have: `--trusted-host pypi.org` was reported as an unpinned package called `pypi.org` (the value-taking flag list is now complete); prose merely mentioning `pin-allow:` - documenting this very mechanism - became an allowance and then failed for covering nothing (the pattern is anchored to the start of the comment); and an allowance written *below* its install covered nothing, contradicting all three places that document it as valid anywhere, so allowances are now collected in a pass of their own. `set -f` around the word split, because `pip install *` was expanding against the working directory. A bash-4 check with a real message, since the associative arrays otherwise fail as `declare: -A: invalid option` on macOS's system bash - the same floor check-versions.sh already stands on with `mapfile`. What the gate cannot see is now said rather than implied: a script an image runs installs packages of its own (esp-matter's `./install.sh`), so those lines are reported, and the closing line claims only what the Dockerfiles name. Two fixes outside the script. The esp-idf verification greps for the exact `esptool==<version>` instead of the name - `pip freeze` always prints the name, so the old form passed on any version, which is precisely the drift the layer was added to catch. And esp-matter regenerates /opt/esp/python-packages.txt rather than inheriting it: install.sh populates the same venv, so the base's snapshot described an environment that image no longer has, while the esp-idf README sends readers to that very file. Verified: 16 fixtures covering every case above, both IDF lines rebuilt, the esptool assertion confirmed to fail on 4.12.0, shellcheck clean at every level. esp-matter is not buildable locally in reasonable time - CI covers it.
There was a problem hiding this comment.
Pull request overview
This PR makes the developer Docker images reproducible by pinning package installs (especially the ESP-IDF pytest harness) and adding a repo-level lint gate to prevent future “floating” dependencies from silently changing image contents across rebuilds.
Changes:
- Pin the ESP-IDF pytest harness (and its transitive flasher dependency
esptool) to exact versions and add stronger verification (pip check, frozen package snapshot). - Add
scripts/check-pins.shand wire it into CI (🧹 Lint) and local linting to enforce pinning rules per package manager (pip/pio gated, apt advisory). - Update documentation and guidance (root README, image READMEs, CLAUDE.md, Dependabot notes) to reflect the new pinning/verification regime.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/lint.sh |
Adds local execution of the new pin-check gate alongside existing lint steps. |
scripts/check-pins.sh |
New enforcement script that validates pinned install specs in Dockerfiles with pin-allow exceptions. |
README.md |
Documents check-pins.sh and clarifies which lint checks are gates vs advisory. |
images/versions.json |
Notes that Dockerfile-level package pins are validated via check-pins.sh rather than versions.json. |
images/platformio/Dockerfile |
Adds pin-allow exceptions for currently-unpinned pip/pio installs (protobuf/jinja2/Unity). |
images/esp-matter/Dockerfile |
Regenerates the python package freeze in the derived image and surfaces esptool/pytest info in verification. |
images/esp-idf/README.md |
Documents the fully-pinned harness and explains the esptool pin rationale and where to inspect the freeze. |
images/esp-idf/Dockerfile |
Pins the full pytest harness + esptool, adds pip check, and asserts esptool exact version in the freeze. |
CLAUDE.md |
Codifies the “pin what you install; verify what you pin” guidance and the pinning rules per manager. |
.github/workflows/lint.yml |
Adds a CI gate step running ./scripts/check-pins.sh and updates lint rationale text. |
.github/dependabot.yml |
Updates notes to reflect that Dockerfile pip pins are now explicit but still not Dependabot-managed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The comment claimed the layer would catch esptool being replaced, but the grep matched the name, so any version passed - the same code-vs-comment gap this branch had just fixed in esp-idf, reintroduced one image over. The expected version is read out of the base's own snapshot before that file is overwritten, so esp-matter asserts "install.sh did not move esptool" without repeating a number the base already owns - and a base carrying no snapshot (a fork, an older esp-idf) skips the assertion rather than failing on it. Verified against three fixtures in the built esp-idf image: unchanged esptool passes, a moved one fails, a missing base snapshot skips with a message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
images/esp-idf/Dockerfileinstalled its test harness with no version specifierat all, so what the image carried was decided by the day it was built — one tag has
already shipped pytest-embedded 2.2.1 / pytest 8.4.2 and 2.8.1 / 9.1.1 across two
rebuilds. This pins all nine packages with
==and adds the check that keeps thempinned.
Closes #18, taking option A from
#18 (comment).
The pins, and why
esptoolis among themesptoolis not a test dependency and nothing here installed it directly. Itarrives through
pytest-embedded-serial-esp, whose 2.x line requiresesptool<6,>=5.2, and pip overwrote the copy ESP-IDF ships — whilecomponents/esptool_py/esptool/esptool.pyis a nine-line shim aroundpython -m esptool. So an unpinned test package decided whatidf.py flashruns.The image therefore runs esptool 5.3.1 against ESP-IDF 5.5's own
esptool~=4.12.That divergence is deliberate and documented in the Dockerfile, together with the
three ways of "fixing" it that do not work — editing the constraints file inside
the image (a cache with a one-day TTL), re-pointing
PIP_CONSTRAINTat Espressif'scopy (mutable upstream; the pytest pins were already deleted from it), and arming
IDF_PYTHON_CHECK_CONSTRAINTS(everyidf.pycall would then exit non-zero). Itresolves itself at ESP-IDF 6.0, whose constraints already pair esptool 5.x with
pytest-embedded 2.x.
python -m pip checknow closes that layer, and the verificationRUNasserts theexact
esptool==<version>rather than the name —pip freezeprints the nameunder any version, which is precisely the drift the layer exists to catch.
scripts/check-pins.shimages/versions.jsonpins what CI builds; nothing pinned what a buildinstalls. The new script gates that, per package manager:
pip==plus an exact version (==9.*is a PEP 440 prefix match, so it is a range); a VCS reference needs a full commit; a URL needs#sha256=; a-rfile is followed into the build context and held to the same rule, while a-cfile pins nothing by itselfpioname@version, never@^version— the caret is a range, which is howUnity@^${UNITY_VERSION}ships 2.6.1 under an ARG saying 2.6.0aptjq=1.7.1-3build1rolls back six CVE fixesSpecs are compared after ARG expansion, so a range cannot hide in a Dockerfile
default. Exceptions are
# pin-allow: <package> - <reason>comments that name onepackage rather than muting a rule, work above or below the install, and fail the
check when they cover nothing.
It runs in
lint.yml, not inprepare: a Dockerfile belongs to one image, so anunpinned package in one must not withhold another image's tags. The
cross-image blast radius is justified for
images/versions.jsonalone, where thedata really is shared.
What it deliberately does not claim. Packages that a script an image runs
installs for itself — esp-matter's
./install.sh --no-host-tool— are outside it,and the script says so on its own output rather than passing over them in silence.
The closing line reads "every package these Dockerfiles name is pinned".
platformio is exempt for now
Its three unpinned packages carry a
pin-allowwith the reason. Pinning thembelongs to platformio's own change:
protobufandjinja2have no consumer in theimage (neither PlatformIO core nor the
espressif32platform requires them), so"keep or drop" comes before "which version"; and dropping the caret from
Unitychanges what the image contains and wants the matching ARG bump beside it.
Also
/opt/esp/python-packages.txtinstead of inheriting it —install.shpopulates the same venv, so the base's snapshot described anenvironment that image no longer has, while the esp-idf README sends readers to
that very file.
.github/workflows/lint.yml's justification for keeping hadolint advisory wasfactually wrong on two of its three points (no apt package ever passes through a
build arg, and SC3046 is a state-tracking bug that appears after an
ENV, not amissing
SHELL). Replaced with the measured reasons..github/dependabot.ymlsaid the pip installs carry no constraints, which stopsbeing true with this change; it now says why pinning them still changes nothing
for Dependabot (its pip ecosystem reads manifests, never a
pip installline).Verified locally
./scripts/build.sh esp-idfon both IDF lines (v5.5.5 and v5.4.1): builds,pip checkclean,idf.py --versionstill exits 0,pytest --helpstill carries--embedded-services, esptool 5.3.1 in both.check-pins.shagainst 16 fixtures covering every rule and every false-positivecase above;
check-versions.sh,actionlint,shellcheck(clean at everylevel, not just
--severity=error),hadolint(DL3013 is gone from esp-idf; theremaining findings are the documented deliberate ones).
esp-matter is not buildable locally in reasonable time — CI covers it, which is the
point of it being build-validated on pull requests.