diff --git a/docs/DIVERGENCES.md b/docs/DIVERGENCES.md index 01bc17d..e69fe7c 100644 --- a/docs/DIVERGENCES.md +++ b/docs/DIVERGENCES.md @@ -170,6 +170,12 @@ upstream issue it fixes. (upstream #930). This CLI applies the labels with one more build, `FROM `, tagged with the `--image-name` values or, without them, the image's own name (`TestE2E_ComposeBuildLabel`, `internal/cli/e2e_fixes_test.go`). +- **`build --label` applies to a Dockerfile configuration.** TS passes `--label` only to + the build that extends an image configuration with Features (`updateImage`); the single + `docker build` of a Dockerfile configuration (`buildAndExtendImage`) drops it without a + word. This CLI passes each `--label` to that build, as it does for an image and for a + Docker Compose configuration, so that `--label` never goes unapplied without a word + (`TestBuildDockerfileBuildFlags`, `internal/cli/dockerfile_build_held_test.go`). - **`features generate-docs` and `templates generate-docs` document the folders they are given.** TS takes every entry of the project folder as an item folder: pointed at a single Feature or Template folder it looks for metadata under each of its files and @@ -226,7 +232,11 @@ upstream issue it fixes. runtime/E2E/release/`windows-latest`/ConPTY lane. The `platform="win32"` logic is kept solely for parity with the oracle. (arm64 runtime is validated via a non-gating, QEMU-emulated experimental job.) -- **Runtime: Docker only.** Podman is not supported (no parity guarantee or test). +- **Runtime: Docker only.** Podman is not supported (no parity guarantee or test). TS's + Podman-only behavior is not implemented, among it `--security-opt label=disable` on a + build with BuildKit build contexts when `getenforce` is not `Disabled` and `podman info` + reports SELinux enabled (`isUsingSELinuxLabels`), and `--security-opt label=disable` + and `--userns=keep-id` on `podman run`. With Docker, TS adds neither. - **Compose: v2 only** (`docker compose`). Compose v1 (`docker-compose`) is not supported. - **`exec`: inherited terminal** (`docker exec -it` inherits the controlling terminal), no self-managed PTY. The 128+N contract comes from the child process. Interactive diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 0307aa9..c0d0aea 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -3911,6 +3911,28 @@ initial_cases: class: build-runtime current_status: match + - id: build.dockerfile-build-args-order-success + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: | + D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE} + rm -rf $D && mkdir -p $D/ws/.devcontainer + printf 'FROM alpine:3\n' > $D/ws/.devcontainer/Dockerfile + printf '%s' '{"build":{"dockerfile":"Dockerfile","args":{"ZED":"1","10":"2","ALPHA":"3","2":"4"}}}' > $D/ws/.devcontainer/devcontainer.json + printf '%s\n' '#!/bin/sh' "echo \"\$*\" >> $D/docker.log" "exec $(command -v docker) \"\$@\"" > $D/docker + chmod +x $D/docker + echo WORKSPACE=$D/ws + echo DOCKER_WRAP=$D/docker + ts_cmd: "build --workspace-folder ${WORKSPACE} --docker-path ${DOCKER_WRAP} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "grep -o -E -- '--build-arg (ZED|10|ALPHA|2)=[0-9]+' /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}/docker.log" + cleanup_cmd: "docker rmi -f parity-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: build-runtime + current_status: match + notes: "The docker CLI is a wrapper that logs its arguments; verify_cmd lists the build's --build-arg values for build.args. TS passes them in Object.keys order: the integer keys ascending (2, 10), then the others as declared (ZED, ALPHA). Go used to pass them in Go map order, which changes from run to run." + - id: build.dockerfile-multi-image-success lock: feature-content-image lane: runtime diff --git a/internal/cli/compose_up.go b/internal/cli/compose_up.go index 2e77c96..82663ab 100644 --- a/internal/cli/compose_up.go +++ b/internal/cli/compose_up.go @@ -487,9 +487,7 @@ func (r *upRunner) buildAndExtendCompose(ctx context.Context, s composeStart, pe override.Context = emptyDir } override.BuildKitInlineCache = s.useBuildx - for _, k := range sortedStringKeys(info.BuildArgs) { - override.BuildArgs = append(override.BuildArgs, [2]string{k, info.BuildArgs[k]}) - } + override.BuildArgs = orderedBuildArgs(info.BuildArgs, featureBuildArgOrder(info.BuildArgs)) for _, k := range sortedStringKeys(info.BuildKitContexts) { v := info.BuildKitContexts[k] if v == "." { diff --git a/internal/cli/compose_up_held_test.go b/internal/cli/compose_up_held_test.go index 9619468..404a488 100644 --- a/internal/cli/compose_up_held_test.go +++ b/internal/cli/compose_up_held_test.go @@ -886,7 +886,7 @@ func TestBuildAndExtendComposeImageServiceWithFeatures(t *testing.T) { for _, want := range []string{ "\n image: " + folderImageName(h.ws) + "\n", "\n context: " + emptyDir + "\n", - "\n - _DEV_CONTAINERS_IMAGE_USER=vscode\n", + "\n - _DEV_CONTAINERS_IMAGE_USER=vscode\n - _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp\n", } { if !strings.Contains(override, want) { t.Errorf("override lacks %q:\n%s", want, override) diff --git a/internal/cli/dockerfile_build.go b/internal/cli/dockerfile_build.go index ff15fa2..ce40bab 100644 --- a/internal/cli/dockerfile_build.go +++ b/internal/cli/dockerfile_build.go @@ -9,6 +9,7 @@ import ( "github.com/devcontainers/cli/internal/config" "github.com/devcontainers/cli/internal/docker" + "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" ) @@ -156,7 +157,7 @@ func buildDockerfileImage(ctx context.Context, logger log.Logger, dockerClient * ContextPath: dockerContextPath(cfg), Tags: imageNames, Target: info.OverrideTarget, - BuildArgs: configArgs, + BuildArgs: orderedBuildArgs(configArgs, features.PropertyOrder(configArgs, cfg.BuildArgsOrder)), CacheFrom: cacheFrom, Labels: fb.Labels, NoCache: fb.NoCache, @@ -182,6 +183,14 @@ func buildDockerfileImage(ctx context.Context, logger log.Logger, dockerClient * return nil } +func orderedBuildArgs(args map[string]string, keys []string) [][2]string { + out := make([][2]string, 0, len(keys)) + for _, k := range keys { + out = append(out, [2]string{k, args[k]}) + } + return out +} + func featureBuildArgOrder(args map[string]string) []string { var keys []string for _, k := range []string{"_DEV_CONTAINERS_BASE_IMAGE", "_DEV_CONTAINERS_IMAGE_USER", "_DEV_CONTAINERS_FEATURE_CONTENT_SOURCE"} { diff --git a/internal/cli/dockerfile_build_held_test.go b/internal/cli/dockerfile_build_held_test.go index dbba53a..ef46acd 100644 --- a/internal/cli/dockerfile_build_held_test.go +++ b/internal/cli/dockerfile_build_held_test.go @@ -341,6 +341,28 @@ func TestBuildDockerfileWritesTheReferenceDockerfile(t *testing.T) { }) } +func TestBuildDockerfileBuildArgOrder(t *testing.T) { + res := dockerfileBuildRun{ + files: map[string]string{".devcontainer/Dockerfile": "FROM alpine AS dev\n"}, + config: config.DevContainer{ + Build: &config.Build{Dockerfile: "Dockerfile", Args: map[string]string{"MID": "5", "ALPHA": "4", "ZED": "3", "10": "2", "2": "1"}}, + BuildArgsOrder: []string{"ZED", "10", "ALPHA", "2", "MID", "ZED"}, + }, + useBuildx: true, + }.run(t) + if res.err != nil { + t.Fatal(res.err) + } + wantArgs := dockerCall{"buildx", "build", "--load", "--build-arg", "BUILDKIT_INLINE_CACHE=1", + "-f", res.path, "-t", "img", "--target", "dev_containers_target_stage", + "--build-arg", "2=1", "--build-arg", "10=2", "--build-arg", "ZED=3", "--build-arg", "ALPHA=4", "--build-arg", "MID=5", + "--build-arg", "_DEV_CONTAINERS_BASE_IMAGE=dev", + filepath.Join(res.ws, ".devcontainer")} + if !reflect.DeepEqual(res.build, wantArgs) { + t.Errorf("build =\n%v\nwant\n%v", res.build, wantArgs) + } +} + func TestBuildDockerfileStage(t *testing.T) { t.Run("a configured target is the base stage, the Dockerfile kept", func(t *testing.T) { res := dockerfileBuildRun{ diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index 009034c..4f75788 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -706,7 +706,7 @@ func extendImageWithFeatures( ContextPath: tmpDir, Tags: tags, Target: buildInfo.OverrideTarget, - BuildArgs: buildInfo.BuildArgs, + BuildArgs: orderedBuildArgs(buildInfo.BuildArgs, featureBuildArgOrder(buildInfo.BuildArgs)), UseBuildx: useBuildx, NoCache: fbOpts.NoCache, CacheFrom: fbOpts.CacheFrom, diff --git a/internal/cli/feature_install_held_test.go b/internal/cli/feature_install_held_test.go index f48bd19..5cfcb8d 100644 --- a/internal/cli/feature_install_held_test.go +++ b/internal/cli/feature_install_held_test.go @@ -656,6 +656,24 @@ func TestExtendImageWithFeaturesTags(t *testing.T) { } } +func TestExtendImageWithFeaturesBuildArgOrder(t *testing.T) { + res := extendRun{useBuildx: true}.run(t) + if res.err != nil { + t.Fatal(res.err) + } + var got []string + for i, a := range res.build { + if a == "--build-arg" && i+1 < len(res.build) { + got = append(got, res.build[i+1]) + } + } + want := []string{"BUILDKIT_INLINE_CACHE=1", "_DEV_CONTAINERS_BASE_IMAGE=base:latest", "_DEV_CONTAINERS_IMAGE_USER=root", + "_DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=" + imagemeta.ComposeFeatureContentImage} + if !slices.Equal(got, want) { + t.Errorf("build args = %v, want %v", got, want) + } +} + func TestExtendImageWithFeaturesUsesTheImageUser(t *testing.T) { for name, tc := range map[string]struct { config *dockerspec.DockerOCIImageConfig diff --git a/internal/config/key_order_test.go b/internal/config/key_order_test.go index 59abaaa..4ab7aa3 100644 --- a/internal/config/key_order_test.go +++ b/internal/config/key_order_test.go @@ -58,3 +58,25 @@ func TestLoadKeepsContainerEnvAndFeatureOptionOrder(t *testing.T) { } } } + +func TestLoadKeepsBuildArgsOrder(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "devcontainer.json") + override := filepath.Join(dir, "override.json") + if err := os.WriteFile(base, []byte(`{ + // comment + "build": {"dockerfile": "Dockerfile", "args": {"ZED": "1", "10": "a", "ALPHA": "2"}} + }`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(override, []byte(`{"build": {"args": {"MID": "3", "ZED": "4"}}}`), 0o644); err != nil { + t.Fatal(err) + } + res, err := LoadDevContainerConfig(dir, base, override) + if err != nil { + t.Fatal(err) + } + if want := []string{"ZED", "10", "ALPHA", "MID", "ZED"}; !slices.Equal(res.Config.BuildArgsOrder, want) { + t.Errorf("BuildArgsOrder = %v, want %v", res.Config.BuildArgsOrder, want) + } +} diff --git a/internal/config/loader.go b/internal/config/loader.go index 6d31519..ba201aa 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -211,7 +211,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi } config.ConfigFilePath = configFilePath - config.ContainerEnvOrder, config.FeatureOptionOrder = declaredOrder(docs) + config.ContainerEnvOrder, config.FeatureOptionOrder, config.BuildArgsOrder = declaredOrder(docs) return &LoadResult{ Config: &config, @@ -223,9 +223,10 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi }, nil } -func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[string][]string) { +func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[string][]string, buildArgs []string) { for _, data := range docs { containerEnv = append(containerEnv, jsonc.MemberNames(data, "containerEnv")...) + buildArgs = append(buildArgs, jsonc.MemberNames(data, "build", "args")...) for _, id := range jsonc.MemberNames(data, "features") { if names := jsonc.MemberNames(data, "features", id); len(names) > 0 { if featureOptions == nil { @@ -235,7 +236,7 @@ func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[str } } } - return containerEnv, featureOptions + return containerEnv, featureOptions, buildArgs } func deepMergeConfig(base, override map[string]any) map[string]any { diff --git a/internal/config/types.go b/internal/config/types.go index ef3a73a..3eea9fa 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -59,6 +59,7 @@ type DevContainer struct { ContainerEnvOrder []string `json:"-"` FeatureOptionOrder map[string][]string `json:"-"` + BuildArgsOrder []string `json:"-"` } type Customizations map[string]any diff --git a/internal/docker/client.go b/internal/docker/client.go index 7ace159..4d54c27 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -74,7 +74,7 @@ type BuildOptions struct { ContextPath string Tags []string Target string - BuildArgs map[string]string + BuildArgs [][2]string CacheFrom []string Labels []string NoCache bool @@ -149,8 +149,8 @@ func (c *Client) buildArgs(opts BuildOptions) []string { for _, cf := range opts.CacheFrom { args = append(args, "--cache-from", cf) } - for k, v := range opts.BuildArgs { - args = append(args, "--build-arg", fmt.Sprintf("%s=%s", k, v)) + for _, a := range opts.BuildArgs { + args = append(args, "--build-arg", a[0]+"="+a[1]) } for _, l := range opts.Labels { args = append(args, "--label", l) diff --git a/internal/docker/client_test.go b/internal/docker/client_test.go index 936f5a9..92dee06 100644 --- a/internal/docker/client_test.go +++ b/internal/docker/client_test.go @@ -104,7 +104,7 @@ func TestBuildArgs(t *testing.T) { { name: "BuildArgMap", opts: BuildOptions{ - BuildArgs: map[string]string{"NODE_VERSION": "18"}, + BuildArgs: [][2]string{{"NODE_VERSION", "18"}}, Dockerfile: "Dockerfile", ContextPath: ".", }, diff --git a/internal/docker/updateuid.go b/internal/docker/updateuid.go index 98e0409..288a06c 100644 --- a/internal/docker/updateuid.go +++ b/internal/docker/updateuid.go @@ -45,12 +45,12 @@ func BuildUIDImage(ctx context.Context, client *Client, u UIDUpdate) error { ContextPath: contextDir, Tags: []string{u.FixedImage}, ExtraArgs: extra, - BuildArgs: map[string]string{ - "BASE_IMAGE": u.BaseImage, - "REMOTE_USER": u.RemoteUser, - "NEW_UID": strconv.Itoa(u.UID), - "NEW_GID": strconv.Itoa(u.GID), - "IMAGE_USER": u.ImageUser, + BuildArgs: [][2]string{ + {"BASE_IMAGE", u.BaseImage}, + {"REMOTE_USER", u.RemoteUser}, + {"NEW_UID", strconv.Itoa(u.UID)}, + {"NEW_GID", strconv.Itoa(u.GID)}, + {"IMAGE_USER", u.ImageUser}, }, }) if err != nil { diff --git a/internal/docker/updateuid_test.go b/internal/docker/updateuid_test.go index bf840b8..8a9c652 100644 --- a/internal/docker/updateuid_test.go +++ b/internal/docker/updateuid_test.go @@ -1,8 +1,12 @@ package docker -import "strings" +import ( + "slices" + "strings" + "testing" -import "testing" + "github.com/devcontainers/cli/internal/log" +) // TestUpdateUIDDockerfileArgHasDefault guards against reintroducing the // InvalidDefaultArgInFrom BuildKit warning: the ARG used by the FROM must carry @@ -15,3 +19,23 @@ func TestUpdateUIDDockerfileArgHasDefault(t *testing.T) { t.Errorf("updateUID.Dockerfile should still FROM $BASE_IMAGE") } } + +func TestBuildUIDImageBuildArgOrder(t *testing.T) { + rr := &recordingRunner{} + c := &Client{DockerPath: "docker", Log: log.Null, Runner: rr} + err := BuildUIDImage(t.Context(), c, UIDUpdate{BaseImage: "base", FixedImage: "fixed", RemoteUser: "dev", ImageUser: "root", UID: 1000, GID: 1001, ContextDir: "/ctx"}) + if err != nil { + t.Fatal(err) + } + var got []string + args := rr.last().args + for i, a := range args { + if a == "--build-arg" && i+1 < len(args) { + got = append(got, args[i+1]) + } + } + want := []string{"BASE_IMAGE=base", "REMOTE_USER=dev", "NEW_UID=1000", "NEW_GID=1001", "IMAGE_USER=root"} + if !slices.Equal(got, want) { + t.Errorf("build args = %v, want %v", got, want) + } +}