Support preloads in subincludes (#2859)

* Support preloads in subincludes

* fix interpreter test

* Handle transitive preloads

* WIP: handle transitive subincludes in preloads

* Fix scope mode when evaluating call expressions

* Fix tests

* Lint

* Handle errors

* version

* ChangeLog version typo

---------

Co-authored-by: rgodden <[email protected]>
Co-authored-by: Peter Ebden <[email protected]>
Co-authored-by: Sam Westmoreland <[email protected]>

ChangeLog

@@ -1,3 +1,7 @@
+Version 17.2.1
+---------------
+    * Better support preloaded subincludes that themselves subinclude (#2859)
+
 Version 17.2.0
 ---------------
     * Added hostinfo prometheus counter (#2835)

VERSION

@@ -1 +1 @@
-17.2.0
+17.2.1

src/build/build_step_stress_test.go

@@ -99,6 +99,10 @@ type fakeParser struct {
 	PostBuildFunctions buildFunctionMap
 }
 
+func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
+	return nil
+}
+
 // ParseFile stub
 func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
 	return nil

src/build/build_step_test.go

@@ -606,6 +606,10 @@ func (*mockCache) Shutdown()                      {}
 type fakeParser struct {
 }
 
+func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
+	return nil
+}
+
 // ParseFile stub
 func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
 	return nil

src/core/state.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/cespare/xxhash/v2"
 	"github.com/zeebo/blake3"
+	"golang.org/x/sync/errgroup"
 
 	"github.com/thought-machine/please/src/cli"
 	"github.com/thought-machine/please/src/cmap"
@@ -90,6 +91,7 @@ type Parser interface {
 	RunPostBuildFunction(state *BuildState, target *BuildTarget, output string) error
 	// BuildRuleArgOrder returns a map of the arguments to build rule and the order they appear in the source file
 	BuildRuleArgOrder() map[string]int
+	RegisterPreload(label BuildLabel) error
 }
 
 // A RemoteClient is the interface to a remote execution service.
@@ -634,28 +636,30 @@ func (state *BuildState) forwardResults() {
 	}
 }
 
-// WaitForPreloadedSubincludeTargetsAndEnsureDownloaded waits for all preloaded subinclude targets to be built, and
-// downloads them.
-func (state *BuildState) WaitForPreloadedSubincludeTargetsAndEnsureDownloaded() {
+// RegisterPreloads waits for all preloaded subinclude targets to be built, downloads them, and then registers them with
+// the interpreter. We have to actually register them otherwise this will return before we build any
+// transitive subincludes.
+func (state *BuildState) RegisterPreloads() error {
+	var err error
 	state.preloadDownloadOnce.Do(func() {
-		wg := sync.WaitGroup{}
+		var eg errgroup.Group
 		for _, inc := range state.GetPreloadedSubincludes() {
 			if inc.IsPseudoTarget() {
 				log.Fatalf("Can't preload pseudotarget %v", inc)
 			}
 
 			// Queue them up asynchronously to feed the queues as quickly as possible
-			wg.Add(1)
-			go func(inc BuildLabel) {
+			inc := inc
+			eg.Go(func() error {
 				state.WaitForTargetAndEnsureDownload(inc, OriginalTarget, true)
-				wg.Done()
-			}(inc)
+				return state.Parser.RegisterPreload(inc)
+			})
 		}
-
 		// We must wait for all the subinclude targets to be built otherwise updating the locals might race with parsing
 		// a package
-		wg.Wait()
+		err = eg.Wait()
 	})
+	return err
 }
 
 // checkForCycles is run to detect a cycle in the graph. It converts any returned error into an async error.
@@ -821,7 +825,7 @@ func (state *BuildState) SyncParsePackage(label BuildLabel) *Package {
 
 // WaitForPackage is similar to WaitForBuiltTarget however it waits for the package to be parsed, queuing it for parse
 // if necessary
-func (state *BuildState) WaitForPackage(l, dependent BuildLabel) *Package {
+func (state *BuildState) WaitForPackage(l, dependent BuildLabel, mode ParseMode) *Package {
 	if p := state.Graph.PackageByLabel(l); p != nil {
 		return p
 	}
@@ -840,10 +844,10 @@ func (state *BuildState) WaitForPackage(l, dependent BuildLabel) *Package {
 	}
 
 	// Otherwise queue the target for parse and recurse
-	state.addPendingParse(l, dependent, ParseModeForSubinclude)
+	state.addPendingParse(l, dependent, mode)
 	state.progress.packageWaits.Set(key, make(chan struct{}))
 
-	return state.WaitForPackage(l, dependent)
+	return state.WaitForPackage(l, dependent, mode)
 }
 
 // WaitForBuiltTarget blocks until the given label is available as a build target and has been successfully built.
@@ -1030,9 +1034,6 @@ func (state *BuildState) QueueTarget(label, dependent BuildLabel, forceBuild boo
 }
 
 func (state *BuildState) queueTarget(label, dependent BuildLabel, forceBuild bool, mode ParseMode) error {
-	if label.Name == "arcat" {
-		log.Debug("")
-	}
 	target := state.Graph.Target(label)
 	if target == nil {
 		// If the package isn't loaded yet, we need to queue a parse for it.

src/parse/asp/builtins.go

@@ -197,7 +197,7 @@ func buildRule(s *scope, args []pyObject) pyObject {
 	}
 
 	if s.parsingFor != nil && s.parsingFor.label == target.Label {
-		if err := s.state.ActivateTarget(s.pkg, s.parsingFor.label, s.parsingFor.dependent, s.parsingFor.mode); err != nil {
+		if err := s.state.ActivateTarget(s.pkg, s.parsingFor.label, s.parsingFor.dependent, s.mode); err != nil {
 			s.Error("%v", err)
 		}
 	}
@@ -284,7 +284,7 @@ func bazelLoad(s *scope, args []pyObject) pyObject {
 		}
 		filename = subrepo.Dir(filename)
 	}
-	s.SetAll(s.interpreter.Subinclude(s, filename, l), false)
+	s.SetAll(s.interpreter.Subinclude(s, filename, l, false), false)
 	return None
 }
 
@@ -295,7 +295,7 @@ func (s *scope) WaitForSubincludedTarget(l, dependent core.BuildLabel) *core.Bui
 	s.interpreter.limiter.Release()
 	defer s.interpreter.limiter.Acquire()
 
-	return s.state.WaitForTargetAndEnsureDownload(l, dependent, false)
+	return s.state.WaitForTargetAndEnsureDownload(l, dependent, s.mode.IsPreload())
 }
 
 // builtinFail raises an immediate error that can't be intercepted.
@@ -320,7 +320,7 @@ func subinclude(s *scope, args []pyObject) pyObject {
 		s.interpreter.loadPluginConfig(s, incPkgState)
 
 		for _, out := range t.Outputs() {
-			s.SetAll(s.interpreter.Subinclude(s, filepath.Join(t.OutDir(), out), t.Label), false)
+			s.SetAll(s.interpreter.Subinclude(s, filepath.Join(t.OutDir(), out), t.Label, false), false)
 		}
 	}
 	return None
@@ -348,7 +348,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget {
 			Subrepo:     subrepoLabel.Subrepo,
 			Name:        "all",
 		}
-		s.state.WaitForPackage(subrepoPackageLabel, pkgLabel)
+		s.state.WaitForPackage(subrepoPackageLabel, pkgLabel, s.mode|core.ParseModeForSubinclude)
 	}
 
 	// isLocal is true when this subinclude target in the current package being parsed
@@ -363,11 +363,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget {
 			s.Error("Target :%s is not defined in this package; it has to be defined before the subinclude() call", l.Name)
 		}
 		if t.State() < core.Active {
-			mode := core.ParseModeForSubinclude
-			if s.parsingFor != nil {
-				mode = s.parsingFor.mode // Propagate whether this is a preload or not
-			}
-			if err := s.state.ActivateTarget(s.pkg, l, pkgLabel, mode); err != nil {
+			if err := s.state.ActivateTarget(s.pkg, l, pkgLabel, s.mode|core.ParseModeForSubinclude); err != nil {
 				s.Error("Failed to activate subinclude target: %v", err)
 			}
 		}

src/parse/asp/interpreter.go

@@ -78,7 +78,7 @@ func (i *interpreter) getConfig(state *core.BuildState) *pyConfig {
 
 // LoadBuiltins loads a set of builtins from a file, optionally with its contents.
 func (i *interpreter) LoadBuiltins(filename string, contents []byte, statements []*Statement) error {
-	s := i.scope.NewScope(filename)
+	s := i.scope.NewScope(filename, 0)
 	// Gentle hack - attach the native code once we have loaded the correct file.
 	// Needs to be after this file is loaded but before any of the others that will
 	// use functions from it.
@@ -117,35 +117,42 @@ func (i *interpreter) loadBuiltinStatements(s *scope, statements []*Statement, e
 	return err
 }
 
-func (i *interpreter) preloadSubincludes(s *scope) (err error) {
+func (i *interpreter) preloadSubincludes(s *scope) error {
+	// We should have ensured these targets are downloaded by this point in `parse_step.go`
+	for _, label := range s.state.GetPreloadedSubincludes() {
+		if err := i.preloadSubinclude(s, label); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (i *interpreter) preloadSubinclude(s *scope, label core.BuildLabel) (err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			err = handleErrors(r)
 		}
 	}()
 
-	// We should have ensured these targets are downloaded by this point in `parse_step.go`
-	for _, label := range s.state.GetPreloadedSubincludes() {
-		t := s.state.Graph.TargetOrDie(label)
+	t := s.state.Graph.TargetOrDie(label)
 
-		includeState := s.state
-		if t.Label.Subrepo != "" {
-			subrepo := s.state.Graph.SubrepoOrDie(t.Label.Subrepo)
-			includeState = subrepo.State
-		}
+	includeState := s.state
+	if t.Label.Subrepo != "" {
+		subrepo := s.state.Graph.SubrepoOrDie(t.Label.Subrepo)
+		includeState = subrepo.State
+	}
 
-		s.interpreter.loadPluginConfig(s, includeState)
-		for _, out := range t.FullOutputs() {
-			s.SetAll(s.interpreter.Subinclude(s, out, t.Label), false)
-		}
+	s.interpreter.loadPluginConfig(s, includeState)
+	for _, out := range t.FullOutputs() {
+		s.SetAll(s.interpreter.Subinclude(s, out, t.Label, true), false)
 	}
-	return
+	return nil
 }
 
 // interpretAll runs a series of statements in the scope of the given package.
 // The first return value is for testing only.
 func (i *interpreter) interpretAll(pkg *core.Package, forLabel, dependent *core.BuildLabel, mode core.ParseMode, statements []*Statement) (*scope, error) {
-	s := i.scope.NewPackagedScope(pkg, 1)
+	s := i.scope.NewPackagedScope(pkg, mode, 1)
 	s.config = i.getConfig(s.state).Copy()
 
 	// Config needs a little separate tweaking.
@@ -155,7 +162,6 @@ func (i *interpreter) interpretAll(pkg *core.Package, forLabel, dependent *core.
 		s.parsingFor = &parseTarget{
 			label:     *forLabel,
 			dependent: *dependent,
-			mode:      mode,
 		}
 	}
 
@@ -194,7 +200,7 @@ func (i *interpreter) interpretStatements(s *scope, statements []*Statement) (re
 }
 
 // Subinclude returns the global values corresponding to subincluding the given file.
-func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildLabel) pyDict {
+func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildLabel, preload bool) pyDict {
 	key := filepath.Join(path, pkgScope.state.CurrentSubrepo)
 	globals, wait, first := i.subincludes.GetOrWait(key)
 	if globals != nil {
@@ -205,19 +211,31 @@ func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildL
 		<-wait
 		return i.subincludes.Get(key)
 	}
+
 	// If we get here, it falls to us to parse this.
 	stmts, err := i.parser.parse(path)
 	if err != nil {
 		panic(err) // We're already inside another interpreter, which will handle this for us.
 	}
 	stmts = i.parser.optimise(stmts)
-	s := i.scope.NewScope(path)
+	mode := pkgScope.mode
+	if preload {
+		mode |= core.ParseModeForPreload
+	}
+	s := i.scope.NewScope(path, mode)
+
 	s.state = pkgScope.state
 	// Scope needs a local version of CONFIG
 	s.config = i.scope.config.Copy()
-	s.subincludeLabel = &label
 	s.Set("CONFIG", s.config)
+	s.subincludeLabel = &label
 	i.optimiseExpressions(stmts)
+
+	if !mode.IsPreload() {
+		if err := i.preloadSubincludes(s); err != nil {
+			s.Error("failed: %v", err)
+		}
+	}
 	s.interpretStatements(stmts)
 	locals := s.Freeze()
 	if s.config.overlay == nil {
@@ -253,7 +271,6 @@ func (i *interpreter) optimiseExpressions(stmts []*Statement) {
 type parseTarget struct {
 	label     core.BuildLabel
 	dependent core.BuildLabel
-	mode      core.ParseMode
 }
 
 // A scope contains all the information about a lexical scope.
@@ -270,6 +287,7 @@ type scope struct {
 	globber         *fs.Globber
 	// True if this scope is for a pre- or post-build callback.
 	Callback bool
+	mode     core.ParseMode
 }
 
 // parseAnnotatedLabelInPackage similarly to parseLabelInPackage, parses the label contextualising it to the provided
@@ -340,17 +358,17 @@ func (s *scope) subincludePackage() *core.Package {
 }
 
 // NewScope creates a new child scope of this one.
-func (s *scope) NewScope(filename string) *scope {
-	return s.newScope(s.pkg, filename, 0)
+func (s *scope) NewScope(filename string, mode core.ParseMode) *scope {
+	return s.newScope(s.pkg, mode, filename, 0)
 }
 
 // NewPackagedScope creates a new child scope of this one pointing to the given package.
 // hint is a size hint for the new set of locals.
-func (s *scope) NewPackagedScope(pkg *core.Package, hint int) *scope {
-	return s.newScope(pkg, pkg.Filename, hint)
+func (s *scope) NewPackagedScope(pkg *core.Package, mode core.ParseMode, hint int) *scope {
+	return s.newScope(pkg, mode, pkg.Filename, hint)
 }
 
-func (s *scope) newScope(pkg *core.Package, filename string, hint int) *scope {
+func (s *scope) newScope(pkg *core.Package, mode core.ParseMode, filename string, hint int) *scope {
 	s2 := &scope{
 		filename:    filename,
 		interpreter: s.interpreter,
@@ -361,6 +379,7 @@ func (s *scope) newScope(pkg *core.Package, filename string, hint int) *scope {
 		locals:      make(pyDict, hint),
 		config:      s.config,
 		Callback:    s.Callback,
+		mode:        mode,
 	}
 	if pkg != nil && pkg.Subrepo != nil && pkg.Subrepo.State != nil {
 		s2.state = pkg.Subrepo.State
@@ -777,7 +796,7 @@ func (s *scope) interpretList(expr *List) pyList {
 	if expr.Comprehension == nil {
 		return pyList(s.evaluateExpressions(expr.Values))
 	}
-	cs := s.NewScope(s.filename)
+	cs := s.NewScope(s.filename, s.mode)
 	l := s.iterate(expr.Comprehension.Expr)
 	ret := make(pyList, 0, len(l))
 	cs.evaluateComprehension(l, expr.Comprehension, func(li pyObject) {
@@ -798,7 +817,7 @@ func (s *scope) interpretDict(expr *Dict) pyObject {
 		}
 		return d
 	}
-	cs := s.NewScope(s.filename)
+	cs := s.NewScope(s.filename, s.mode)
 	l := cs.iterate(expr.Comprehension.Expr)
 	ret := make(pyDict, len(l))
 	cs.evaluateComprehension(l, expr.Comprehension, func(li pyObject) {

src/parse/asp/interpreter_test.go

@@ -294,7 +294,7 @@ func TestInterpreterFStrings(t *testing.T) {
 func TestInterpreterSubincludeConfig(t *testing.T) {
 	s, err := parseFile("src/parse/asp/test_data/interpreter/partition.build")
 	assert.NoError(t, err)
-	s.SetAll(s.interpreter.Subinclude(s, "src/parse/asp/test_data/interpreter/subinclude_config.build", core.NewPackage("test").Label()), false)
+	s.SetAll(s.interpreter.Subinclude(s, "src/parse/asp/test_data/interpreter/subinclude_config.build", core.NewPackage("test").Label(), false), false)
 	assert.EqualValues(t, "test test", s.config.Get("test", None))
 }
 
@@ -482,7 +482,7 @@ func TestJSON(t *testing.T) {
 	statements = parser.optimise(statements)
 	parser.interpreter.optimiseExpressions(statements)
 
-	s := parser.interpreter.scope.NewScope("BUILD")
+	s := parser.interpreter.scope.NewScope("BUILD", core.ParseModeNormal)
 
 	list := pyList{pyString("foo"), pyInt(5)}
 	dict := pyDict{"foo": pyString("bar")}
@@ -550,7 +550,7 @@ func TestLogConfigVariable(t *testing.T) {
 	confBase := &pyConfigBase{dict: dict}
 	config := &pyConfig{base: confBase, overlay: pyDict{"baz": pyInt(6)}}
 
-	s := parser.interpreter.scope.NewScope("BUILD")
+	s := parser.interpreter.scope.NewScope("BUILD", core.ParseModeNormal)
 	s.config = config
 	s.Set("CONFIG", config)