Repository: moby/buildkit

Issue:
WCOW: support for `setx`, installers vs. `ENV`, a discussion
Related to #3158 #4895 #4901 

# Background

Environment variables are sourced in two different ways on Windows containers:

1. **Registry:** (primarily) -- this is the primary approach on how environment variables are stored and retrieved. On the shell, this done with the `setx` command. A rough equivalent of `export` on Unix systems.
2. **Image config** -- An image's config file has a [`config.Env` field](https://github.com/opencontainers/image-spec/blob/main/config.md), which containers entries in the format of VARNAME=VARVALUE; these values act as defaults and are merged with any specified when creating a container. Using dockerfile frontend, this is set using `ENV`.

For a `mcr.microsoft.com/windows/nanoserver:ltsc2022` for example, and all the other official Windows base images, the `config.Env` field is `null`. Therefore all the environment variables come from the registry.

```powershell
# quick way of inspecting the config:
PS> docker inspect mcr.microsoft.com/windows/nanoserver:ltsc2022
# <snip>
 "Config": {
            ...
            "StdinOnce": false,
            "Env": null,  # <---- this
            "Cmd": [
                "c:\\windows\\system32\\cmd.exe"
            ],
          ...
        },
# <snip>

PS>  docker run mcr.microsoft.com/windows/nanoserver:ltsc2022 cmd /c echo %PATH%
C:\Windows\system32;C:\Windows;
```

## `setx` vs. `ENV` on buildkit and classic docker builder

When there is a name collision between what is set in the config and what is set in the registry with `setx` for example, the one in config takes precedence. For example:

```dockerfile
FROM mcr.microsoft.com/windows/nanoserver:ltsc2022 AS base
USER ContainerAdministrator

RUN setx /M PATH "C:\\my\\other\\path;%PATH%"
ENV PATH="C:\\my\\path;C:\\Windows\\system32;C:\\Windows;"

FROM base
RUN echo %PATH%
```

Buildkit:
```
#6 [stage-1 1/1] RUN echo %PATH%
#6 1.864 C:\my\path;C:\Windows\system32;C:\Windows;
#6 DONE 2.2s
```

Classic `docker build`:
```
Step 6/6 : RUN echo %PATH%
 ---> Running in 4e30c4c4d266
C:\my\path;C:\Windows\system32;C:\Windows;
```

Compare with this one:

```dockerfile
FROM mcr.microsoft.com/windows/nanoserver:ltsc2022 AS base
USER ContainerAdministrator

RUN setx /M PATH "C:\\my\\other\\path;%PATH%"

FROM base
RUN echo %PATH%
```

Buildkit:
```
#6 [stage-1 1/1] RUN echo %PATH%
#6 1.897 c:\Windows\System32;c:\Windows
        ^
        |  for buildkit, if PATH is not set in the config, this default one is set instead, see #3158
#6 DONE 2.3s
```

Classic `docker build`:
```
Step 5/5 : RUN echo %PATH%
 ---> Running in 4ef43f8a3bc0
C:\\my\\other\\path;C:\Windows\system32;C:\Windows;
```

At this point, there is **no backward compatibility with classic `docker build`**.

> The difference between `buildkit` and classic `docker build` is the source for #3158 #4895.
> If the default path is left empty, Windows will handle getting the right path from registry.

## Windows Installers

Installers on Windows also set environment variables (in the registry). A good example close home, is the Go installer.
See example below:

```dockerfile
FROM mcr.microsoft.com/windows/servercore:ltsc2022

# Set environment variables for Go
ENV GOLANG_VERSION 1.23.2
# before installation
RUN echo PATH=%PATH%
RUN echo GOPATH=%GOPATH%

# Download and Install Go
RUN curl.exe -Lo go.msi "https://go.dev/dl/go%GOLANG_VERSION%.windows-amd64.msi"

RUN  msiexec.exe /i go.msi /quiet /norestart 
RUN dir .

# Verify Go installation
RUN go version

# after installation
RUN echo PATH=%PATH%
RUN echo GOPATH=%GOPATH%
```

Classic `docker build`:

```
Step 3/10 : RUN echo PATH=%PATH%
 ---> Running in 1f89b3852891
PATH=C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;
C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps

Step 4/10 : RUN echo GOPATH=%GOPATH%
 ---> Running in 9bfc156f0158
GOPATH=%GOPATH%

<snip>

Step 9/10 : RUN echo PATH=%PATH%
 ---> Running in 5c1d0d196931
PATH=C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;
C:\Windows\System32\OpenSSH\;C:\Program Files\Go\bin;
C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps;C:\Users\ContainerAdministrator\go\bin

Step 10/10 : RUN echo GOPATH=%GOPATH%
 ---> Running in ed75cae10192
GOPATH=C:\Users\ContainerAdministrator\go
```

For such case, you want to make sure to set the same environment variables declaratively in the dockerfile, for a consistent build experience across classic `docker build` and buildkit.

```dockerfile
FROM mcr.microsoft.com/windows/servercore:ltsc2022

# Set environment variables for Go
ENV GOLANG_VERSION 1.23.2
ENV GOPATH "C:\\Users\\ContainerAdministrator\\go"
ENV PATH "$PATH;C:\\Program Files\\Go\\bin;C:\\Users\ContainerAdministrator\\go\\bin"

# <snip>
```

# A case for building windows images from Linux

Buildkit can build windows images from Linux (cross-platform builds), however, with some limitations like you can't execute `RUN` statements. However, others like `COPY`, `ENV`, `WORKDIR` are supported. This is sort of an "offline build", since actual Windows containers are not spined up.

Actually, this is how Buildkit has been used for building Windows images by the early adopters, before [WCOW support came about](https://github.com/moby/buildkit/releases/tag/v0.13.0-rc1). See [`microsoft/Windows-Containers#493`](https://github.com/microsoft/Windows-Containers/issues/493) for example.

Here is a simple example dockerfile that will build currently on both Window and Linux:

```dockerfile
FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
ENV GOPATH="C:/go"

COPY ./sample.go $GOPATH/pkg/sample.go
```

Building from Linux with `buildx`:

```sh
$ docker buildx create --name buildkitd-dev \
  --driver=remote  unix:///run/buildkit/buildkitd.sock \
  --platform windows/amd64

$ docker buildx build \
  --platform windows/amd64 \
  --builder buildkitd-dev \
  --no-cache \
  --progress plain \
  --output type=image,name=profnandaa/hello-buildkit,push=false .

# <snip>
#5 [1/2] FROM mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:f59e2f21720e4d1192e0dde47e32c7dbf27144d52d79be14b2538fa07145c869
#5 CACHED

#6 [2/2] COPY ./sample.go C:/go/pkg/sample.go
#6 DONE 0.1s

#7 exporting to image
#7 exporting layers
```


# Conclusion

A proper conclusion is yet to be made after the discussion. However, my preliminary suggestion is that we prioritize maintaining a uniform experience between building Windows images on Windows vs. building them on Linux. Therefore, I suggest the declarative way of setting environment variables using `ENV`, since they are treated similarly across board.

This means leaving everything as-is with a minimal change to fix #4901 (to open PR next, #5446 ) and a clear documentation on how to handle environment variables and advisory to prefer `ENV` over `setx`.

Hints from issue comments:
@profnandaa: /cc. @thaJeztah @tonistiigi: I think Windows images should define the environment variables in the image config and not rely on the registry. Only variables in the config can be used for variable expansion in Dockerfile, they should take precedence over registry at runtime and `ENV` should remain metadata-only command.

What we could do is add some validation routines that would detect if `RUN` added a new global environment variable that was not set in image config or allow some functionality to "copy-up" value from registry to config, either at the end of the build or in `RUN`. Allowing this does disable lots of buildkit's lazy semantics though because even to just get a config and `DefinitionOp` (needed to link builds together for example) the whole build would need to run.

There is also a discussion in #3158 if some of the special cases only apply to `PATH`. Reading those responses makes me think if maybe `hcsshim` needs a special handling of `PATH` instead. E.g. something that would combine the subpaths from the image config with the ones inside the registry before running the container.@profnandaa: @tonistiigi -- thanks for taking a look!

> I think Windows images should define the environment variables in the image config and not rely on the registry. Only variables in the config can be used for variable expansion in Dockerfile, they should take precedence over registry at runtime and `ENV` should remain metadata-only command.

We actually considered taking this request to the platform team, but if this is done, it will break the current/classic `docker build` experience for `RUN setx`. I had highlighted that in https://github.com/moby/buildkit/pull/5446 

> 
> What we could do is add some validation routines that would detect if `RUN` added a new global environment variable that was not set in image config or allow some functionality to "copy-up" value from registry to config, either at the end of the build or in `RUN`. Allowing this does disable lots of buildkit's lazy semantics though because even to just get a config and `DefinitionOp` (needed to link builds together for example) the whole build would need to run.

I see, I don't know if this will be right price to pay...

> 
> There is also a discussion in [#3158](https://github.com/moby/buildkit/pull/3158) if some of the special cases only apply to `PATH`. Reading those responses makes me think if maybe `hcsshim` needs a special handling of `PATH` instead. E.g. something that would combine the subpaths from the image config with the ones inside the registry before running the container.

Makes sense to handle $PATH a little different that the rest. I think taking the `hcsshim` route sounds like our best bet. I'll be taking up this discussion to the platform team and later bubble up to HCS folks for this consideration.

In the meantime, do you think we could land https://github.com/moby/buildkit/pull/5446 as a stop-gap measure for now?

@slonopotamus: I believe that registry should not be ignored. If user installs a software that adds itself to PATH, it should Just Work in a container, without any extra steps.@profnandaa: **NOTE:** need to update documentation how to handle backward compatibility for Dockerfiles using registry to store PATH, e.g.  [docker/library/go:windowsservercore-ltsc2022](https://github.com/docker-library/golang/blob/master/1.22/windows/windowsservercore-ltsc2022/Dockerfile)

Solution patch:
diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
index e0e49dd04..934c3a338 100644
--- a/frontend/dockerfile/dockerfile2llb/convert.go
+++ b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -376,6 +376,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 					}
 				}
 				allDispatchStates.addState(ds)
+				ds.base = nil // reset base set by addState
 				continue
 			}
 		}
@@ -1065,6 +1066,8 @@ func (ds *dispatchState) init() {
 	ds.state = ds.base.state
 	ds.platform = ds.base.platform
 	ds.image = clone(ds.base.image)
+	// onbuild triggers to not carry over from base stage
+	ds.image.Config.OnBuild = nil
 	ds.baseImg = cloneX(ds.base.baseImg)
 	// Utilize the same path index as our base image so we propagate
 	// the paths we use back to the base image.
diff --git a/frontend/dockerfile/dockerfile2llb/image.go b/frontend/dockerfile/dockerfile2llb/image.go
index b6b589e77..0f10542af 100644
--- a/frontend/dockerfile/dockerfile2llb/image.go
+++ b/frontend/dockerfile/dockerfile2llb/image.go
@@ -12,6 +12,7 @@ func clone(src dockerspec.DockerOCIImage) dockerspec.DockerOCIImage {
 	img.Config.Env = append([]string{}, src.Config.Env...)
 	img.Config.Cmd = append([]string{}, src.Config.Cmd...)
 	img.Config.Entrypoint = append([]string{}, src.Config.Entrypoint...)
+	img.Config.OnBuild = append([]string{}, src.Config.OnBuild...)
 	return img
 }
 
diff --git a/util/system/path.go b/util/system/path.go
index cdfff6800..14f02b495 100644
--- a/util/system/path.go
+++ b/util/system/path.go
@@ -16,7 +16,7 @@ const DefaultPathEnvUnix = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/s
 // DefaultPathEnvWindows is windows style list of directories to search for
 // executables. Each directory is separated from the next by a colon
 // ';' character .
-const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows"
+const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows;C:\\Windows\\System32\\WindowsPowerShell\\v1.0"
 
 func DefaultPathEnv(os string) string {
 	if os == "windows" {



Test patch:
diff --git a/frontend/dockerfile/dockerfile_mount_test.go b/frontend/dockerfile/dockerfile_mount_test.go
index 56e4031b2..0423145ea 100644
--- a/frontend/dockerfile/dockerfile_mount_test.go
+++ b/frontend/dockerfile/dockerfile_mount_test.go
@@ -28,6 +28,7 @@ var mountTests = integration.TestFuncs(
 	testMountTmpfsSize,
 	testMountDuplicate,
 	testCacheMountUser,
+	testCacheMountParallel,
 )
 
 func init() {
@@ -536,3 +537,43 @@ COPY --from=base /combined.txt /
 	test("foo\n")
 	test("updated\n")
 }
+
+// moby/buildkit#5566
+func testCacheMountParallel(t *testing.T, sb integration.Sandbox) {
+	integration.SkipOnPlatform(t, "windows")
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(`
+FROM alpine AS b1
+RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 1
+
+FROM alpine AS b2
+RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 2
+
+FROM scratch
+COPY --from=b1 /etc/passwd p1
+COPY --from=b2 /etc/passwd p2
+`)
+
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	for i := 0; i < 20; i++ {
+		_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+			FrontendAttrs: map[string]string{
+				"no-cache": "",
+			},
+			LocalMounts: map[string]fsutil.FS{
+				dockerui.DefaultLocalNameDockerfile: dir,
+				dockerui.DefaultLocalNameContext:    dir,
+			},
+		}, nil)
+		require.NoError(t, err)
+	}
+}
diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go
index 8fc3b4ad7..2e7eba867 100644
--- a/frontend/dockerfile/dockerfile_test.go
+++ b/frontend/dockerfile/dockerfile_test.go
@@ -127,6 +127,7 @@ var allTests = integration.TestFuncs(
 	testEnvEmptyFormatting,
 	testCacheMultiPlatformImportExport,
 	testOnBuildCleared,
+	testOnBuildWithChildStage,
 	testOnBuildInheritedStageRun,
 	testOnBuildInheritedStageWithFrom,
 	testOnBuildNewDeps,
@@ -212,6 +213,7 @@ var allTests = integration.TestFuncs(
 	testLocalCustomSessionID,
 	testTargetStageNameArg,
 	testStepNames,
+	testPowershellInDefaultPathOnWindows,
 )
 
 // Tests that depend on the `security.*` entitlements
@@ -1812,7 +1814,7 @@ COPY Dockerfile .
 		entrypoint []string
 		env        []string
 	}{
-		{p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows"}},
+		{p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows;C:\\Windows\\System32\\WindowsPowerShell\\v1.0"}},
 		{p: "linux/amd64", entrypoint: []string{"/bin/sh", "-c", "foo bar"}, env: []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}},
 	} {
 		t.Run(exp.p, func(t *testing.T) {
@@ -1949,6 +1951,49 @@ COPY --from=base /out/ /
 	require.Equal(t, "value:final", string(dt))
 }
 
+func testPowershellInDefaultPathOnWindows(t *testing.T, sb integration.Sandbox) {
+	integration.SkipOnPlatform(t, "!windows")
+
+	f := getFrontend(t, sb)
+
+	// just testing that the powershell path is in PATH
+	// but not testing powershell itself since it will need
+	// servercore image that is too bulky for just one single test.
+	dockerfile := []byte(`
+FROM nanoserver
+USER ContainerAdministrator
+RUN echo %PATH% > env_path.txt
+`)
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	destDir := t.TempDir()
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		Exports: []client.ExportEntry{
+			{
+				Type:      client.ExporterLocal,
+				OutputDir: destDir,
+			},
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, nil)
+	require.NoError(t, err)
+
+	dt, err := os.ReadFile(filepath.Join(destDir, "env_path.txt"))
+	require.NoError(t, err)
+
+	envPath := string(dt)
+	require.Contains(t, envPath, "C:\\Windows\\System32\\WindowsPowerShell\\v1.0")
+}
+
 func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureMultiPlatform)
@@ -5013,6 +5058,96 @@ ONBUILD RUN mkdir \out && echo 11>> \out\foo
 	require.Equal(t, integration.UnixOrWindows("11", "11\r\n"), string(dt))
 }
 
+// testOnBuildWithChildStage tests that ONBUILD rules from the parent image do
+// not run again if another stage inherits from current stage.
+// moby/buildkit#5578
+func testOnBuildWithChildStage(t *testing.T, sb integration.Sandbox) {
+	integration.SkipOnPlatform(t, "windows")
+	workers.CheckFeatureCompat(t, sb, workers.FeatureDirectPush)
+	f := getFrontend(t, sb)
+
+	registry, err := sb.NewRegistry()
+	if errors.Is(err, integration.ErrRequirements) {
+		t.Skip(err.Error())
+	}
+	require.NoError(t, err)
+
+	dockerfile := []byte(`
+FROM busybox
+ONBUILD RUN mkdir -p /out && echo -n yes >> /out/didrun
+`)
+
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	target := registry + "/buildkit/testonbuildstage:base"
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		Exports: []client.ExportEntry{
+			{
+				Type: client.ExporterImage,
+				Attrs: map[string]string{
+					"push": "true",
+					"name": target,
+				},
+			},
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, nil)
+	require.NoError(t, err)
+
+	dockerfile = []byte(fmt.Sprintf(`
+FROM %s AS base
+RUN [ -f /out/didrun ] && touch /step1
+RUN rm /out/didrun
+RUN [ ! -f /out/didrun ] && touch /step2
+
+FROM base AS child
+RUN [ ! -f /out/didrun ] && touch /step3
+
+FROM scratch
+COPY --from=child /step* /
+	`, target))
+
+	dir = integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+	destDir := t.TempDir()
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		Exports: []client.ExportEntry{
+			{
+				Type:      client.ExporterLocal,
+				OutputDir: destDir,
+			},
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, nil)
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(destDir, "step1"))
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(destDir, "step2"))
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(destDir, "step3"))
+	require.NoError(t, err)
+}
+
 func testOnBuildNamedContext(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureOCILayout)
@@ -7116,6 +7251,44 @@ COPY --from=base /env_foobar /
 	dt, err = os.ReadFile(filepath.Join(destDir, "env_path"))
 	require.NoError(t, err)
 	require.Contains(t, string(dt), "/foobar:")
+
+	// this case checks replacing stage that is based on another stage.
+	// moby/buildkit#5578-2539397486
+
+	dockerfile = []byte(`
+FROM busybox AS parent
+FROM parent AS base
+RUN echo base > /out
+FROM base
+RUN [ -f /etc/alpine-release ]
+RUN [ ! -f /out ]
+`)
+
+	dir = integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	f = getFrontend(t, sb)
+
+	destDir = t.TempDir()
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		FrontendAttrs: map[string]string{
+			"context:base": "docker-image://" + target,
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+		Exports: []client.ExportEntry{
+			{
+				Type:      client.ExporterLocal,
+				OutputDir: destDir,
+			},
+		},
+	}, nil)
+	require.NoError(t, err)
 }
 
 func testNamedImageContextPlatform(t *testing.T, sb integration.Sandbox) {