solver: use toSelectors to filter root paths instead of custom logic

This updates moby#4270 to add an integration test and also merge some of the
logic for how the selectors are created. Now, `toSelectors` will perform
the root path detection instead of some custom logic in `getMountDeps`.

`dedupePaths` has also been updated to check if the number of paths is 1
or less so it can avoid an allocation when the function is a no-op.

Signed-off-by: Jonathan A. Sternberg <[email protected]>

frontend/dockerfile/dockerfile_mount_test.go

@@ -1,6 +1,7 @@
 package dockerfile
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -24,6 +25,7 @@ var mountTests = integration.TestFuncs(
 	testMountFromError,
 	testMountInvalid,
 	testMountTmpfsSize,
+	testMountDuplicate,
 	testCacheMountUser,
 )
 
@@ -468,3 +470,55 @@ COPY --from=base /tmpfssize /
 	require.NoError(t, err)
 	require.Contains(t, string(dt), `size=131072k`)
 }
+
+// moby/buildkit#4123
+func testMountDuplicate(t *testing.T, sb integration.Sandbox) {
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(`
+FROM busybox AS base
+RUN --mount=source=.,target=/tmp/test \
+  --mount=source=b.txt,target=/tmp/b.txt \
+  cat /tmp/test/a.txt /tmp/b.txt > /combined.txt
+FROM scratch
+COPY --from=base /combined.txt /
+`)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	destDir := t.TempDir()
+
+	// Run this dockerfile a few times. It should update the context
+	// for a.txt properly and update the output.
+	test := func(text string) {
+		dir := integration.Tmpdir(
+			t,
+			fstest.CreateFile("Dockerfile", dockerfile, 0600),
+			fstest.CreateFile("a.txt", []byte(text), 0600),
+			fstest.CreateFile("b.txt", []byte("bar\n"), 0600),
+		)
+
+		_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+			Exports: []client.ExportEntry{
+				{
+					Type:      client.ExporterLocal,
+					OutputDir: destDir,
+				},
+			},
+			LocalDirs: map[string]string{
+				dockerui.DefaultLocalNameDockerfile: dir,
+				dockerui.DefaultLocalNameContext:    dir,
+			},
+		}, nil)
+		require.NoError(t, err)
+
+		dt, err := os.ReadFile(filepath.Join(destDir, "combined.txt"))
+		require.NoError(t, err)
+		require.Equal(t, fmt.Sprintf("%sbar\n", text), string(dt))
+	}
+
+	test("foo\n")
+	test("updated\n")
+}

solver/llbsolver/ops/exec.go

@@ -189,7 +189,10 @@ func dedupePaths(inp []string) []string {
 	for p1 := range old {
 		var skip bool
 		for p2 := range old {
-			if p1 != p2 && strings.HasPrefix(p1, p2+"/") {
+			// Check if p2 is a prefix of p1. Ensure that p2 ends in a slash
+			// so that we know p2 is a parent directory of p1. We don't want
+			// /foo to be a parent of /foobar.
+			if p1 != p2 && strings.HasPrefix(p1, forceTrailingSlash(p2)) {
 				skip = true
 				break
 			}
@@ -204,6 +207,15 @@ func dedupePaths(inp []string) []string {
 	return paths
 }
 
+// forceTrailingSlash ensures that the path always ends with a path separator.
+// If the path already ends with a /, this method returns the same string.
+func forceTrailingSlash(s string) string {
+	if strings.HasSuffix(s, "/") {
+		return s
+	}
+	return s + "/"
+}
+
 func toSelectors(p []string) []opsutils.Selector {
 	sel := make([]opsutils.Selector, 0, len(p))
 	for _, p := range p {
@@ -227,28 +239,13 @@ func (e *ExecOp) getMountDeps() ([]dep, error) {
 			return nil, errors.Errorf("invalid mountinput %v", m)
 		}
 
-		// Mark the selector path as used. In this section, we need to
-		// record root selectors so the selection criteria isn't narrowed
-		// erroneously.
 		sel := path.Join("/", m.Selector)
 		deps[m.Input].Selectors = append(deps[m.Input].Selectors, sel)
 
 		if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 { // exclude read-only rootfs && read-write mounts
 			deps[m.Input].NoContentBasedHash = true
 		}
 	}
-
-	// Remove extraneous selectors that may have been generated from above.
-	for i, dep := range deps {
-		for _, sel := range dep.Selectors {
-			// If the root path is included in the list of selectors,
-			// this is the same as if no selector was used. Zero out this field.
-			if sel == "/" {
-				deps[i].Selectors = nil
-				break
-			}
-		}
-	}
 	return deps, nil
 }
 

solver/llbsolver/ops/exec_test.go

@@ -3,9 +3,6 @@ package ops
 import (
 	"testing"
 
-	"github.com/moby/buildkit/solver"
-	"github.com/moby/buildkit/solver/pb"
-	digest "github.com/opencontainers/go-digest"
 	"github.com/stretchr/testify/require"
 )
 
@@ -27,73 +24,7 @@ func TestDedupPaths(t *testing.T) {
 
 	res = dedupePaths([]string{"foo/bar/baz", "foo/bara", "foo/bar/bax", "foo/bar"})
 	require.Equal(t, []string{"foo/bar", "foo/bara"}, res)
-}
-
-func TestExecOp_getMountDeps(t *testing.T) {
-	v1 := &vertex{name: "local://context"}
-	v2 := &vertex{
-		name: "foo",
-		inputs: []solver.Edge{
-			{Vertex: v1, Index: 0},
-		},
-	}
-	op2, err := NewExecOp(v2, &pb.Op_Exec{
-		Exec: &pb.ExecOp{
-			Meta: &pb.Meta{
-				Args: []string{"/bin/bash", "-l"},
-			},
-			Mounts: []*pb.Mount{
-				{
-					Input:    pb.Empty,
-					Dest:     "/",
-					Readonly: true,
-					Output:   pb.SkipOutput,
-				},
-				{
-					Input:    pb.InputIndex(0),
-					Selector: "b.txt",
-					Dest:     "/test/b.txt",
-					Output:   pb.SkipOutput,
-				},
-				{
-					Input:  pb.InputIndex(0),
-					Dest:   "/test/data",
-					Output: pb.SkipOutput,
-				},
-			},
-		},
-	}, nil, nil, nil, nil, nil, nil)
-	require.NoError(t, err)
-
-	deps, err := op2.getMountDeps()
-	require.NoError(t, err)
-
-	require.Len(t, deps, 1)
-	require.Len(t, deps[0].Selectors, 0)
-	require.False(t, deps[0].NoContentBasedHash)
-}
-
-type vertex struct {
-	name   string
-	inputs []solver.Edge
-}
-
-func (v *vertex) Digest() digest.Digest {
-	return digest.FromString(v.name)
-}
-
-func (v *vertex) Sys() interface{} {
-	return v
-}
-
-func (v *vertex) Options() solver.VertexOptions {
-	return solver.VertexOptions{}
-}
-
-func (v *vertex) Inputs() []solver.Edge {
-	return v.inputs
-}
 
-func (v *vertex) Name() string {
-	return v.name
+	res = dedupePaths([]string{"/", "/foo"})
+	require.Equal(t, []string{"/"}, res)
 }