Merge pull request #1747 from tonistiigi/default-shell-path

dockerfile: set correct default path and shell based on OS

client/llb/exec.go

@@ -153,7 +153,13 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
 	}
 	if c.Caps != nil {
 		if err := c.Caps.Supports(pb.CapExecMetaSetsDefaultPath); err != nil {
-			env = env.SetDefault("PATH", system.DefaultPathEnv)
+			os := "linux"
+			if c.Platform != nil {
+				os = c.Platform.OS
+			} else if e.constraints.Platform != nil {
+				os = e.constraints.Platform.OS
+			}
+			env = env.SetDefault("PATH", system.DefaultPathEnv(os))
 		} else {
 			addCap(&e.constraints, pb.CapExecMetaSetsDefaultPath)
 		}

client/llb/llbtest/platform_test.go

@@ -143,7 +143,7 @@ func TestFallbackPath(t *testing.T) {
 	require.False(t, def.Metadata[e.Vertex.Digest()].Caps[pb.CapExecMetaSetsDefaultPath])
 	v, ok := getenv(e, "PATH")
 	require.True(t, ok)
-	require.Equal(t, system.DefaultPathEnv, v)
+	require.Equal(t, system.DefaultPathEnvUnix, v)
 
 	// All capabilities, including pb.CapExecMetaSetsDefaultPath,
 	// so should get no PATH (not present at all, rather than

examples/buildkit0/buildkit.go

@@ -35,7 +35,7 @@ func main() {
 func goBuildBase() llb.State {
 	goAlpine := llb.Image("docker.io/library/golang:1.13-alpine")
 	return goAlpine.
-		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnv).
+		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnvUnix).
 		AddEnv("GOPATH", "/go").
 		Run(llb.Shlex("apk add --no-cache g++ linux-headers")).
 		Run(llb.Shlex("apk add --no-cache git libseccomp-dev make")).Root()

examples/buildkit1/buildkit.go

@@ -35,7 +35,7 @@ func main() {
 func goBuildBase() llb.State {
 	goAlpine := llb.Image("docker.io/library/golang:1.13-alpine")
 	return goAlpine.
-		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnv).
+		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnvUnix).
 		AddEnv("GOPATH", "/go").
 		Run(llb.Shlex("apk add --no-cache g++ linux-headers")).
 		Run(llb.Shlex("apk add --no-cache git libseccomp-dev make")).Root()

examples/buildkit2/buildkit.go

@@ -35,7 +35,7 @@ func main() {
 func goBuildBase() llb.State {
 	goAlpine := llb.Image("docker.io/library/golang:1.13-alpine")
 	return goAlpine.
-		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnv).
+		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnvUnix).
 		AddEnv("GOPATH", "/go").
 		Run(llb.Shlex("apk add --no-cache g++ linux-headers libseccomp-dev make")).Root()
 }

examples/buildkit3/buildkit.go

@@ -36,7 +36,7 @@ func main() {
 func goBuildBase() llb.State {
 	goAlpine := llb.Image("docker.io/library/golang:1.13-alpine")
 	return goAlpine.
-		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnv).
+		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnvUnix).
 		AddEnv("GOPATH", "/go").
 		Run(llb.Shlex("apk add --no-cache g++ linux-headers libseccomp-dev make")).Root()
 }

examples/nested-llb/main.go

@@ -34,7 +34,7 @@ func main() {
 func goBuildBase() llb.State {
 	goAlpine := llb.Image("docker.io/library/golang:1.13-alpine")
 	return goAlpine.
-		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnv).
+		AddEnv("PATH", "/usr/local/go/bin:"+system.DefaultPathEnvUnix).
 		AddEnv("GOPATH", "/go").
 		Run(llb.Shlex("apk add --no-cache g++ linux-headers make")).Root()
 }

exporter/containerimage/writer.go

@@ -315,7 +315,7 @@ func emptyImageConfig() ([]byte, error) {
 	}
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"
-	img.Config.Env = []string{"PATH=" + system.DefaultPathEnv}
+	img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(pl.OS)}
 	dt, err := json.Marshal(img)
 	return dt, errors.Wrap(err, "failed to create empty image config")
 }

frontend/dockerfile/dockerfile2llb/convert.go

@@ -317,7 +317,11 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
 
 		// make sure that PATH is always set
 		if _, ok := shell.BuildEnvs(d.image.Config.Env)["PATH"]; !ok {
-			d.image.Config.Env = append(d.image.Config.Env, "PATH="+system.DefaultPathEnv)
+			var os string
+			if d.platform != nil {
+				os = d.platform.OS
+			}
+			d.image.Config.Env = append(d.image.Config.Env, "PATH="+system.DefaultPathEnv(os))
 		}
 
 		// initialize base metadata from image conf
@@ -1349,7 +1353,7 @@ func withShell(img Image, args []string) []string {
 	if len(img.Config.Shell) > 0 {
 		shell = append([]string{}, img.Config.Shell...)
 	} else {
-		shell = defaultShell()
+		shell = defaultShell(img.OS)
 	}
 	return append(shell, strings.Join(args, " "))
 }

frontend/dockerfile/dockerfile2llb/defaultshell.go

@@ -0,0 +1,8 @@
+package dockerfile2llb
+
+func defaultShell(os string) []string {
+	if os == "windows" {
+		return []string{"cmd", "/S", "/C"}
+	}
+	return []string{"/bin/sh", "-c"}
+}

frontend/dockerfile/dockerfile2llb/image.go

@@ -74,6 +74,6 @@ func emptyImage(platform specs.Platform) Image {
 	}
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"
-	img.Config.Env = []string{"PATH=" + system.DefaultPathEnv}
+	img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(platform.OS)}
 	return img
 }

frontend/dockerfile/dockerfile_test.go

@@ -106,6 +106,7 @@ var allTests = []integration.Test{
 	testMultiArgs,
 	testFrontendSubrequests,
 	testDockefileCheckHostname,
+	testDefaultShellAndPath,
 }
 
 var fileOpTests = []integration.Test{
@@ -1209,6 +1210,96 @@ COPY --from=build /out .
 	require.Equal(t, "foo bar:box-foo:123 456", string(dt))
 }
 
+func testDefaultShellAndPath(t *testing.T, sb integration.Sandbox) {
+	skipDockerd(t, sb)
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(`
+FROM scratch
+ENTRYPOINT foo bar
+COPY Dockerfile .
+`)
+
+	dir, err := tmpdir(
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	c, err := client.New(context.TODO(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	destDir, err := ioutil.TempDir("", "buildkit")
+	require.NoError(t, err)
+	defer os.RemoveAll(destDir)
+
+	out := filepath.Join(destDir, "out.tar")
+	outW, err := os.Create(out)
+	require.NoError(t, err)
+
+	_, err = f.Solve(context.TODO(), c, client.SolveOpt{
+		LocalDirs: map[string]string{
+			builder.DefaultLocalNameDockerfile: dir,
+			builder.DefaultLocalNameContext:    dir,
+		},
+		FrontendAttrs: map[string]string{
+			"platform": "windows/amd64,linux/amd64",
+		},
+		Exports: []client.ExportEntry{
+			{
+				Type:   client.ExporterOCI,
+				Output: fixedWriteCloser(outW),
+			},
+		},
+	}, nil)
+	require.NoError(t, err)
+
+	dt, err := ioutil.ReadFile(filepath.Join(destDir, "out.tar"))
+	require.NoError(t, err)
+
+	m, err := testutil.ReadTarToMap(dt, false)
+	require.NoError(t, err)
+
+	var idx ocispec.Index
+	err = json.Unmarshal(m["index.json"].Data, &idx)
+	require.NoError(t, err)
+
+	mlistHex := idx.Manifests[0].Digest.Hex()
+
+	idx = ocispec.Index{}
+	err = json.Unmarshal(m["blobs/sha256/"+mlistHex].Data, &idx)
+	require.NoError(t, err)
+
+	require.Equal(t, 2, len(idx.Manifests))
+
+	for i, exp := range []struct {
+		p          string
+		entrypoint []string
+		env        []string
+	}{
+		{p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows"}},
+		{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) {
+			require.Equal(t, exp.p, platforms.Format(*idx.Manifests[i].Platform))
+
+			var mfst ocispec.Manifest
+			err = json.Unmarshal(m["blobs/sha256/"+idx.Manifests[i].Digest.Hex()].Data, &mfst)
+			require.NoError(t, err)
+
+			require.Equal(t, 1, len(mfst.Layers))
+
+			var img ocispec.Image
+			err = json.Unmarshal(m["blobs/sha256/"+mfst.Config.Digest.Hex()].Data, &img)
+			require.NoError(t, err)
+
+			require.Equal(t, exp.entrypoint, img.Config.Entrypoint)
+			require.Equal(t, exp.env, img.Config.Env)
+		})
+	}
+}
+
 func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) {
 	skipDockerd(t, sb)
 	f := getFrontend(t, sb)

frontend/gateway/container.go

@@ -227,7 +227,7 @@ func (gwCtr *gatewayContainer) Start(ctx context.Context, req client.StartReques
 	if procInfo.Meta.Cwd == "" {
 		procInfo.Meta.Cwd = "/"
 	}
-	procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnv)
+	procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnvUnix) // support windows?
 	if req.Tty {
 		procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "TERM", "xterm")
 	}

solver/llbsolver/ops/exec.go

@@ -380,7 +380,11 @@ func (e *execOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
 	if e.op.Meta.ProxyEnv != nil {
 		meta.Env = append(meta.Env, proxyEnvList(e.op.Meta.ProxyEnv)...)
 	}
-	meta.Env = addDefaultEnvvar(meta.Env, "PATH", utilsystem.DefaultPathEnv)
+	var currentOS string
+	if e.platform != nil {
+		currentOS = e.platform.OS
+	}
+	meta.Env = addDefaultEnvvar(meta.Env, "PATH", utilsystem.DefaultPathEnv(currentOS))
 
 	stdout, stderr := logs.NewLogStreams(ctx, os.Getenv("BUILDKIT_DEBUG_EXEC_OUTPUT") == "1")
 	defer stdout.Close()

util/system/path.go

@@ -0,0 +1,18 @@
+package system
+
+// DefaultPathEnvUnix is unix style list of directories to search for
+// executables. Each directory is separated from the next by a colon
+// ':' character .
+const DefaultPathEnvUnix = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
+
+// 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"
+
+func DefaultPathEnv(os string) string {
+	if os == "windows" {
+		return DefaultPathEnvWindows
+	}
+	return DefaultPathEnvUnix
+}

util/system/path_unix.go

@@ -2,11 +2,6 @@
 
 package system
 
-// DefaultPathEnv is unix style list of directories to search for
-// executables. Each directory is separated from the next by a colon
-// ':' character .
-const DefaultPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
-
 // CheckSystemDriveAndRemoveDriveLetter verifies that a path, if it includes a drive letter,
 // is the system drive. This is a no-op on Linux.
 func CheckSystemDriveAndRemoveDriveLetter(path string) (string, error) {

util/system/path_windows.go

@@ -8,10 +8,6 @@ import (
 	"strings"
 )
 
-// DefaultPathEnv is deliberately empty on Windows as the default path will be set by
-// the container. Docker has no context of what the default path should be.
-const DefaultPathEnv = ""
-
 // CheckSystemDriveAndRemoveDriveLetter verifies and manipulates a Windows path.
 // This is used, for example, when validating a user provided path in docker cp.
 // If a drive letter is supplied, it must be the system drive. The drive letter