Pass context down to pre/post build functions

VERSION

@@ -1 +1 @@
-17.34.0-beta.5
+17.34.0-beta.6

src/build/build_step.go

@@ -3,6 +3,7 @@ package build
 
 import (
 	"bytes"
+	"context"
 	"encoding/hex"
 	"errors"
 	"fmt"
@@ -59,11 +60,11 @@ var successfulLocalTargetBuildDuration = metrics.NewHistogramVec(
 )
 
 // Build implements the core logic for building a single target.
-func Build(state *core.BuildState, target *core.BuildTarget, remote bool) error {
+func Build(ctx context.Context, state *core.BuildState, target *core.BuildTarget, remote bool) error {
 	state = state.ForTarget(target)
 	target.SetState(core.Building)
 	start := time.Now()
-	if err := buildTarget(state, target, remote); err != nil {
+	if err := buildTarget(ctx, state, target, remote); err != nil {
 		if errors.Is(err, errStop) {
 			target.SetState(core.Stopped)
 			state.LogBuildResult(target, core.TargetBuildStopped, "Build stopped")
@@ -159,7 +160,7 @@ func prepareOnly(state *core.BuildState, target *core.BuildTarget) error {
 //     b) attempt to fetch the outputs from the cache based on the output hash
 //  3. Actually build the rule
 //  4. Store result in the cache
-func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely bool) (err error) {
+func buildTarget(ctx context.Context, state *core.BuildState, target *core.BuildTarget, runRemotely bool) (err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			if e, ok := r.(error); ok {
@@ -180,7 +181,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 	// This must run before we can leave this function successfully by any path.
 	if target.PreBuildFunction != nil {
 		log.Debug("Running pre-build function for %s", target.Label)
-		if err := state.Parser.RunPreBuildFunction(state, target); err != nil {
+		if err := state.Parser.RunPreBuildFunction(ctx, state, target); err != nil {
 			return err
 		}
 		log.Debug("Finished pre-build function for %s", target.Label)
@@ -242,7 +243,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 				addOutDirOutsFromMetadata(target, metadata)
 
 				if target.PostBuildFunction != nil {
-					if err := runPostBuildFunction(state, target, string(metadata.Stdout), ""); err != nil {
+					if err := runPostBuildFunction(ctx, state, target, string(metadata.Stdout), ""); err != nil {
 						log.Warning("Error from post-build function for %s: %s; will rebuild", target.Label, err)
 					}
 				}
@@ -302,7 +303,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 					addOutDirOutsFromMetadata(target, metadata)
 					if target.PostBuildFunction != nil && !haveRunPostBuildFunction {
 						postBuildOutput = string(metadata.Stdout)
-						if err := runPostBuildFunction(state, target, postBuildOutput, ""); err != nil {
+						if err := runPostBuildFunction(ctx, state, target, postBuildOutput, ""); err != nil {
 							return err
 						}
 					}
@@ -344,7 +345,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 
 	if target.PostBuildFunction != nil {
 		outs := target.Outputs(state.Graph)
-		if err := runPostBuildFunction(state, target, string(metadata.Stdout), postBuildOutput); err != nil {
+		if err := runPostBuildFunction(ctx, state, target, string(metadata.Stdout), postBuildOutput); err != nil {
 			return err
 		}
 
@@ -996,7 +997,7 @@ func checkRuleHashesOfType(target *core.BuildTarget, hashes, outputs []string, h
 // In some cases it may have already run; if so we compare the previous output and warn
 // if the two differ (they must be deterministic to ensure it's a pure function, since there
 // are a few different paths through here and we guarantee to only run them once).
-func runPostBuildFunction(state *core.BuildState, target *core.BuildTarget, output, prevOutput string) error {
+func runPostBuildFunction(ctx context.Context, state *core.BuildState, target *core.BuildTarget, output, prevOutput string) error {
 	if prevOutput != "" {
 		if output != prevOutput {
 			log.Warning("The build output for %s differs from what we got back from the cache earlier.\n"+
@@ -1007,7 +1008,7 @@ func runPostBuildFunction(state *core.BuildState, target *core.BuildTarget, outp
 		}
 		return nil
 	}
-	return state.Parser.RunPostBuildFunction(state, target, output)
+	return state.Parser.RunPostBuildFunction(ctx, state, target, output)
 }
 
 // checkLicences checks the licences for the target match what we've accepted / rejected in the config

src/build/build_step_stress_test.go

@@ -123,12 +123,12 @@ func (fake *fakeParser) ParseReader(_ context.Context, pkg *core.Package, r io.R
 }
 
 // RunPreBuildFunction stub
-func (fake *fakeParser) RunPreBuildFunction(state *core.BuildState, target *core.BuildTarget) error {
+func (fake *fakeParser) RunPreBuildFunction(_ context.Context, state *core.BuildState, target *core.BuildTarget) error {
 	return nil
 }
 
 // RunPostBuildFunction stub
-func (fake *fakeParser) RunPostBuildFunction(state *core.BuildState, target *core.BuildTarget, output string) error {
+func (fake *fakeParser) RunPostBuildFunction(_ context.Context, state *core.BuildState, target *core.BuildTarget, output string) error {
 	if f, present := fake.PostBuildFunctions[target]; present {
 		return f(target, output)
 	}

src/build/build_step_test.go

@@ -32,15 +32,15 @@ var cache core.Cache
 func TestBuildTargetWithNoDeps(t *testing.T) {
 	state, target := newState("//package1:target1")
 	target.AddOutput("file1")
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 }
 
 func TestFailedBuildTarget(t *testing.T) {
 	state, target := newState("//package1:target1a")
 	target.Command = "false"
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.Error(t, err)
 }
 
@@ -49,7 +49,7 @@ func TestBuildTargetWhichNeedsRebuilding(t *testing.T) {
 	// because there's no rule hash file.
 	state, target := newState("//package1:target2")
 	target.AddOutput("file2")
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 }
@@ -60,7 +60,7 @@ func TestBuildTargetWhichDoesntNeedRebuilding(t *testing.T) {
 	target.AddOutput("file3")
 	StoreTargetMetadata(target, new(core.BuildMetadata))
 	assert.NoError(t, writeRuleHash(state, target))
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Reused, target.State())
 }
@@ -73,7 +73,7 @@ func TestModifiedBuildTargetStillNeedsRebuilding(t *testing.T) {
 	assert.NoError(t, writeRuleHash(state, target))
 	target.Command = "echo -n 'wibble wibble wibble' > $OUT"
 	target.RuleHash = nil // Have to force a reset of this
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 }
@@ -84,7 +84,7 @@ func TestSymlinkedOutputs(t *testing.T) {
 	target.AddOutput("file5")
 	target.AddSource(core.FileLabel{File: "src5", Package: "package1"})
 	target.Command = "ln -s $SRC $OUT"
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 }
@@ -98,7 +98,7 @@ func TestPreBuildFunction(t *testing.T) {
 		target.Command = "echo 'wibble wibble wibble' > $OUT"
 		return nil
 	})
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 }
@@ -112,7 +112,7 @@ func TestPostBuildFunction(t *testing.T) {
 		assert.Equal(t, "wibble wibble wibble", output)
 		return nil
 	})
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 	assert.Equal(t, []string{"file7"}, target.Outputs(state.Graph))
@@ -130,7 +130,7 @@ func TestOutputDir(t *testing.T) {
 
 	state, target := newTarget()
 
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.Equal(t, []string{"file7"}, target.Outputs(state.Graph))
 
@@ -142,7 +142,7 @@ func TestOutputDir(t *testing.T) {
 
 	// Run again to load the outputs from the metadata
 	state, target = newTarget()
-	err = buildTarget(state, target, false)
+	err = buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.Equal(t, []string{"file7"}, target.Outputs(state.Graph))
 	assert.Equal(t, core.Reused, target.State())
@@ -165,7 +165,7 @@ func TestOutputDirDoubleStar(t *testing.T) {
 
 	state, target := newTarget(false)
 
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.Equal(t, []string{"foo"}, target.Outputs(state.Graph))
 
@@ -181,7 +181,7 @@ func TestOutputDirDoubleStar(t *testing.T) {
 
 	state, target = newTarget(true)
 
-	err = buildTarget(state, target, false)
+	err = buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.Equal(t, []string{"foo/file7"}, target.Outputs(state.Graph))
 
@@ -196,7 +196,7 @@ func TestCacheRetrieval(t *testing.T) {
 	target.AddOutput("file8")
 	target.Command = "false" // Will fail if we try to build it.
 	state.Cache = cache
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Cached, target.State())
 }
@@ -214,7 +214,7 @@ func TestPostBuildFunctionAndCache(t *testing.T) {
 		return nil
 	})
 	state.Cache = cache
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Built, target.State())
 	assert.True(t, called)
@@ -234,7 +234,7 @@ func TestPostBuildFunctionAndCache2(t *testing.T) {
 		return nil
 	})
 	state.Cache = cache
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	assert.NoError(t, err)
 	assert.Equal(t, core.Cached, target.State())
 	assert.True(t, called)
@@ -277,7 +277,7 @@ func TestCreatePlzOutGo(t *testing.T) {
 	target.AddLabel("link:plz-out/go/${PKG}/src")
 	target.AddOutput("file1.go")
 	assert.False(t, fs.PathExists("plz-out/go"))
-	assert.NoError(t, buildTarget(state, target, false))
+	assert.NoError(t, buildTarget(t.Context(), state, target, false))
 	assert.True(t, fs.PathExists("plz-out/go/package1/src/file1.go"))
 }
 
@@ -436,7 +436,7 @@ func TestBuildMetadatafileIsCreated(t *testing.T) {
 
 	state, target := newState("//package1:mdtest")
 	target.AddOutput("file1")
-	err := buildTarget(state, target, false)
+	err := buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.False(t, target.BuildCouldModifyTarget())
 	assert.True(t, fs.FileExists(filepath.Join(target.OutDir(), target.TargetBuildMetadataFileName())))
@@ -448,7 +448,7 @@ func TestBuildMetadatafileIsCreated(t *testing.T) {
 		assert.Equal(t, stdOut, output)
 		return nil
 	})
-	err = buildTarget(state, target, false)
+	err = buildTarget(t.Context(), state, target, false)
 	require.NoError(t, err)
 	assert.True(t, target.BuildCouldModifyTarget())
 	assert.True(t, fs.FileExists(filepath.Join(target.OutDir(), target.TargetBuildMetadataFileName())))
@@ -634,12 +634,12 @@ func (fake *fakeParser) ParseReader(_ context.Context, pkg *core.Package, r io.R
 }
 
 // RunPreBuildFunction stub
-func (fake *fakeParser) RunPreBuildFunction(state *core.BuildState, target *core.BuildTarget) error {
+func (fake *fakeParser) RunPreBuildFunction(_ context.Context, state *core.BuildState, target *core.BuildTarget) error {
 	return target.PreBuildFunction.Call(target)
 }
 
 // RunPostBuildFunction stub
-func (fake *fakeParser) RunPostBuildFunction(state *core.BuildState, target *core.BuildTarget, output string) error {
+func (fake *fakeParser) RunPostBuildFunction(_ context.Context, state *core.BuildState, target *core.BuildTarget, output string) error {
 	return target.PostBuildFunction.Call(target, output)
 }
 

src/core/state.go

@@ -72,9 +72,9 @@ type Parser interface {
 	// ParseReader parses a single BUILD file into the given package.
 	ParseReader(ctx context.Context, pkg *Package, reader io.ReadSeeker, forLabel, dependent *BuildLabel) error
 	// RunPreBuildFunction runs a pre-build function for a target.
-	RunPreBuildFunction(state *BuildState, target *BuildTarget) error
+	RunPreBuildFunction(ctx context.Context, state *BuildState, target *BuildTarget) error
 	// RunPostBuildFunction runs a post-build function for a target.
-	RunPostBuildFunction(state *BuildState, target *BuildTarget, output string) error
+	RunPostBuildFunction(ctx context.Context, state *BuildState, target *BuildTarget, output string) error
 }
 
 // A RemoteClient is the interface to a remote execution service.

src/exec/exec_test.go

@@ -34,7 +34,7 @@ func TestPrepareRuntimeDir(t *testing.T) {
 	if err := build.StoreTargetMetadata(target, &core.BuildMetadata{}); err != nil {
 		panic(err)
 	}
-	build.Build(state, target, false)
+	build.Build(t.Context(), state, target, false)
 
 	err := core.PrepareRuntimeDir(state, target, "plz-out/exec/pkg")
 	assert.Nil(t, err)

src/parse/init.go

@@ -77,28 +77,27 @@ func (p *aspParser) ParseReader(ctx context.Context, pkg *core.Package, reader i
 	return err
 }
 
-func (p *aspParser) RunPreBuildFunction(state *core.BuildState, target *core.BuildTarget) error {
-	return p.runBuildFunction(state, target, "pre", func() error {
+func (p *aspParser) RunPreBuildFunction(ctx context.Context, state *core.BuildState, target *core.BuildTarget) error {
+	return p.runBuildFunction(ctx, state, target, "pre", func() error {
 		return target.PreBuildFunction.Call(target)
 	})
 }
 
-func (p *aspParser) RunPostBuildFunction(state *core.BuildState, target *core.BuildTarget, output string) error {
-	return p.runBuildFunction(state, target, "post", func() error {
+func (p *aspParser) RunPostBuildFunction(ctx context.Context, state *core.BuildState, target *core.BuildTarget, output string) error {
+	return p.runBuildFunction(ctx, state, target, "post", func() error {
 		log.Debug("Running post-build function for %s. Build output:\n%s", target.Label, output)
 		return target.PostBuildFunction.Call(target, output)
 	})
 }
 
 // runBuildFunction runs either the pre- or post-build function.
-func (p *aspParser) runBuildFunction(state *core.BuildState, target *core.BuildTarget, callbackType string, f func() error) error {
+func (p *aspParser) runBuildFunction(ctx context.Context, state *core.BuildState, target *core.BuildTarget, callbackType string, f func() error) error {
 	state.LogBuildResult(target, core.PackageParsing, fmt.Sprintf("Running %s-build function for %s", callbackType, target.Label))
 	// This doesn't re-parse anything; it waits for the parse of this target's package to complete if
 	// one is still in flight. Targets are added to the graph as each rule is created, so a target can
 	// be picked up and built before the file that defines it has been fully interpreted - but the
 	// callback both reads the package out of the graph and mutates it, so it can't run until then.
-	// There's no parse in flight for us to inherit a context from, hence Background.
-	if _, err := p.callbacks.Parse(context.Background(), target.Label, target.Label); err != nil {
+	if _, err := p.callbacks.Parse(ctx, target.Label, target.Label); err != nil {
 		return err
 	}
 	if err := f(); err != nil {

src/plz/plz.go

@@ -460,6 +460,9 @@ func (r *runner) recursiveParsePackage(ctx context.Context, label, dependent cor
 func (r *runner) parseTarget(ctx context.Context, label, dependent core.BuildLabel) (*core.BuildTarget, error) {
 	target, wait := r.state.Graph.TargetOrWait(label)
 	if target != nil {
+		if err := r.ensurePreloads(ctx, r.state.ForTarget(target)); err != nil {
+			return nil, err
+		}
 		return target, nil
 	}
 	pkg, err := r.parse(ctx, label, dependent, false, wait)
@@ -612,7 +615,7 @@ func (r *runner) runBuildAction(ctx context.Context, target *core.BuildTarget) e
 	if err := ctx.Err(); err != nil {
 		return err
 	}
-	return build.Build(r.state, target, remote)
+	return build.Build(ctx, r.state, target, remote)
 }
 
 // buildAll builds all the targets specified by the given label (which can be :all, but can't be ...).

test/preload_pre_build/BUILD

@@ -0,0 +1,13 @@
+subinclude("//test/build_defs")
+
+# A preloaded subinclude that needs a target with a pre-build function. Running that function parses
+# a package in the same repo, which must not wait on the preloads it's part of resolving.
+please_repo_e2e_test(
+    name = "preload_pre_build_test",
+    expected_output = {
+        "out.txt": "//src:src",
+    },
+    plz_command = "plz build //src:src && plz query alltargets //src:all > out.txt",
+    repo = "test_repo",
+    timeout = 120,
+)

test/preload_pre_build/test_repo/.plzconfig

@@ -0,0 +1,3 @@
+[parse]
+BuildFileName = BUILD
+PreloadSubincludes = //build_defs:defs

test/preload_pre_build/test_repo/build_defs/BUILD_FILE

@@ -0,0 +1,16 @@
+def _pre_build(name):
+    pass
+
+genrule(
+    name = "gen",
+    outs = ["gen.build_defs"],
+    cmd = "echo 'def gen():\n    pass' > $OUT",
+    pre_build = _pre_build,
+    visibility = ["PUBLIC"],
+)
+
+filegroup(
+    name = "defs",
+    srcs = ["defs.build_defs"],
+    visibility = ["PUBLIC"],
+)

test/preload_pre_build/test_repo/build_defs/defs.build_defs

@@ -0,0 +1,8 @@
+subinclude("//build_defs:gen")
+
+def defs(name):
+    gen()
+    return filegroup(
+        name = name,
+        srcs = [name + ".txt"],
+    )