Conversation
|
AS far as i can say except for the comments I had, it seems alright. |
Co-authored-by: Copilot <copilot@github.com>
|
I closed all answered feedback. If anything should be discussed, please re-open. |
There was a problem hiding this comment.
Pull request overview
This PR adds a shared, lockfile-driven tool distribution mechanism so the same pinned CLI tool binaries can be used both inside the DevContainer and via Bazel (rules_multitool) outside the container.
Changes:
- Introduces shared tool lockfiles plus a stdlib-only Python installer and a unified
run_tool.shentrypoint. - Wires DevContainer features/tests to consume lockfile metadata and installs lockfile-managed tools during image build.
- Adds Bazel module +
tools/BUILD.bazelaliases to expose the same tools viabazel run //tools:<tool>and updates pre-commit to route viatools/run_tool.sh.
Reviewed changes
Copilot reviewed 27 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/versions.sh | Adds YAML-to-env loader used by devcontainer features/tests (via yq). |
| tools/tool_installer.py | Adds stdlib-only downloader/verifier/installer for lockfile-managed tools. |
| tools/run_tool.sh | Adds unified wrapper to run tools from PATH (in-container) or via Bazel (host). |
| tools/lockfiles/actionlint.lock.json | Adds pinned actionlint binaries + checksums. |
| tools/lockfiles/ruff.lock.json | Adds pinned ruff binaries + checksums. |
| tools/lockfiles/shellcheck.lock.json | Adds pinned shellcheck binaries + checksums. |
| tools/lockfiles/yamlfmt.lock.json | Adds pinned yamlfmt binaries + checksums. |
| tools/lockfiles/uv.lock.json | Adds pinned uv/uvx binaries + checksums. |
| tools/lockfiles/buildifier.lock.json | Adds pinned buildifier binaries + checksums. |
| tools/lockfiles/starpls.lock.json | Adds pinned starpls binaries + checksums. |
| tools/lockfiles/bazelisk.lock.json | Adds pinned bazelisk binaries + checksums. |
| tools/arch.svg | Adds architecture diagram used by tools documentation. |
| tools/README.md | Documents the dual-path tooling strategy and lockfile “source of truth”. |
| tools/BUILD.bazel | Exports lockfiles and adds Bazel aliases for tool targets. |
| MODULE.bazel | Adds rules_multitool hub + toolchain registration for tool targets. |
| MODULE.bazel.lock | Adds Bazel module lockfile for reproducible bzlmod resolution. |
| src/s-core-devcontainer/.devcontainer/Dockerfile | Copies tools/ into the image for feature/test consumption. |
| src/s-core-devcontainer/.devcontainer/with-proxy-vars.Dockerfile | Copies tools/ into the proxy-variant image and fixes unset-proxy.sh copy path. |
| src/s-core-devcontainer/.devcontainer/devcontainer.json | Adjusts build context to repo root to make tools/ available during build. |
| src/s-core-devcontainer/.devcontainer/s-core-local/versions.yaml | Removes lockfile-managed tool version/checksum duplication from YAML. |
| src/s-core-devcontainer/.devcontainer/s-core-local/install.sh | Switches installs of certain tools to the lockfile-based installer. |
| src/s-core-devcontainer/.devcontainer/s-core-local/tests/test_default.sh | Updates tool version assertions to read versions from lockfiles. |
| src/s-core-devcontainer/.devcontainer/bazel-feature/versions.yaml | Removes lockfile-managed tool version/checksum duplication from YAML. |
| src/s-core-devcontainer/.devcontainer/bazel-feature/install.sh | Switches Bazel-related tool installs to the lockfile-based installer and symlinks bazel. |
| src/s-core-devcontainer/.devcontainer/bazel-feature/tests/test_default.sh | Updates Bazel tool version assertions to read versions from lockfiles. |
| src/s-core-devcontainer/.devcontainer/bazel-feature/devcontainer-feature.json | Removes feature dependency and leaves feature metadata. |
| .pre-commit-config.yaml | Routes yamlfmt/shellcheck through tools/run_tool.sh (Bazel on host, PATH in container). |
| .shellcheckrc | Disables SC1091 to accommodate sourced scripts. |
| .gitignore | Ignores Bazel outputs and Python bytecode. |
| .devcontainer/post_create_command.sh | Installs tools via the new installer for the repo’s devcontainer workflow. |
| REUSE.toml | Adds REUSE annotations for generated/lock files and lockfile JSONs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I can also provide python tools (reuse) via bazel. But rules_py pulls in half the world: protobuf, java, c++, swift, kotlin. So I would avoid that. |
|
adding reuse to the "same approach". To be discussed. |
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
With the changes & the answers looks fine from my side.
@AlexanderLanin I'd like to get @lurtz go ahead as well before merging this as he is the main person here.
lurtz
left a comment
There was a problem hiding this comment.
I like the reduction of shell scripts. I am a bit afraid that you did not test if the devcontainer image is actually usable. At the moment only the presence of tools and expected versions are tested.
We should add tests building some S-CORE repos as a followup.
Consumer tests would be a good idea indeed. |
I would use specific releases of a few selected repos and test that some commands (bazel, pre-commit) run successfully there. We should of course from time to time bump then the releases. This makes at least sure that the devcontainer image does not get horribly broken. In the repos using the devcontainer ideally the CI should make of the devcontainer image as well. But that is a hard sell when there is bazel, which especially has issues with linux-sandbox on Ubuntu 24.04+. |
Readable docs: https://github.com/eclipse-score/devcontainer/blob/bazel/tools/README.md
Usage example: eclipse-score/docs-as-code#511