Merge branch 'master' into use-separator-for-arch

ChangeLog

@@ -12,6 +12,13 @@ Version 17.4.0
     * Fix reduce builtin (#2925)
     * Fix issue with completing idents in the language server (#2917)
     * Improve performance when a large number of input targets are supplied (#2942)
+    * Virtualise the subrepo filesystem for remote execution to avoid downloading
+      the whole subrepo to plz-out (#2952, #2954, #2953, #2955, #2960, #2961,
+      #2962, #2963, #2966, #2968, #2965)
+    * Fix issue where we would double the architecture subrepo if it was explicitly
+      specified in a subrepo (#2973)
+    * Always treat the first argument of `plz run` commands as a build label (#2967)
+    * Redact secrets from test step logging (#2970)
 
 Version 17.3.1
 --------------

VERSION

@@ -1 +1 @@
-17.4.0-beta.4
+17.4.0-beta.6

docs/BUILD

@@ -59,7 +59,7 @@ genrule(
 plugins = {
     "python": "v1.5.0",
     "java": "v0.3.0",
-    "go": "v1.11.3",
+    "go": "v1.11.4",
     "cc": "v0.4.0",
     "shell": "v0.2.0",
     "go-proto": "v0.2.1",

docs/language.html

@@ -271,6 +271,12 @@ <h2 id="grammar" class="title-2">
     if they make mistakes in their BUILD files (e.g. passing a string where a
     list is required).
   </p>
+  <p>
+    Additionally, arguments can be aliased using the
+    <code class="code">def func(arg_name: str&arg_alias):...</code> syntax, such that
+    <code class="code">func(arg_name="example")</code> and
+    <code class="code">func(arg_alias="example")</code> are equivalent.
+  </p>
 
   <p>User-defined varargs and kwargs functions are not supported.</p>
 

plugins/BUILD

@@ -1,7 +1,7 @@
 plugin_repo(
     name = "go",
     plugin = "go-rules",
-    revision = "v1.11.3",
+    revision = "v1.11.4",
 )
 
 plugin_repo(

src/core/state.go

@@ -8,6 +8,7 @@ import (
 	"hash/crc32"
 	"hash/crc64"
 	"io"
+	iofs "io/fs"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -112,6 +113,8 @@ type RemoteClient interface {
 	DataRate() (int, int, int, int)
 	// Disconnect disconnects from the remote execution server.
 	Disconnect() error
+	// SubrepoFS returns a virtual filesystem for the subrepo target
+	SubrepoFS(target *BuildTarget, root string) iofs.FS
 }
 
 // A TargetHasher is a thing that knows how to create hashes for targets.
@@ -846,12 +849,7 @@ func (state *BuildState) WaitForPackage(l, dependent BuildLabel, mode ParseMode)
 	return state.WaitForPackage(l, dependent, mode)
 }
 
-// WaitForBuiltTarget blocks until the given label is available as a build target and has been successfully built.
-func (state *BuildState) WaitForBuiltTarget(l, dependent BuildLabel) *BuildTarget {
-	return state.waitForBuiltTarget(l, dependent, ParseModeForSubinclude)
-}
-
-func (state *BuildState) waitForBuiltTarget(l, dependent BuildLabel, mode ParseMode) *BuildTarget {
+func (state *BuildState) WaitForBuiltTarget(l, dependent BuildLabel, mode ParseMode) *BuildTarget {
 	if t := state.Graph.Target(l); t != nil {
 		if t.State().IsBuilt() {
 			return t
@@ -871,7 +869,7 @@ func (state *BuildState) waitForBuiltTarget(l, dependent BuildLabel, mode ParseM
 	// Do this all over; the re-checking that happens here is actually fairly important to resolve
 	// a potential race condition if the target was built between us checking earlier and registering
 	// the channel just now.
-	return state.waitForBuiltTarget(l, dependent, mode)
+	return state.WaitForBuiltTarget(l, dependent, mode)
 }
 
 // AddTarget adds a new target to the build graph.
@@ -947,7 +945,7 @@ func (state *BuildState) WaitForInitialTargetAndEnsureDownload(l, dependent Buil
 }
 
 func (state *BuildState) waitForTargetAndEnsureDownload(l, dependent BuildLabel, mode ParseMode) *BuildTarget {
-	target := state.waitForBuiltTarget(l, dependent, mode)
+	target := state.WaitForBuiltTarget(l, dependent, mode)
 	if !target.State().IsBuilt() {
 		return nil
 	}
@@ -1074,7 +1072,7 @@ func (state *BuildState) QueueTestTarget(target *BuildTarget) {
 func (state *BuildState) queueTargetData(target *BuildTarget) {
 	for _, data := range target.AllData() {
 		if l, ok := data.Label(); ok {
-			state.WaitForBuiltTarget(l, target.Label)
+			state.WaitForBuiltTarget(l, target.Label, ParseModeForSubinclude)
 		}
 	}
 }

src/core/subrepo.go

@@ -18,8 +18,6 @@ type Subrepo struct {
 	Name string
 	// The root directory to load it from.
 	Root string
-	// A file system rooted at the subrepo's root directory.
-	FS iofs.FS
 	// A root directory for outputs of this subrepo's targets
 	PackageRoot string
 	// If this repo is output by a target, this is the target that creates it. Can be nil.
@@ -36,22 +34,32 @@ type Subrepo struct {
 }
 
 func NewSubrepo(state *BuildState, name, root string, target *BuildTarget, arch cli.Arch, isCrosscompile bool) *Subrepo {
-	subrepoFS := os.DirFS(root)
-	if root == "" {
-		// This happens for architecture subrepos, which should use the same FS as the host repo
-		subrepoFS = fs.HostFS
-	}
 	return &Subrepo{
 		Name:           name,
 		Root:           root,
-		FS:             subrepoFS,
 		State:          state,
 		Target:         target,
 		Arch:           arch,
 		IsCrossCompile: isCrosscompile,
 	}
 }
 
+func (s *Subrepo) FS() iofs.FS {
+	if s == nil || s.Root == "" {
+		// Must be an architecture subrepo
+		return fs.HostFS
+	}
+	if s.IsRemoteSubrepo() {
+		return s.State.RemoteClient.SubrepoFS(s.Target, s.Root)
+	}
+	return os.DirFS(s.Root)
+}
+
+// IsRemoteSubrepo returns true when the subrepo sources are remote i.e. not downloaded to plz-out
+func (s *Subrepo) IsRemoteSubrepo() bool {
+	return s.Root != "" && s.Target != nil && !s.Target.Local && s.State.RemoteClient != nil
+}
+
 // SubrepoForArch creates a new subrepo for the given architecture.
 func SubrepoForArch(state *BuildState, arch cli.Arch) *Subrepo {
 	s := NewSubrepo(state.ForArch(arch), arch.String(), "", nil, arch, true)
@@ -93,7 +101,7 @@ func readSubrepoConfig(repoConfig *Configuration, subrepo *Subrepo) error {
 			return err
 		}
 	}
-	return readConfigFile(subrepo.FS, repoConfig, ".plzconfig", true)
+	return readConfigFile(subrepo.FS(), repoConfig, ".plzconfig", true)
 }
 
 func validateSubrepoNameAndPluginConfig(config, repoConfig *Configuration, subrepo *Subrepo) error {

src/help/help.go

@@ -236,7 +236,8 @@ func getPluginBuildDefs(subrepo *core.Subrepo) map[string]*asp.Statement {
 	p := asp.NewParser(subrepo.State)
 	ret := make(map[string]*asp.Statement)
 	for _, dir := range dirs {
-		dirEntries, err := iofs.ReadDir(subrepo.FS, dir)
+		fs := subrepo.FS()
+		dirEntries, err := iofs.ReadDir(fs, dir)
 		if err != nil {
 			log.Warningf("Failed to read %s: %s", dir, err)
 		}
@@ -246,7 +247,7 @@ func getPluginBuildDefs(subrepo *core.Subrepo) map[string]*asp.Statement {
 			}
 
 			path := filepath.Join(dir, entry.Name())
-			bs, err := iofs.ReadFile(subrepo.FS, path)
+			bs, err := iofs.ReadFile(fs, path)
 			if err != nil {
 				log.Warningf("Failed to read %s: %s", path, err)
 			}

src/parse/asp/builtins.go

@@ -608,7 +608,7 @@ func glob(s *scope, args []pyObject) pyObject {
 	exclude = append(exclude, s.state.Config.Parse.BuildFileName...)
 	if s.globber == nil {
 		if s.pkg.Subrepo != nil {
-			s.globber = fs.NewGlobber(s.pkg.Subrepo.FS, s.state.Config.Parse.BuildFileName)
+			s.globber = fs.NewGlobber(s.pkg.Subrepo.FS(), s.state.Config.Parse.BuildFileName)
 		} else {
 			s.globber = fs.NewGlobber(fs.HostFS, s.state.Config.Parse.BuildFileName)
 		}
@@ -1293,11 +1293,12 @@ func subrepo(s *scope, args []pyObject) pyObject {
 	if dep != "" {
 		// N.B. The target must be already registered on this package.
 		target = s.pkg.TargetOrDie(s.parseLabelInPackage(dep, s.pkg).Name)
+		root = target.Label.Name
 		if len(target.Outputs()) == 1 {
-			root = filepath.Join(target.OutDir(), target.Outputs()[0])
-		} else {
-			// TODO(jpoole): perhaps this should be a fatal error?
-			root = filepath.Join(target.OutDir(), name)
+			root = target.Outputs()[0]
+		}
+		if target.Local || s.state.RemoteClient == nil {
+			root = filepath.Join(target.OutDir(), root)
 		}
 	} else if args[PathArgIdx] != None {
 		root = string(args[PathArgIdx].(pyString))

src/parse/parse_step.go

@@ -67,7 +67,7 @@ func parse(state *core.BuildState, label, dependent core.BuildLabel, mode core.P
 
 	if subrepo != nil && subrepo.Target != nil {
 		// We have got the definition of the subrepo but it depends on something, make sure that has been built.
-		state.WaitForTargetAndEnsureDownload(subrepo.Target.Label, label, mode.IsPreload())
+		state.WaitForBuiltTarget(subrepo.Target.Label, label, mode|core.ParseModeForSubinclude)
 		if !subrepo.Target.State().IsBuilt() {
 			return fmt.Errorf("%v: failed to build subrepo", label)
 		}
@@ -167,7 +167,7 @@ func parsePackage(state *core.BuildState, label, dependent core.BuildLabel, subr
 	var fileSystem iofs.FS = fs.HostFS
 	if subrepo != nil {
 		pkg.SubrepoName = subrepo.Name
-		fileSystem = subrepo.FS
+		fileSystem = subrepo.FS()
 	}
 	// Only load the internal package for the host repo's state
 	if state.ParentState == nil && packageName == InternalPackageName {
@@ -186,6 +186,7 @@ func parsePackage(state *core.BuildState, label, dependent core.BuildLabel, subr
 				return nil, err
 			}
 			defer file.Close()
+
 			pkg.Filename = filename
 			if err := state.Parser.ParseReader(pkg, file, &label, &dependent, mode); err != nil {
 				return nil, err

src/please.go

@@ -554,7 +554,7 @@ var buildFunctions = map[string]func() int{
 		return 0
 	},
 	"exec.sequential": func() int {
-		annotated, unannotated, args := opts.Exec.Sequential.Args.Targets.Separate()
+		annotated, unannotated, args := opts.Exec.Sequential.Args.Targets.Separate(true)
 		if len(unannotated) == 0 {
 			return 0
 		}
@@ -568,7 +568,7 @@ var buildFunctions = map[string]func() int{
 		return 0
 	},
 	"exec.parallel": func() int {
-		annotated, unannotated, args := opts.Exec.Parallel.Args.Targets.Separate()
+		annotated, unannotated, args := opts.Exec.Parallel.Args.Targets.Separate(true)
 		if len(unannotated) == 0 {
 			return 0
 		}
@@ -602,7 +602,7 @@ var buildFunctions = map[string]func() int{
 		return 1 // We should never return from run.Run so if we make it here something's wrong.
 	},
 	"run.parallel": func() int {
-		annotated, unannotated, args := opts.Run.Parallel.PositionalArgs.Targets.Separate()
+		annotated, unannotated, args := opts.Run.Parallel.PositionalArgs.Targets.Separate(true)
 		if len(unannotated) == 0 {
 			return 0
 		}
@@ -619,7 +619,7 @@ var buildFunctions = map[string]func() int{
 		return 1
 	},
 	"run.sequential": func() int {
-		annotated, unannotated, args := opts.Run.Sequential.PositionalArgs.Targets.Separate()
+		annotated, unannotated, args := opts.Run.Sequential.PositionalArgs.Targets.Separate(true)
 		if len(unannotated) == 0 {
 			return 0
 		}
@@ -1224,7 +1224,12 @@ func (arg *TargetOrArg) UnmarshalFlag(value string) error {
 type TargetsOrArgs []TargetOrArg
 
 // Separate splits the targets & arguments into the labels (in both annotated & unannotated forms) and the arguments.
-func (l TargetsOrArgs) Separate() (annotated []core.AnnotatedOutputLabel, unannotated []core.BuildLabel, args []string) {
+func (l TargetsOrArgs) Separate(requireOneLabel bool) (annotated []core.AnnotatedOutputLabel, unannotated []core.BuildLabel, args []string) {
+	if requireOneLabel && l[0].arg != "" && l[0].arg != "-" {
+		if err := l[0].label.UnmarshalFlag(l[0].arg); err != nil {
+			log.Fatalf("First argument must be a build label: %s", l[0].arg)
+		}
+	}
 	for _, arg := range l {
 		if l, _ := arg.label.Label(); l.IsEmpty() {
 			if arg.arg == "-" {
@@ -1246,7 +1251,7 @@ func (l TargetsOrArgs) Separate() (annotated []core.AnnotatedOutputLabel, unanno
 
 // SeparateUnannotated splits the targets & arguments into two slices. Annotations aren't permitted.
 func (l TargetsOrArgs) SeparateUnannotated() ([]core.BuildLabel, []string) {
-	annotated, unannotated, args := l.Separate()
+	annotated, unannotated, args := l.Separate(false)
 	for _, a := range annotated {
 		if a.Annotation != "" {
 			log.Fatalf("Invalid build label; annotations are not permitted here: %s", a)

src/remote/action.go

@@ -24,6 +24,7 @@ import (
 	"github.com/thought-machine/please/src/core"
 	"github.com/thought-machine/please/src/fs"
 	"github.com/thought-machine/please/src/process"
+	remotefs "github.com/thought-machine/please/src/remote/fs"
 )
 
 // uploadAction uploads a build action for a target and returns its digest.
@@ -257,6 +258,30 @@ func (c *Client) uploadInputDir(ch chan<- *uploadinfo.Entry, target *core.BuildT
 			}
 			continue
 		}
+		if i, ok := input.(core.SubrepoFileLabel); ok && target.Subrepo.IsRemoteSubrepo() {
+			for _, p := range i.Paths(c.state.Graph) {
+				subrepoPath, err := filepath.Rel(target.Subrepo.PackageRoot, p)
+				if err != nil {
+					return nil, fmt.Errorf("%v: source file not in subrepo package root (%v): %v", target.Label, p, err)
+				}
+				fileNode, dirNode, symlinkNode, err := remotefs.FindNode(target.Subrepo.FS(), subrepoPath)
+				if err != nil {
+					return nil, fmt.Errorf("%v: failed to find file in subrepo output: %v", target.Label, err)
+				}
+
+				dir := b.Dir(filepath.Dir(p))
+				if fileNode != nil {
+					dir.Files = append(dir.Files, fileNode)
+				}
+				if dirNode != nil {
+					dir.Directories = append(dir.Directories, dirNode)
+				}
+				if symlinkNode != nil {
+					dir.Symlinks = append(dir.Symlinks, symlinkNode)
+				}
+			}
+			continue
+		}
 		if err := c.uploadInput(b, ch, input); err != nil {
 			return nil, err
 		}

src/remote/fs/fs.go

@@ -26,6 +26,14 @@ type Client interface {
 	ReadBlob(ctx context.Context, d digest.Digest) ([]byte, *client.MovedBytesMetadata, error)
 }
 
+func FindNode(fs iofs.FS, path string) (*pb.FileNode, *pb.DirectoryNode, *pb.SymlinkNode, error) {
+	casFS, ok := fs.(*CASFileSystem)
+	if !ok {
+		return nil, nil, nil, fmt.Errorf("not supported")
+	}
+	return casFS.FindNode(path)
+}
+
 // CASFileSystem is an fs.FS implemented on top of a Tree proto. This will download files as they are needed from the
 // CAS when they are opened.
 type CASFileSystem struct {

src/remote/remote.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/hex"
 	"fmt"
+	iofs "io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -37,6 +38,7 @@ import (
 	"github.com/thought-machine/please/src/core"
 	"github.com/thought-machine/please/src/fs"
 	"github.com/thought-machine/please/src/metrics"
+	remotefs "github.com/thought-machine/please/src/remote/fs"
 )
 
 var log = logging.Log
@@ -146,8 +148,8 @@ func New(state *core.BuildState) *Client {
 	c := &Client{
 		state:        state,
 		instance:     state.Config.Remote.Instance,
-		outputs:      map[core.BuildLabel]*pb.Directory{},
-		subrepoTrees: map[core.BuildLabel]*pb.Tree{},
+		outputs:      make(map[core.BuildLabel]*pb.Directory, 100),
+		subrepoTrees: make(map[core.BuildLabel]*pb.Tree, 10),
 		mdStore:      newDirMDStore(state.Config.Remote.Instance, time.Duration(state.Config.Remote.CacheDuration)),
 		existingBlobs: map[string]struct{}{
 			digest.Empty.Hash: {},
@@ -313,6 +315,14 @@ func (c *Client) digestEnum() pb.DigestFunction_Value {
 	}
 }
 
+func (c *Client) SubrepoFS(target *core.BuildTarget, root string) iofs.FS {
+	c.outputMutex.RLock()
+	defer c.outputMutex.RUnlock()
+
+	tree := c.subrepoTrees[target.Label]
+	return remotefs.New(c.client, tree, root)
+}
+
 // Build executes a remote build of the given target.
 func (c *Client) Build(target *core.BuildTarget) (*core.BuildMetadata, error) {
 	if err := c.CheckInitialised(); err != nil {