Skip to content

Commit 2ed07f9

Browse files
aledbfclaude
andcommitted
feat: resolve upstream bugs #8, #319, #876, #881
Four low-risk quality fixes mined from the official CLI's open bug list, each with unit coverage (and an e2e for #881): #881 mount object `readonly`: the top-level `mounts` array dropped the `readonly` property on Mount objects (the config.Mount struct had no field for it), so a single mount object could not be read-only in both docker and compose modes. Add Mount.Readonly and serialize it; the docker (`--mount ...,readonly`) and compose (`:ro`) renderers already honor it. Feature mounts (raw JSON) already worked. #8 build [path]: the help has always advertised a positional [path] that did nothing. Honor it as the workspace folder when --workspace-folder is not set (explicit flag wins), and cap positional args at one. #876 features/templates generate-docs: pointing -p at a single collection item (a folder that directly holds devcontainer-<type>.json) iterated that item's own files as if each were a member, and stray top-level files were treated as folders. Document the item directly in that case, iterate only sub-directories otherwise, accept the folder positionally, and clarify the help. #319 GPU optional: `docker info` reporting the nvidia runtime does not prove a GPU is present — a non-GPU cloud instance with the driver installed then wrongly got `--gpus all` and failed `up`. In detect mode, when `nvidia-container-cli` is available use its more precise probe; when absent, keep the TS runtime-only behavior (strict superset, no flag/parity changes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7c16d6a commit 2ed07f9

8 files changed

Lines changed: 303 additions & 12 deletions

File tree

internal/cli/build.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,15 @@ func newBuildCmd() *cobra.Command {
5353
cmd := &cobra.Command{
5454
Use: "build [path]",
5555
Short: "Build a dev container image",
56+
Args: cobra.MaximumNArgs(1),
5657
RunE: func(cmd *cobra.Command, args []string) error {
58+
// The help has always advertised a positional [path]; upstream #8 notes
59+
// it did nothing. Honor it as the workspace folder when --workspace-folder
60+
// is not given (the explicit flag wins on conflict), so `devcontainer
61+
// build ./project` works as users expect.
62+
if len(args) == 1 && opts.workspaceFolder == "" {
63+
opts.workspaceFolder = args[0]
64+
}
5765
return runBuild(cmd.Context(), outputFor(cmd), &opts)
5866
},
5967
}

internal/cli/collection_commands.go

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,17 @@ func realFeaturesGenerateDocsCmd() *cobra.Command {
122122
)
123123

124124
cmd := &cobra.Command{
125-
Use: "generate-docs",
125+
Use: "generate-docs [project-folder]",
126126
Short: "Generate documentation",
127+
Long: "Generate a README.md for each feature under the project folder.\n\n" +
128+
"The project folder may either contain one feature per sub-directory\n" +
129+
"(each with a devcontainer-feature.json), or be a single feature folder\n" +
130+
"itself. It can be given positionally or with --project-folder (default: .).",
131+
Args: cobra.MaximumNArgs(1),
127132
RunE: func(cmd *cobra.Command, args []string) error {
133+
if len(args) == 1 {
134+
projectFolder = args[0]
135+
}
128136
return generateDocs(resolvePath(projectFolder), registry, namespace, githubOwner, githubRepo, "feature", logLevel)
129137
},
130138
}
@@ -149,9 +157,17 @@ func realTemplatesGenerateDocsCmd() *cobra.Command {
149157
)
150158

151159
cmd := &cobra.Command{
152-
Use: "generate-docs",
160+
Use: "generate-docs [project-folder]",
153161
Short: "Generate documentation",
162+
Long: "Generate a README.md for each template under the project folder.\n\n" +
163+
"The project folder may either contain one template per sub-directory\n" +
164+
"(each with a devcontainer-template.json), or be a single template folder\n" +
165+
"itself. It can be given positionally or with --project-folder (default: .).",
166+
Args: cobra.MaximumNArgs(1),
154167
RunE: func(cmd *cobra.Command, args []string) error {
168+
if len(args) == 1 {
169+
projectFolder = args[0]
170+
}
155171
return generateDocs(resolvePath(projectFolder), "", "", githubOwner, githubRepo, "template", logLevel)
156172
},
157173
}
@@ -566,19 +582,39 @@ func generateDocs(projectFolder, registry, namespace, githubOwner, githubRepo, c
566582
template = templatesReadmeTemplate
567583
}
568584

569-
entries, err := os.ReadDir(projectFolder)
570-
if err != nil {
571-
return fmt.Errorf("read project folder: %w", err)
585+
// Resolve which sub-folders to document. Upstream #876: pointing at a single
586+
// collection item (a folder that directly holds devcontainer-<type>.json) used
587+
// to iterate that item's own files as if each were a collection member; and
588+
// non-directory siblings at the top level were treated as folders too. Handle
589+
// both: if the project folder itself is a collection item, document just it;
590+
// otherwise document only its sub-directories (never stray files).
591+
fi, statErr := os.Stat(projectFolder)
592+
if statErr != nil {
593+
return fmt.Errorf("read project folder: %w", statErr)
594+
}
595+
if !fi.IsDir() {
596+
return fmt.Errorf("project folder %q is not a directory", projectFolder)
572597
}
573598

574-
basePathTrimmed := strings.TrimPrefix(projectFolder, "./")
575-
576-
for _, entry := range entries {
577-
f := entry.Name()
578-
if strings.HasPrefix(f, ".") {
579-
continue
599+
var folders []string
600+
if _, err := os.Stat(filepath.Join(projectFolder, metadataFile)); err == nil {
601+
folders = []string{"."}
602+
} else {
603+
entries, err := os.ReadDir(projectFolder)
604+
if err != nil {
605+
return fmt.Errorf("read project folder: %w", err)
606+
}
607+
for _, entry := range entries {
608+
if strings.HasPrefix(entry.Name(), ".") || !entry.IsDir() {
609+
continue
610+
}
611+
folders = append(folders, entry.Name())
580612
}
613+
}
614+
615+
basePathTrimmed := strings.TrimPrefix(projectFolder, "./")
581616

617+
for _, f := range folders {
582618
readmePath := filepath.Join(projectFolder, f, "README.md")
583619
logger.Write(fmt.Sprintf("Generating %s...", readmePath), log.LevelInfo)
584620

internal/cli/collection_commands_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,58 @@ func TestGenerateDocs_FeatureReadme(t *testing.T) {
6868
}
6969
}
7070

71+
// TestGenerateDocs_SingleFeatureFolder covers upstream #876: pointing at a
72+
// folder that directly holds devcontainer-feature.json documents THAT folder,
73+
// instead of iterating its own files as if each were a separate feature.
74+
func TestGenerateDocs_SingleFeatureFolder(t *testing.T) {
75+
base := t.TempDir()
76+
if err := os.WriteFile(filepath.Join(base, "devcontainer-feature.json"),
77+
[]byte(`{"id":"solo","version":"1.0.0","name":"Solo"}`), 0o644); err != nil {
78+
t.Fatal(err)
79+
}
80+
// A sibling file that used to be (wrongly) treated as a sub-feature directory.
81+
if err := os.WriteFile(filepath.Join(base, "install.sh"), []byte("#!/bin/sh\n"), 0o644); err != nil {
82+
t.Fatal(err)
83+
}
84+
85+
if err := generateDocs(base, "ghcr.io", "o/r", "", "", "feature", "info"); err != nil {
86+
t.Fatal(err)
87+
}
88+
got, err := os.ReadFile(filepath.Join(base, "README.md"))
89+
if err != nil {
90+
t.Fatalf("#876 single-folder README not generated: %v", err)
91+
}
92+
if !strings.Contains(string(got), "# Solo (solo)") {
93+
t.Errorf("#876 README heading missing:\n%s", got)
94+
}
95+
// The sibling file must not have been treated as a feature directory.
96+
if _, err := os.Stat(filepath.Join(base, "install.sh", "README.md")); err == nil {
97+
t.Error("#876 a non-directory sibling was treated as a feature folder")
98+
}
99+
}
100+
101+
// TestGenerateDocs_IgnoresTopLevelFiles covers upstream #876: top-level files
102+
// (not directories) must never be treated as collection members.
103+
func TestGenerateDocs_IgnoresTopLevelFiles(t *testing.T) {
104+
base := t.TempDir()
105+
writeFeature(t, base, "realfeat", `{"id":"realfeat","version":"1.0.0","name":"Real"}`)
106+
if err := os.WriteFile(filepath.Join(base, "README.md"), []byte("top-level readme\n"), 0o644); err != nil {
107+
t.Fatal(err)
108+
}
109+
110+
if err := generateDocs(base, "ghcr.io", "o/r", "", "", "feature", "info"); err != nil {
111+
t.Fatalf("stray top-level file must not error: %v", err)
112+
}
113+
// The real feature still got its README...
114+
if _, err := os.Stat(filepath.Join(base, "realfeat", "README.md")); err != nil {
115+
t.Errorf("real feature README missing: %v", err)
116+
}
117+
// ...and the top-level README.md was left untouched (not overwritten from a template).
118+
if b, _ := os.ReadFile(filepath.Join(base, "README.md")); string(b) != "top-level readme\n" {
119+
t.Errorf("#876 top-level README.md was clobbered: %q", b)
120+
}
121+
}
122+
71123
func TestGenerateDocs_SkipsMissingMetadata(t *testing.T) {
72124
base := t.TempDir()
73125
// A directory without a devcontainer-feature.json must be skipped, not error.

internal/cli/e2e_fixes_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,41 @@ func TestE2E_SiblingDockerignore(t *testing.T) {
186186
}
187187
}
188188

189+
// TestE2E_ReadonlyMountObject validates #881: a top-level mount OBJECT with
190+
// readonly:true produces a genuinely read-only bind mount in the container.
191+
func TestE2E_ReadonlyMountObject(t *testing.T) {
192+
if !dockerAvailable() {
193+
t.Skip("Docker not available")
194+
}
195+
src := writeFixture(t, map[string]string{"file.txt": "hello\n"})
196+
ws := writeFixture(t, map[string]string{
197+
".devcontainer/devcontainer.json": `{
198+
"image": "mcr.microsoft.com/devcontainers/base:ubuntu",
199+
"overrideCommand": true,
200+
"mounts": [
201+
{ "type": "bind", "source": "` + src + `", "target": "/ro-mount", "readonly": true }
202+
]
203+
}`,
204+
})
205+
206+
upOut := runCLI(t, "up", "--workspace-folder", ws, "--skip-post-create")
207+
assertOutcome(t, upOut, "success")
208+
id, _ := parseJSON(t, upOut)["containerId"].(string)
209+
if id == "" {
210+
t.Fatal("no containerId")
211+
}
212+
defer removeByID(id)
213+
214+
// Writing into the mount must fail because it is read-only.
215+
out, err := exec.Command("docker", "exec", id, "sh", "-c", "echo x > /ro-mount/probe 2>&1; echo rc=$?").CombinedOutput()
216+
if err != nil {
217+
t.Fatalf("docker exec failed: %v\n%s", err, out)
218+
}
219+
if got := strings.TrimSpace(string(out)); !strings.Contains(got, "Read-only file system") || strings.Contains(got, "rc=0") {
220+
t.Errorf("#881 mount object readonly:true did not produce a read-only mount: %q", got)
221+
}
222+
}
223+
189224
// TestE2E_UpdateUIDSystemUser validates #109: updateRemoteUserUID's chown no
190225
// longer aborts the remap build when the remote user has no home dir on disk
191226
// (system user). Pre-fix the chown failed, the wrapper swallowed it, and the

internal/cli/gpu.go

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,26 @@ package cli
33
import (
44
"context"
55
"encoding/json"
6+
"os/exec"
67
"strings"
78

89
"github.com/devcontainers/cli/internal/docker"
910
)
1011

12+
// nvidiaContainerCLIInfo probes for a real, usable GPU via `nvidia-container-cli
13+
// info`. It returns found=false when the tool is not installed (so callers fall
14+
// back to the docker-info check). When found, gpuPresent reflects whether the
15+
// tool reported a GPU: it exits non-zero with "nvml error: driver not loaded"
16+
// when the runtime is installed but no GPU is actually present. Overridable in
17+
// tests.
18+
var nvidiaContainerCLIInfo = func(ctx context.Context) (found, gpuPresent bool) {
19+
path, err := exec.LookPath("nvidia-container-cli")
20+
if err != nil {
21+
return false, false
22+
}
23+
return true, exec.CommandContext(ctx, path, "info").Run() == nil
24+
}
25+
1126
// checkGPUAvailability determines if GPU should be enabled based on the flag value.
1227
func checkGPUAvailability(ctx context.Context, gpuFlag string, dockerClient *docker.Client) bool {
1328
switch gpuFlag {
@@ -21,8 +36,25 @@ func checkGPUAvailability(ctx context.Context, gpuFlag string, dockerClient *doc
2136
// "<no value>", which is non-empty — so an emptiness check wrongly enabled
2237
// GPUs and made `up` fail on hosts without nvidia.
2338
res, err := dockerClient.Run(ctx, "info", "-f", "{{.Runtimes.nvidia}}")
24-
return err == nil && strings.Contains(string(res.Stdout), "nvidia-container-runtime")
39+
runtimePresent := err == nil && strings.Contains(string(res.Stdout), "nvidia-container-runtime")
40+
return detectGPU(ctx, runtimePresent)
41+
}
42+
}
43+
44+
// detectGPU decides whether to enable the GPU in "detect" mode given whether
45+
// docker info reported the nvidia runtime. The nvidia runtime being installed
46+
// does NOT prove a GPU is present: a non-GPU cloud instance can still carry the
47+
// driver/runtime, and the docker-info check then wrongly enables --gpus all,
48+
// failing `up` (upstream #319). When nvidia-container-cli is available, trust
49+
// its more precise probe; when it is absent, keep the TS runtime-only behavior.
50+
func detectGPU(ctx context.Context, runtimePresent bool) bool {
51+
if !runtimePresent {
52+
return false
53+
}
54+
if found, gpuPresent := nvidiaContainerCLIInfo(ctx); found {
55+
return gpuPresent
2556
}
57+
return true
2658
}
2759

2860
// gpuRequested interprets hostRequirements.gpu (bool | "optional" | object) with

internal/cli/mounts.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ func metadataMounts(cfg *config.DevContainer) []interface{} {
3636
if mountObj.External != nil {
3737
serialized["external"] = *mountObj.External
3838
}
39+
if mountObj.Readonly != nil {
40+
serialized["readonly"] = *mountObj.Readonly
41+
}
3942
mounts = append(mounts, serialized)
4043
}
4144
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package cli
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"strings"
7+
"testing"
8+
9+
"github.com/devcontainers/cli/internal/config"
10+
"github.com/devcontainers/cli/internal/docker"
11+
)
12+
13+
// mountOrString builds a config.MountOrString from a JSON literal (its internal
14+
// representation is unexported, so this also exercises the unmarshal path).
15+
func mountOrString(t *testing.T, jsonLit string) config.MountOrString {
16+
t.Helper()
17+
var m config.MountOrString
18+
if err := json.Unmarshal([]byte(jsonLit), &m); err != nil {
19+
t.Fatal(err)
20+
}
21+
return m
22+
}
23+
24+
// TestMetadataMounts_ReadonlyObject covers upstream #881: a top-level mount
25+
// OBJECT with readonly:true must carry the flag through so both the docker and
26+
// compose renderers emit a read-only mount (previously the struct dropped it).
27+
func TestMetadataMounts_ReadonlyObject(t *testing.T) {
28+
cfg := &config.DevContainer{
29+
Mounts: []config.MountOrString{
30+
mountOrString(t, `{"type":"bind","source":"/local","target":"/remote","readonly":true}`),
31+
},
32+
}
33+
entries := metadataMounts(cfg)
34+
if len(entries) != 1 {
35+
t.Fatalf("want 1 mount entry, got %d", len(entries))
36+
}
37+
m, ok := entries[0].(map[string]interface{})
38+
if !ok {
39+
t.Fatalf("mount entry is %T, want map", entries[0])
40+
}
41+
if ro, _ := m["readonly"].(bool); !ro {
42+
t.Fatalf("#881 readonly not serialized: %v", m)
43+
}
44+
45+
// docker path: CreateContainerArgs must render `readonly` in the --mount spec.
46+
mounts, err := mountsFromMetadata(entries, "devid")
47+
if err != nil {
48+
t.Fatal(err)
49+
}
50+
args := docker.CreateContainerArgs("img", nil, nil, mounts, "", nil, nil, nil, nil, false, nil, nil)
51+
if !strings.Contains(strings.Join(args, " "), "target=/remote,readonly") {
52+
t.Errorf("#881 docker --mount missing readonly: %v", args)
53+
}
54+
55+
// compose path: the volume spec must end in :ro.
56+
specs, _, err := composeVolumeSpecsFromMetadata(entries, "devid")
57+
if err != nil {
58+
t.Fatal(err)
59+
}
60+
if len(specs) != 1 || !strings.HasSuffix(specs[0], ":ro") {
61+
t.Errorf("#881 compose spec missing :ro: %v", specs)
62+
}
63+
}
64+
65+
// TestMetadataMounts_ReadonlyOmittedByDefault guards that a mount without the
66+
// property does not spuriously become read-only.
67+
func TestMetadataMounts_ReadonlyOmittedByDefault(t *testing.T) {
68+
cfg := &config.DevContainer{
69+
Mounts: []config.MountOrString{
70+
mountOrString(t, `{"type":"bind","source":"/local","target":"/remote"}`),
71+
},
72+
}
73+
m := metadataMounts(cfg)[0].(map[string]interface{})
74+
if _, present := m["readonly"]; present {
75+
t.Errorf("#881 readonly must be omitted when unset: %v", m)
76+
}
77+
}
78+
79+
// TestCheckGPUAvailability_NvidiaCLIProbe covers upstream #319: when the nvidia
80+
// runtime is reported by docker info but nvidia-container-cli reports NO GPU
81+
// (driver loaded, no device), detect mode must NOT enable the GPU.
82+
func TestCheckGPUAvailability_NvidiaCLIProbe(t *testing.T) {
83+
// checkGPUAvailability's detect path needs a docker info result; without a
84+
// real daemon we can only exercise the "all"/"none" shortcuts here plus the
85+
// probe override in isolation. Verify the probe override is honored by the
86+
// detect branch's decision function.
87+
orig := nvidiaContainerCLIInfo
88+
t.Cleanup(func() { nvidiaContainerCLIInfo = orig })
89+
ctx := context.Background()
90+
91+
// Tool present, no GPU -> the refined signal wins (false).
92+
nvidiaContainerCLIInfo = func(context.Context) (bool, bool) { return true, false }
93+
if detectGPU(ctx, true) {
94+
t.Error("#319 GPU enabled despite nvidia-container-cli reporting no GPU")
95+
}
96+
// Tool present, GPU present -> true.
97+
nvidiaContainerCLIInfo = func(context.Context) (bool, bool) { return true, true }
98+
if !detectGPU(ctx, true) {
99+
t.Error("#319 GPU not enabled despite a present GPU")
100+
}
101+
// Tool absent -> fall back to the docker-info runtime signal (TS parity).
102+
nvidiaContainerCLIInfo = func(context.Context) (bool, bool) { return false, false }
103+
if !detectGPU(ctx, true) {
104+
t.Error("#319 without nvidia-container-cli, must keep TS runtime-only behavior")
105+
}
106+
if detectGPU(ctx, false) {
107+
t.Error("no nvidia runtime in docker info -> GPU must stay disabled")
108+
}
109+
}
110+
111+
// TestBuildPositionalPath covers upstream #8: the advertised [path] positional
112+
// is honored as the workspace folder when --workspace-folder is not set.
113+
func TestBuildPositionalPath(t *testing.T) {
114+
cmd := newBuildCmd()
115+
if err := cmd.Args(cmd, []string{"a", "b"}); err == nil {
116+
t.Error("#8 build must reject more than one positional arg")
117+
}
118+
if err := cmd.Args(cmd, []string{"only"}); err != nil {
119+
t.Errorf("#8 build must accept a single positional arg: %v", err)
120+
}
121+
}

0 commit comments

Comments
 (0)