fix(cli)!: build a Dockerfile configuration in one build as the reference does

docs/DIVERGENCES.md

@@ -157,14 +157,14 @@ upstream issue it fixes.
   in the build context (upstream #969). This CLI copies the ignore file next to every
   Dockerfile it generates (`TestCopyDockerignore`,
   `TestBuildAndExtendComposeDockerfileService` in `internal/cli/compose_up_held_test.go`,
-  and `TestE2E_SiblingDockerignore` in `internal/cli/e2e_fixes_test.go`).
+  `TestBuildDockerfileKeepsTheSiblingDockerignore`, and `TestE2E_SiblingDockerignore` in `internal/cli/e2e_fixes_test.go`).
 - **A `LABEL devcontainer.metadata` in the Dockerfile is kept.** TS builds the image's
   metadata label from the base image's label and the configuration and writes it over the
   image, so a `LABEL devcontainer.metadata=…` written in the built stage of the user's
   Dockerfile is lost (upstream #1225). This CLI merges that stage's entries between the
   base image's and the configuration's (`TestStageLabelsDevcontainerMetadata` and
-  `TestStageLabelsReadsTheNamedStage` in `internal/docker`, `TestDockerfileImageMetadata`
-  in `internal/cli/build_held_test.go`).
+  `TestStageLabelsReadsTheNamedStage` in `internal/docker`, `TestBuildDockerfileMetadataLabel`
+  in `internal/cli/dockerfile_build_held_test.go`).
 - **`build --label` applies to a Docker Compose configuration.** `docker compose build`
   takes no labels, and TS drops `--label` for a Compose configuration without a word
   (upstream #930). This CLI applies the labels with one more build, `FROM <image>`, tagged

docs/migration.md

@@ -101,6 +101,16 @@ runs the official CLI is unaffected:
   matching container.
 - Features are staged under `<tmp>/devcontainercli-<user>/container-features/`,
   one folder per build, and that folder is kept as the official CLI keeps it.
+- A Dockerfile configuration's image is made by one `docker build`, as the
+  official CLI makes it: the Dockerfile followed by the stage that installs the
+  Features and writes the `devcontainer.metadata` label, written as
+  `Dockerfile-with-features` into that Features folder and built with
+  `--target dev_containers_target_stage`. Earlier releases built the Dockerfile
+  first, wrote `<Dockerfile>.devcontainer.build` next to it when its last stage had
+  no name (failing in a read-only workspace), and added the Features in a second
+  build. `up` of a Dockerfile configuration with Features now tags the image
+  `vsc-<folder>-<hash>`, not `vsc-<folder>-<hash>-features`; a script that looked
+  the image up by the old name should use the new one.
 - When the workspace is a Git worktree and the configuration sets both
   `workspaceMount` and `workspaceFolder`, `up` no longer mounts the main
   repository's Git common directory, and `read-configuration` no longer reports

docs/parity/parity-matrix.yaml

@@ -1779,6 +1779,34 @@ initial_cases:
     current_status: match
     notes: "Without build.context the context is the Dockerfile's folder (utils.ts getDockerContextPath: parentURI(getDockerfilePath)), not the configuration's: COPY finds the file next to sub/Dockerfile. Go used the configuration's folder and the COPY failed."
 
+  - id: build.read-only-workspace-dockerfile-success
+    lane: runtime
+    command: build
+    priority: p1
+    docker_required: true
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; chmod -R u+w $WORKSPACE 2>/dev/null; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s\\n' 'FROM alpine:3' 'RUN echo ok > /marker' > $WORKSPACE/.devcontainer/Dockerfile; printf '%s' '{\"build\":{\"dockerfile\":\"Dockerfile\"},\"remoteUser\":\"root\"}' > $WORKSPACE/.devcontainer/devcontainer.json; chmod -R a-w $WORKSPACE; echo WORKSPACE=$WORKSPACE"
+    ts_cmd: "build --workspace-folder ${WORKSPACE} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    verify_cmd: "cd ${WORKSPACE} && find . | sort; docker history --no-trunc --format '{{.CreatedBy}}' ${IMAGE_NAME}"
+    cleanup_cmd: "docker rmi -f parity-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; chmod -R u+w /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE} 2>/dev/null; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true"
+    asserts: [exit_code, stdout_normalized]
+    class: build-runtime
+    current_status: match
+    notes: "TS buildAndExtendImage (singleContainer.ts) builds a Dockerfile configuration in one docker build of Dockerfile-with-features, written into the Features folder under the cache folder: the user's Dockerfile with its final stage named, then FROM $_DEV_CONTAINERS_BASE_IMAGE AS dev_containers_target_stage with the devcontainer.metadata LABEL, built with --target dev_containers_target_stage. A read-only workspace builds, nothing is written into it, and the image history ends with that LABEL instruction. Go wrote <Dockerfile>.devcontainer.build next to the Dockerfile (failing here with permission denied), built it with --target <stage> and --label devcontainer.metadata=..., and added Features in a second build."
+
+  - id: up.read-only-workspace-dockerfile-success
+    lane: runtime
+    command: up
+    priority: p1
+    docker_required: true
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; chmod -R u+w $WORKSPACE 2>/dev/null; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s\\n' 'FROM alpine:3' 'RUN echo ok > /marker' > $WORKSPACE/.devcontainer/Dockerfile; printf '%s' '{\"build\":{\"dockerfile\":\"Dockerfile\"},\"remoteUser\":\"root\"}' > $WORKSPACE/.devcontainer/devcontainer.json; chmod -R a-w $WORKSPACE; echo WORKSPACE=$WORKSPACE"
+    ts_cmd: "up --workspace-folder ${WORKSPACE} --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}"
+    verify_cmd: "cd ${WORKSPACE} && find . | sort; docker exec ${CONTAINER_ID} cat /marker; I=$(docker inspect -f '{{.Image}}' ${CONTAINER_ID}); docker history --no-trunc --format '{{.CreatedBy}}' $I"
+    cleanup_cmd: "I=$(docker inspect -f '{{.Image}}' ${CONTAINER_ID} 2>/dev/null); docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; [ -n \"$I\" ] && docker rmi -f $I >/dev/null 2>&1; chmod -R u+w /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE} 2>/dev/null; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true"
+    asserts: [exit_code, stdout_normalized]
+    class: up-runtime
+    current_status: match
+    notes: "As build.read-only-workspace-dockerfile-success, for up: one build of Dockerfile-with-features from the cache folder, so a read-only workspace starts and holds only what it held. Go failed writing <Dockerfile>.devcontainer.build into the workspace."
+
   - id: upgrade.write-lockfile-to-disk-success
     lane: runtime
     command: upgrade

internal/cli/build.go

@@ -44,6 +44,7 @@ type buildOpts struct {
 	secretsFile                string
 	secrets                    fileSecrets
 	caBundle                   string
+	omitSyntaxDirective        bool
 	lockfileExcludeIDs         map[string]bool
 }
 
@@ -92,7 +93,7 @@ func newBuildCmd() *cobra.Command {
 	f.BoolVar(&opts.frozenLockfile, "frozen-lockfile", false, "Ensure lockfile exists and remains unchanged; fail otherwise.")
 	f.StringVar(&opts.secretsFile, "secrets-file", "", "Path to a JSON file with build secrets ({\"KEY\":\"VALUE\"}); each is passed to BuildKit as a build secret (requires buildx).")
 	f.StringVar(&opts.caBundle, "ca-bundle", "", caBundleFlagUsage)
-	f.Bool("omit-syntax-directive", false, "")
+	f.BoolVar(&opts.omitSyntaxDirective, "omit-syntax-directive", false, "")
 	cmd.Flags().MarkHidden("skip-feature-auto-mapping")
 	cmd.Flags().MarkHidden("skip-persisting-customizations-from-features")
 	cmd.Flags().MarkHidden("experimental-lockfile")
@@ -267,15 +268,6 @@ func buildFlagConflict(opts *buildOpts, useBuildx, compose bool) string {
 	return ""
 }
 
-type dockerfileBuildPrep struct {
-	Path      string
-	StageName string
-	Parsed    *docker.Dockerfile
-	cleanup   func()
-}
-
-func (p *dockerfileBuildPrep) Cleanup() { p.cleanup() }
-
 // copyDockerignore copies a Dockerfile's sibling `<name>.dockerignore` next to a
 // renamed/generated build Dockerfile, so BuildKit's per-Dockerfile ignore lookup
 // (`<dockerfile>.dockerignore`) still finds the user's rules instead of leaking
@@ -307,132 +299,21 @@ func dockerContextPath(cfg *config.DevContainer) string {
 	return filepath.Dir(dockerfileConfigPath(cfg))
 }
 
-func prepareDockerfileBuild(cfg *config.DevContainer) (*dockerfileBuildPrep, error) {
-	dockerfilePath := cfg.Dockerfile()
-	if dockerfilePath == "" {
-		return nil, fmt.Errorf("no Dockerfile specified")
-	}
-	dockerfilePath = dockerfileConfigPath(cfg)
-
-	content, err := os.ReadFile(dockerfilePath)
-	if err != nil {
-		return nil, fmt.Errorf("read Dockerfile: %w", err)
-	}
-
-	prep := &dockerfileBuildPrep{
-		Parsed:  docker.ExtractDockerfile(string(content)),
-		cleanup: func() {},
-	}
-
-	if cfg.Build != nil && cfg.Build.Target != "" {
-		prep.StageName = cfg.Build.Target
-	} else {
-		stageName, modified, err := docker.EnsureFinalStageName(string(content), "dev_container_auto_added_stage_label")
-		if err != nil {
-			return nil, err
-		}
-		prep.StageName = stageName
-		if modified != "" {
-			tmpDockerfile := dockerfilePath + ".devcontainer.build"
-			// mutate-exempt: only the docker build this process starts reads the file, as its owner, and Cleanup removes it
-			if err := os.WriteFile(tmpDockerfile, []byte(modified), 0644); err != nil {
-				return nil, fmt.Errorf("write build dockerfile: %w", err)
-			}
-			copyDockerignore(dockerfilePath, tmpDockerfile)
-			prep.cleanup = func() {
-				os.Remove(tmpDockerfile)
-				os.Remove(tmpDockerfile + ".dockerignore")
-			}
-			dockerfilePath = tmpDockerfile
-		}
-	}
-
-	prep.Path = dockerfilePath
-	return prep, nil
-}
-
-// mutate-lane: TestParityMatrix (build.dockerfile-multi-image-success,
-// build.subfolder-config-success, build.apply-build-options-success,
-// build.output-oci-success, build.cache-to-local-success,
-// build.platform-supported-dockerfile-success), also run by
+// mutate-lane: TestBuildDockerfileBuildsOnce and TestParityMatrix
+// (build.dockerfile-multi-image-success, build.subfolder-config-success,
+// build.apply-build-options-success, build.output-oci-success,
+// build.cache-to-local-success, build.platform-supported-dockerfile-success,
+// build.read-only-workspace-dockerfile-success), also run by
 // TestE2E_BuildUpExec_Dockerfile, TestE2E_SiblingDockerignore and
 // TestE2E_CABundle (an opted-in RUN reads the bundle).
 func (r *buildRunner) buildDockerfile(ctx context.Context, cfg *config.DevContainer, loadResult *config.LoadResult, useBuildx bool) ([]string, error) {
-	logger, dockerClient, engine, opts := r.log, r.docker, r.engine, r.opts
-
-	prep, err := prepareDockerfileBuild(cfg)
-	if err != nil {
-		return nil, err
-	}
-	defer prep.Cleanup()
-	dockerfilePath := prep.Path
-	stageName := prep.StageName
-
-	findTarget := ""
-	if cfg.Build != nil {
-		findTarget = cfg.Build.Target
-	}
-	baseImage := docker.FindBaseImage(prep.Parsed, buildArgsFromConfig(cfg), findTarget, buildxPlatformArgs(opts.platform))
-	logger.Write(fmt.Sprintf("Base image: %s", baseImage), log.LevelInfo)
-
-	baseLabels := engine.ImageLabelsEnsuringPresent(ctx, baseImage)
-	baseMetadata := imagemeta.ReadMetadataFromLabels(baseLabels, logger)
-	stageMetadata := imagemeta.ReadMetadataFromLabels(prep.Parsed.StageLabels(stageName), logger)
-
+	opts := r.opts
+	fb := r.featureBuildOptions(cfg, &loadResult.HostSub)
+	fb.Secrets = opts.secrets.env()
 	imageNames := imageNamesOr(opts.imageNames, folderImageName(resolvePath(opts.workspaceFolder)))
-
-	contextPath := dockerContextPath(cfg)
-
-	buildArgs := buildArgsFromConfig(cfg)
-
-	allLabels := append(slices.Clone(opts.labels), imageLabelArgs(dockerfileImageMetadata(cfg, baseMetadata, stageMetadata, r.configLabel))...)
-
-	baseOutput, basePush := dockerfileBaseExport(opts, cfg)
-
-	cacheFrom := cacheFromForDockerfileBuild(opts.cacheFrom, cfg)
-	authEnv, authCleanup := bridgeBuildAuth(logger, baseImage, imageNames, cacheFrom, opts.cacheTo)
-	defer authCleanup()
-
-	buildSecrets := buildxSecrets(opts.secrets.env(), useBuildx, logger)
-	caSecrets := caBundleBuildSecrets(opts.caBundle, useBuildx, logger)
-
-	buildOpts := docker.BuildOptions{
-		Dockerfile:  dockerfilePath,
-		ContextPath: contextPath,
-		Tags:        imageNames,
-		Target:      stageName,
-		BuildArgs:   buildArgs,
-		CacheFrom:   cacheFrom,
-		Labels:      allLabels,
-		NoCache:     opts.noCache,
-		ExtraArgs:   buildOptionsFromConfig(cfg),
-		UseBuildx:   useBuildx,
-		Platform:    opts.platform,
-		Push:        basePush,
-		Output:      baseOutput,
-		CacheTo:     opts.cacheTo,
-		Env:         authEnv,
-		Secrets:     buildSecrets,
-		FileSecrets: caSecrets,
-	}
-
-	result, err := dockerClient.Build(ctx, buildOpts)
-	if err != nil {
+	if err := buildDockerfileImage(ctx, r.log, r.docker, r.engine, cfg, useBuildx, imageNames, opts.platform, fb); err != nil {
 		return nil, err
 	}
-	if result.ExitCode != 0 {
-		return nil, fmt.Errorf(msgDockerBuildFailed, result.ExitCode, string(result.Stderr))
-	}
-
-	if dockerClient.ProgressWriter == nil {
-		logger.Write(string(result.Stderr), log.LevelInfo)
-	}
-
-	if len(cfg.Features) > 0 {
-		baseImageName := imageNames[0]
-		return extendImageWithFeatures(ctx, logger, dockerClient, engine, baseImageName, cfg.Features, useBuildx, imageNames, r.featureBuildOptions(cfg, &loadResult.HostSub))
-	}
-
 	return imageNames, nil
 }
 
@@ -593,26 +474,6 @@ func imageNamesOr(names []string, fallback string) []string {
 	return names
 }
 
-// dockerfileImageMetadata is the metadata a Dockerfile build stamps on its
-// image: the base image's, then a `LABEL devcontainer.metadata` the user's
-// Dockerfile declares (#1225; the CLI's own label would otherwise clobber it),
-// then the config's entry as written (configLabel). With Features the Feature
-// build appends that entry instead.
-func dockerfileImageMetadata(cfg *config.DevContainer, base, stage []imagemeta.Entry, configLabel imagemeta.Entry) []imagemeta.Entry {
-	metadata := append(append([]imagemeta.Entry{}, base...), stage...)
-	if len(cfg.Features) == 0 {
-		metadata = append(metadata, configLabel)
-	}
-	return metadata
-}
-
-func dockerfileBaseExport(opts *buildOpts, cfg *config.DevContainer) (output string, push bool) {
-	if len(cfg.Features) > 0 {
-		return "", false
-	}
-	return opts.output, opts.push
-}
-
 func buildxSecrets(secrets []string, useBuildx bool, logger log.Logger) []string {
 	if useBuildx {
 		return secrets
@@ -746,6 +607,7 @@ func (r *buildRunner) buildCompose(ctx context.Context, cfg *config.DevContainer
 			noLockfile:                 opts.noLockfile,
 			lockfileExcludeIDs:         opts.lockfileExcludeIDs,
 			caBundle:                   opts.caBundle,
+			omitSyntaxDirective:        opts.omitSyntaxDirective,
 		},
 		substitute:  hostMetadataSubstitution(loadResult.HostSub, nil),
 		configLabel: r.configLabel,
@@ -840,5 +702,6 @@ func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *con
 		WorkspaceFolder:             opts.workspaceFolder,
 		AdditionalMetadata:          []imagemeta.Entry{configToMetadataEntry(cfg)},
 		LabelMetadata:               []imagemeta.Entry{r.configLabel},
+		OmitSyntaxDirective:         opts.omitSyntaxDirective,
 	}
 }

internal/cli/build_held_test.go

@@ -11,7 +11,6 @@ import (
 	"testing"
 
 	"github.com/devcontainers/cli/internal/config"
-	"github.com/devcontainers/cli/internal/imagemeta"
 	"github.com/devcontainers/cli/internal/log"
 )
 
@@ -132,40 +131,6 @@ func TestImageNamesOr(t *testing.T) {
 	}
 }
 
-func TestDockerfileImageMetadata(t *testing.T) {
-	base := []imagemeta.Entry{{ID: "base"}}
-	stage := []imagemeta.Entry{{ID: "stage"}}
-	ids := func(entries []imagemeta.Entry) []string {
-		var out []string
-		for _, e := range entries {
-			out = append(out, e.ID+"/"+e.RemoteUser)
-		}
-		return out
-	}
-
-	cfg := &config.DevContainer{RemoteUser: "dev"}
-	configLabel := imagemeta.Entry{RemoteUser: "${localEnv:USER}"}
-	if got, want := ids(dockerfileImageMetadata(cfg, base, stage, configLabel)), []string{"base/", "stage/", "/${localEnv:USER}"}; !slices.Equal(got, want) {
-		t.Errorf("without features: %v, want %v (the config's entry as written)", got, want)
-	}
-
-	cfg.Features = map[string]any{"ghcr.io/devcontainers/features/go:1": true}
-	if got, want := ids(dockerfileImageMetadata(cfg, base, stage, configLabel)), []string{"base/", "stage/"}; !slices.Equal(got, want) {
-		t.Errorf("with features (the Feature build adds the config entry): %v, want %v", got, want)
-	}
-}
-
-func TestDockerfileBaseExport(t *testing.T) {
-	opts := &buildOpts{output: "type=oci,dest=x.tar", push: true}
-	if output, push := dockerfileBaseExport(opts, &config.DevContainer{}); output != opts.output || !push {
-		t.Errorf("without features: (%q, %v), want the flags", output, push)
-	}
-	withFeature := &config.DevContainer{Features: map[string]any{"ghcr.io/devcontainers/features/go:1": true}}
-	if output, push := dockerfileBaseExport(opts, withFeature); output != "" || push {
-		t.Errorf("with features: (%q, %v), want them left to the Feature build", output, push)
-	}
-}
-
 func TestBuildxSecrets(t *testing.T) {
 	secrets := []string{"TOKEN=x"}
 	var logged bytes.Buffer
@@ -222,33 +187,6 @@ func TestRunBuildBuildKitNeverIgnoresBuildx(t *testing.T) {
 	}
 }
 
-func TestPrepareDockerfileBuildStage(t *testing.T) {
-	ws := writeConfigFixture(t, map[string]string{
-		".devcontainer/Dockerfile": "FROM alpine:3 AS base\nFROM base\n",
-		".devcontainer/Empty":      "RUN true\n",
-	})
-	cfgFile := filepath.Join(ws, ".devcontainer", "devcontainer.json")
-	prep, err := prepareDockerfileBuild(&config.DevContainer{ConfigFilePath: cfgFile, Build: &config.Build{Dockerfile: "Dockerfile", Target: "base"}})
-	if err != nil || prep.StageName != "base" {
-		t.Fatalf("with a target: %+v, %v; want stage base", prep, err)
-	}
-	prep.Cleanup()
-	prep, err = prepareDockerfileBuild(&config.DevContainer{ConfigFilePath: cfgFile, Build: &config.Build{Dockerfile: "Dockerfile"}})
-	if err != nil || prep.StageName != "dev_container_auto_added_stage_label" {
-		t.Fatalf("without a target: %+v, %v", prep, err)
-	}
-	if _, err := os.Stat(prep.Path); err != nil {
-		t.Fatalf("named Dockerfile: %v", err)
-	}
-	prep.Cleanup()
-	if _, err := os.Stat(prep.Path); !os.IsNotExist(err) {
-		t.Errorf("named Dockerfile %s left after Cleanup: %v", prep.Path, err)
-	}
-	if _, err := prepareDockerfileBuild(&config.DevContainer{ConfigFilePath: cfgFile, Build: &config.Build{Dockerfile: "Empty"}}); err == nil || !strings.Contains(err.Error(), "no FROM instructions") {
-		t.Errorf("no FROM: err = %v", err)
-	}
-}
-
 func TestBuildResultImageName(t *testing.T) {
 	for _, tc := range []struct {
 		name     string