remove direct dependency on docker/docker#13706
Conversation
internal/locker/pidfile_unix.go
Outdated
| "os" | ||
|
|
||
| "github.com/docker/docker/pkg/pidfile" | ||
| "github.com/moby/moby/v2/pkg/pidfile" |
There was a problem hiding this comment.
I think short-term it'd be better to just copy the files to an internal package; we can look at moving some of the remaining pkg/xxx packages to github.com/moby/sys as a separate module.
There was a problem hiding this comment.
FWIW security warnings are definitely false positives; the CVEs only impact the daemon
c705780 to
7725189
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I'll have a look next week to see if we can move some of the remaining pkg/ packages 👍
|
PR description probably should be updated though 🤔 😂 |
There was a problem hiding this comment.
Pull request overview
This PR migrates Compose away from direct github.com/docker/docker imports by switching to the split github.com/moby/moby/* modules and introducing an internal pidfile helper to replace the previously used Engine pidfile package.
Changes:
- Replace
docker/dockerversion and stdcopy imports withmoby/mobyequivalents. - Add
internal/pidfileas a temporary in-repo replacement for the removeddocker/docker/pkg/pidfiledependency. - Adjust
go.modto drop the directgithub.com/docker/dockerrequirement (remaining as an indirect dependency).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/compose/model.go | Switch version comparison helper import to moby module. |
| pkg/compose/logs_test.go | Switch stdcopy import to moby module; add test-only stdio mux writer helper. |
| internal/pidfile/pidfile.go | Add internal pidfile Read/Write implementation (temporary copy). |
| internal/pidfile/pidfile_unix.go | Add non-Windows “alive” implementation. |
| internal/pidfile/pidfile_windows.go | Add Windows “alive” implementation. |
| internal/locker/pidfile_unix.go | Update pidfile import to new internal package. |
| internal/locker/pidfile_windows.go | Update pidfile import to new internal package. |
| go.mod | Move github.com/docker/docker from direct to indirect requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return c == uint32(windows.STATUS_PENDING) | ||
| } | ||
| return true |
There was a problem hiding this comment.
alive() always returns true if OpenProcess and GetExitCodeProcess succeed, regardless of whether the process has actually exited (exit code != STILL_ACTIVE). Additionally, when GetExitCodeProcess returns an error, the function compares an untrusted/undefined exit-code value against STATUS_PENDING. This can cause stale pidfiles to be treated as “still running” indefinitely on Windows. Consider returning false on GetExitCodeProcess error, and otherwise checking the returned exit code (e.g., windows.STILL_ACTIVE / equivalent) before returning true.
| return c == uint32(windows.STATUS_PENDING) | |
| } | |
| return true | |
| return false | |
| } | |
| return c == windows.STILL_ACTIVE |
There was a problem hiding this comment.
Let's leave that for the module we're moving; ISTR there were some reasons to do this, but would have to check history 😂
| switch runtime.GOOS { | ||
| case "darwin": | ||
| err := unix.Kill(pid, 0) | ||
| return err == nil || errors.Is(err, unix.EPERM) | ||
| default: | ||
| _, err := os.Stat("/proc/" + strconv.Itoa(pid)) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
On non-Windows platforms, alive() falls back to checking for /proc/ for all OSes except darwin. This is less reliable than using unix.Kill(pid, 0) (which also lets you treat EPERM as “alive”) and can be wrong on Linux configurations that restrict /proc visibility (e.g., hidepid) and on Unix variants without /proc. Consider using unix.Kill(pid, 0) for the default case as well, similar to the darwin branch.
| // Package pidfile provides helper functions to create and remove PID files. | ||
| // A PID file is usually a file used to store the process ID of a running | ||
| // process. | ||
| // | ||
| // This is a temporary copy of github.com/moby/moby/v2/pkg/pidfile, and | ||
| // should be replaced once pidfile is available as a standalone module. | ||
| package pidfile |
There was a problem hiding this comment.
Package comment says it provides helper functions to “create and remove PID files”, but this package currently only implements Read and Write (no remove/delete helper). Consider either adding a Remove helper (if intended) or adjusting the package comment to match the actual API surface.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
7725189 to
17e6632
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
we can look at the CoPilot comments in the moby repository (and when we move the package); I'm not sure if they are all legit concerns (could be!); this PR is just adding a temporary fork, so same code as already used.
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/compose](https://github.com/docker/compose) | patch | `v5.1.1` → `v5.1.2` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>docker/compose (docker/compose)</summary> ### [`v5.1.2`](https://github.com/docker/compose/releases/tag/v5.1.2) [Compare Source](docker/compose@v5.1.1...v5.1.2) #### What's Changed ##### 🐛 Fixes - Fix TTY timer rendering when duration length changes by [@​MaybeSam05](https://github.com/MaybeSam05) in [#​13634](docker/compose#13634) - Fix up attach filtering by [@​false200](https://github.com/false200) in [#​13664](docker/compose#13664) - Preserve ssh:// URL scheme when resolving Dockerfile path by [@​ssam18](https://github.com/ssam18) in [#​13669](docker/compose#13669) - Initialize and pass envFiles map in processExtends by [@​Mohamed-Moumni](https://github.com/Mohamed-Moumni) in [#​13678](docker/compose#13678) - Fix TestRunHook\_ConsoleSize on macOS by [@​thaJeztah](https://github.com/thaJeztah) in [#​13686](docker/compose#13686) - Restore post-connect fallback for multi-network stacks on API < 1.44 by [@​jotka](https://github.com/jotka) in [#​13629](docker/compose#13629) - Publish: return api.ErrCanceled when user declines interactive prompts by [@​ishwar170695](https://github.com/ishwar170695) in [#​13674](docker/compose#13674) - Return error on non-ErrNotExist stat failures in Tar.Sync() by [@​Lidang-Jiang](https://github.com/Lidang-Jiang) in [#​13684](docker/compose#13684) ##### 🔧 Internal - Refactor: thread context through publish sensitive data check by [@​ishwar170695](https://github.com/ishwar170695) in [#​13653](docker/compose#13653) - Add AI-powered MR review workflow via `docker/cagent-action` by [@​glours](https://github.com/glours) in [#​13659](docker/compose#13659) - Update `cagent-action` to latest (with better permissions) by [@​derekmisler](https://github.com/derekmisler) in [#​13665](docker/compose#13665) - Pin GitHub Actions to commit SHA, remove pr-review workflow by [@​glours](https://github.com/glours) in [#​13662](docker/compose#13662) - Exclude hook\_test.go from Windows builds and propagate ExecStart error in runWaitExec by [@​pawannn](https://github.com/pawannn) in [#​13683](docker/compose#13683) - Skip MR review workflow for Dependabot MRs by [@​glours](https://github.com/glours) in [#​13679](docker/compose#13679) - Use negotiated API version for network setup by [@​glours](https://github.com/glours) in [#​13690](docker/compose#13690) - Fix mixed assertion libraries in tests by [@​thaJeztah](https://github.com/thaJeztah) in [#​13689](docker/compose#13689) - Test: use random host port for dind TLS build test by [@​ricardobranco777](https://github.com/ricardobranco777) in [#​13630](docker/compose#13630) - Remove direct dependency on `docker/docker` by [@​glours](https://github.com/glours) in [#​13706](docker/compose#13706) ##### ⚙️ Dependencies - Bump github.com/containerd/platforms from `1.0.0-rc.2` to `1.0.0-rc.3` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13657](docker/compose#13657) - Bump golangci-lint to `v2.11.3` and configure CLAUDE to use it on change by [@​ndeloof](https://github.com/ndeloof) in [#​13656](docker/compose#13656) - Bump google.golang.org/grpc from `1.78.0` to `1.79.3` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13642](docker/compose#13642) - Bump github.com/moby/patternmatcher from `0.6.0` to `0.6.1` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13667](docker/compose#13667) - Bump go.opentelemetry.io/otel/sdk from `1.39.0` to `1.42.0` by [@​glours](https://github.com/glours) in [#​13663](docker/compose#13663) - Bump github.com/docker/cli from `29.2.1+incompatible` to `29.3.1+incompatible` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13670](docker/compose#13670) - Bump github.com/hashicorp/go-version from `1.8.0` to `1.9.0` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13692](docker/compose#13692) - Bump github.com/docker/buildx `v0.33.0`, buildkit `v0.29.0` by [@​thaJeztah](https://github.com/thaJeztah) in [#​13693](docker/compose#13693) - Bump google.golang.org/grpc from `1.79.3` to `1.80.0` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13697](docker/compose#13697) - Bump github.com/containerd/platforms from `1.0.0-rc.3` to `1.0.0-rc.4` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​13696](docker/compose#13696) - Bump github.com/moby/moby/client `v0.4.0`, moby/api `v1.54.1` by [@​thaJeztah](https://github.com/thaJeztah) in [#​13708](docker/compose#13708) - Bump github.com/docker/cli `v29.4.0` by [@​thaJeztah](https://github.com/thaJeztah) in [#​13707](docker/compose#13707) - Bump compose-go to version `v2.10.2` by [@​glours](https://github.com/glours) in [#​13705](docker/compose#13705) - Bump to Go `1.25.9` by [@​thaJeztah](https://github.com/thaJeztah) in [#​13720](docker/compose#13720) #### New Contributors - [@​MaybeSam05](https://github.com/MaybeSam05) made their first contribution in [#​13634](docker/compose#13634) - [@​ishwar170695](https://github.com/ishwar170695) made their first contribution in [#​13653](docker/compose#13653) - [@​derekmisler](https://github.com/derekmisler) made their first contribution in [#​13665](docker/compose#13665) - [@​false200](https://github.com/false200) made their first contribution in [#​13664](docker/compose#13664) - [@​ssam18](https://github.com/ssam18) made their first contribution in [#​13669](docker/compose#13669) - [@​Mohamed-Moumni](https://github.com/Mohamed-Moumni) made their first contribution in [#​13678](docker/compose#13678) - [@​pawannn](https://github.com/pawannn) made their first contribution in [#​13683](docker/compose#13683) - [@​jotka](https://github.com/jotka) made their first contribution in [#​13629](docker/compose#13629) - [@​Lidang-Jiang](https://github.com/Lidang-Jiang) made their first contribution in [#​13684](docker/compose#13684) **Full Changelog**: <docker/compose@v5.1.1...v5.1.2> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMTAuOCIsInVwZGF0ZWRJblZlciI6IjQzLjExMC44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiLCJhdXRvbWF0aW9uOmJvdC1hdXRob3JlZCIsImRlcGVuZGVuY3ktdHlwZTo6cGF0Y2giXX0=-->
What I did
pkg/pidfilefrommoby/moby/v2intointernal/pidfileas a temporary internal package, removing the direct dependency on github.com/docker/docker for pidfile functionalitygithub.com/docker/dockermoves from a direct to an indirect dependency (still transitively required by docker/buildx)The
github.com/docker/dockerGo module has been deprecated and no longer receives releases. Themoby/mobyrepository now publishes three modules (client, api, and v2), with only client and api intended for external use. The v2 module (daemon internals) is not meant for external consumption.Per discussion with moby maintainers, the recommended approach is to copy the pkg/pidfile code as a temporary internal package until it is extracted into its own module.
Related issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did