fix(erofs): set TMPDIR for mkfs.erofs on Windows#13008
Conversation
|
where is ‘mkfs.erofs (MinGW binary)’? |
|
@hsiangkao I think we could add some instructions. We should add some erofs snapshot/unpack tests that can run on Windows and Mac for erofs without requiring a mount. |
sorry, I meant currently erofs-utils doesn't enable windows builds offically (I never tried MinGW or Cygwin). |
|
@hsiangkao yeah, I'll collect those and get those over to the upstream to see whether can include that. We would need that before building it for this project it looks like. |
d4cd59b to
3a596f0
Compare
github-project-automation
Bot
moved this from Needs Triage
to Review In Progress
in Pull Request Review
There was a problem hiding this comment.
Pull request overview
Sets a Windows-specific environment override for mkfs.erofs so it uses a valid temp directory when creating diskbuf temporary files, preventing misleading ENOSPC failures caused by fallback to C:\tmp.
Changes:
- Add
mkfsEnv(layerPath)helper to injectTMPDIRon Windows. - Apply
cmd.Env = mkfsEnv(layerPath)for allmkfs.erofsinvocations in this package.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func mkfsEnv(layerPath string) []string { | ||
| if runtime.GOOS != "windows" { | ||
| return nil | ||
| } | ||
| env := os.Environ() | ||
| tmpdir := filepath.Dir(layerPath) | ||
| env = append(env, "TMPDIR="+tmpdir) | ||
| return env | ||
| } |
There was a problem hiding this comment.
Optional: the new Windows-only TMPDIR behavior in mkfsEnv() isn’t covered by tests. A small unit test (e.g., mount_windows_test.go with a windows build tag) that asserts mkfsEnv returns an env slice containing TMPDIR= would help prevent regressions.
mkfs.erofs uses TMPDIR for its internal diskbuf temp files. Windows does not set TMPDIR (only TEMP/TMP), so the MinGW binary falls back to "/tmp" which resolves to C:\tmp. That directory does not exist on most Windows machines. mkstemp fails, and erofs_diskbuf_init returns ENOSPC regardless of actual errno, producing a misleading "No space left on device" error even on disks with plenty of free space. Set TMPDIR to the snapshot directory (parent of the output layer file) for all mkfs.erofs invocations on Windows. This directory is managed by containerd and guaranteed to exist. On Unix, TMPDIR is left to the parent process (no change in behavior). Signed-off-by: Craig Chelnak <[email protected]>
3a596f0 to
1a6bd70
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func mkfsEnv(layerPath string) []string { | ||
| if runtime.GOOS != "windows" { | ||
| return nil | ||
| } | ||
| env := os.Environ() | ||
| tmpdir := filepath.Dir(layerPath) | ||
| env = append(env, "TMPDIR="+tmpdir) | ||
| return env |
There was a problem hiding this comment.
PR description says TMPDIR is set for all mkfs.erofs invocations on Windows, but this helper is only applied to layer-generating commands in this file. There is still a mkfs.erofs invocation in SupportGenerateFromTar (mkfs.erofs --help) that does not set Env, and there’s also a mkfs.erofs invocation elsewhere in the repo (plugins/snapshots/erofs/erofs.go generateFsMeta). Either update those call sites (with a suitable TMPDIR, or document why they’re exempt) or adjust the PR description to match the actual scope so the original Windows failure mode can’t persist on other code paths.
| args = append(args, layerPath) | ||
| cmd := exec.CommandContext(ctx, "mkfs.erofs", args...) | ||
| cmd.Stdin = r | ||
| cmd.Env = mkfsEnv(layerPath) |
There was a problem hiding this comment.
this should append to cmd.Environ() - at least I recall that has special handling for windows (different from bare os.Environ()); https://pkg.go.dev/os/exec#Cmd.Environ
| } | ||
| env := os.Environ() | ||
| tmpdir := filepath.Dir(layerPath) | ||
| env = append(env, "TMPDIR="+tmpdir) |
There was a problem hiding this comment.
Should this (also?) set SystemTemp? See moby/moby#52181 and the related ticket.
github-project-automation
Bot
moved this from Review In Progress
to Done
in Pull Request Review
Summary
On Windows,
mkfs.erofs(MinGW binary) readsTMPDIRfor its diskbuf temp files. Windows does not setTMPDIR, so mkfs.erofs falls back to/tmpwhich resolves toC:\tmpfor a native MinGW binary.C:\tmpdoesn't exist on most Windows machines —mkstempfails anderofs_diskbuf_initreturnsENOSPCregardless of actual errno, producing a misleading "No space left on device" error even on disks with plenty of free space.Fix
Set
TMPDIRto the snapshot directory (parent of the outputlayer.erofsfile) for allmkfs.erofsinvocations on Windows. On Unix, behavior is unchanged (nilenv inherits parent process).Why not
os.TempDir()?On Windows
os.TempDir()falls back toC:\Windows\TempwhenTMP/TEMP/USERPROFILEare all unset (e.g. service contexts). The snapshot dir is always valid — containerd creates it before calling the differ.Verified