Skip to content

Commit

Permalink
Allow 'default' entry in provide map to decide what happens when a re…
Browse files Browse the repository at this point in the history
…quire doesn't match
  • Loading branch information
Tatskaari committed Apr 24, 2024
1 parent 0a9031b commit 3269527
Show file tree
Hide file tree
Showing 16 changed files with 116 additions and 80 deletions.
8 changes: 4 additions & 4 deletions src/build/filegroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ func buildFilegroup(state *core.BuildState, target *core.BuildTarget) (bool, err
}
changed := false
outDir := target.OutDir()
localSources := target.AllSourceLocalPaths(state.Graph)
for i, source := range target.AllSourceFullPaths(state.Graph) {
localSources := target.AllSourceLocalPaths(state.Graph, state.Config.FeatureFlags.FFDefaultProvides)
for i, source := range target.AllSourceFullPaths(state.Graph, state.Config.FeatureFlags.FFDefaultProvides) {
out := filepath.Join(outDir, localSources[i])
fileChanged, err := theFilegroupBuilder.Build(state, target, source, out)
if err != nil {
Expand Down Expand Up @@ -154,8 +154,8 @@ func buildFilegroup(state *core.BuildState, target *core.BuildTarget) (bool, err
// This is a small optimisation to ensure we don't need to recalculate them unnecessarily.
func copyFilegroupHashes(state *core.BuildState, target *core.BuildTarget) {
outDir := target.OutDir()
localSources := target.AllSourceLocalPaths(state.Graph)
for i, source := range target.AllSourceFullPaths(state.Graph) {
localSources := target.AllSourceLocalPaths(state.Graph, state.Config.FeatureFlags.FFDefaultProvides)
for i, source := range target.AllSourceFullPaths(state.Graph, state.Config.FeatureFlags.FFDefaultProvides) {
if out := filepath.Join(outDir, localSources[i]); out != source {
state.PathHasher.CopyHash(source, out)
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/build_env.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TargetEnvironment(state *BuildState, target *BuildTarget) BuildEnv {
// We read this as being slightly more POSIX-compliant than not having it set at all...
func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) BuildEnv {
env := TargetEnvironment(state, target)
sources := target.AllSourcePaths(state.Graph)
sources := target.AllSourcePaths(state.Graph, state.Config.FeatureFlags.FFDefaultProvides)
outEnv := target.GetTmpOutputAll(target.Outputs())
abs := filepath.IsAbs(tmpDir)

Expand All @@ -92,7 +92,7 @@ func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) Bui
}
// Named source groups if the target declared any.
for name, srcs := range target.NamedSources {
paths := target.SourcePaths(state.Graph, srcs)
paths := target.SourcePaths(state.Graph, srcs, state.Config.FeatureFlags.FFDefaultProvides)
// TODO(macripps): Quote these to prevent spaces from breaking everything (consider joining with NUL or sth?)
env = append(env, "SRCS_"+strings.ToUpper(name)+"="+strings.Join(paths, " "))
}
Expand Down
52 changes: 31 additions & 21 deletions src/core/build_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,28 +491,28 @@ func (target *BuildTarget) StartTestSuite() {
}

// AllSourcePaths returns all the source paths for this target
func (target *BuildTarget) AllSourcePaths(graph *BuildGraph) []string {
return target.allSourcePaths(graph, BuildInput.Paths)
func (target *BuildTarget) AllSourcePaths(graph *BuildGraph, ffDefaultProvide bool) []string {
return target.allSourcePaths(graph, BuildInput.Paths, ffDefaultProvide)
}

// AllSourceFullPaths returns all the source paths for this target, with a leading
// plz-out/gen etc if appropriate.
func (target *BuildTarget) AllSourceFullPaths(graph *BuildGraph) []string {
return target.allSourcePaths(graph, BuildInput.FullPaths)
func (target *BuildTarget) AllSourceFullPaths(graph *BuildGraph, ffDefaultProvide bool) []string {
return target.allSourcePaths(graph, BuildInput.FullPaths, ffDefaultProvide)
}

// AllSourceLocalPaths returns the local part of all the source paths for this target,
// i.e. without this target's package in it.
func (target *BuildTarget) AllSourceLocalPaths(graph *BuildGraph) []string {
return target.allSourcePaths(graph, BuildInput.LocalPaths)
func (target *BuildTarget) AllSourceLocalPaths(graph *BuildGraph, ffDefaultProvide bool) []string {
return target.allSourcePaths(graph, BuildInput.LocalPaths, ffDefaultProvide)
}

type buildPathsFunc func(BuildInput, *BuildGraph) []string

func (target *BuildTarget) allSourcePaths(graph *BuildGraph, full buildPathsFunc) []string {
func (target *BuildTarget) allSourcePaths(graph *BuildGraph, full buildPathsFunc, ffDefaultProvide bool) []string {
ret := make([]string, 0, len(target.Sources))
for _, source := range target.AllSources() {
ret = append(ret, target.sourcePaths(graph, source, full)...)
ret = append(ret, target.sourcePaths(graph, source, full, ffDefaultProvide)...)
}
return ret
}
Expand All @@ -533,7 +533,7 @@ func (target *BuildTarget) AllURLs(state *BuildState) []string {
// TODO(peterebden,tatskaari): Work out if we really want to have this and how the suite of *Dependencies functions
//
// below should behave (preferably nicely).
func (target *BuildTarget) resolveDependencies(graph *BuildGraph, callback func(*BuildTarget) error) error {
func (target *BuildTarget) resolveDependencies(graph *BuildGraph, callback func(*BuildTarget) error, ffDefaultProvide bool) error {
var g errgroup.Group
target.mutex.RLock()
for i := range target.dependencies {
Expand All @@ -542,7 +542,7 @@ func (target *BuildTarget) resolveDependencies(graph *BuildGraph, callback func(
continue // already done
}
g.Go(func() error {
if err := target.resolveOneDependency(graph, dep); err != nil {
if err := target.resolveOneDependency(graph, dep, ffDefaultProvide); err != nil {
return err
}
for _, d := range dep.deps {
Expand All @@ -557,14 +557,15 @@ func (target *BuildTarget) resolveDependencies(graph *BuildGraph, callback func(
return g.Wait()
}

func (target *BuildTarget) resolveOneDependency(graph *BuildGraph, dep *depInfo) error {
func (target *BuildTarget) resolveOneDependency(graph *BuildGraph, dep *depInfo, ffDefaultProvide bool) error {
t := graph.WaitForTarget(*dep.declared)
if t == nil {
return fmt.Errorf("Couldn't find dependency %s", dep.declared)
}
dep.declared = &t.Label // saves memory by not storing the label twice once resolved

labels := t.provideFor(target)
// TODO(jpoole): shouldn't this use the ProvideFor wrapper that handles resolving to self if there's no match?
labels := t.provideFor(target, ffDefaultProvide)

if len(labels) == 0 {
target.mutex.Lock()
Expand Down Expand Up @@ -595,7 +596,7 @@ func (target *BuildTarget) resolveOneDependency(graph *BuildGraph, dep *depInfo)
// MustResolveDependencies is exposed only for testing purposes.
// TODO(peterebden, tatskaari): See if we can get rid of this.
func (target *BuildTarget) ResolveDependencies(graph *BuildGraph) error {
return target.resolveDependencies(graph, func(*BuildTarget) error { return nil })
return target.resolveDependencies(graph, func(*BuildTarget) error { return nil }, false)
}

// DeclaredDependencies returns all the targets this target declared any kind of dependency on (including sources and tools).
Expand Down Expand Up @@ -880,19 +881,19 @@ func (target *BuildTarget) GetRealOutput(output string) string {
}

// SourcePaths returns the source paths for a given set of sources.
func (target *BuildTarget) SourcePaths(graph *BuildGraph, sources []BuildInput) []string {
func (target *BuildTarget) SourcePaths(graph *BuildGraph, sources []BuildInput, ffDefaultProvide bool) []string {
ret := make([]string, 0, len(sources))
for _, source := range sources {
ret = append(ret, target.sourcePaths(graph, source, BuildInput.Paths)...)
ret = append(ret, target.sourcePaths(graph, source, BuildInput.Paths, ffDefaultProvide)...)
}
return ret
}

// sourcePaths returns the source paths for a single source.
func (target *BuildTarget) sourcePaths(graph *BuildGraph, source BuildInput, f buildPathsFunc) []string {
func (target *BuildTarget) sourcePaths(graph *BuildGraph, source BuildInput, f buildPathsFunc, ffDefaultProvide bool) []string {
if label, ok := source.nonOutputLabel(); ok {
ret := []string{}
for _, providedLabel := range graph.TargetOrDie(label).ProvideFor(target) {
for _, providedLabel := range graph.TargetOrDie(label).ProvideFor(target, ffDefaultProvide) {
ret = append(ret, f(providedLabel, graph)...)
}
return ret
Expand Down Expand Up @@ -1195,20 +1196,25 @@ func (target *BuildTarget) AddProvide(language string, label BuildLabel) {
}

// ProvideFor returns the build label that we'd provide for the given target.
func (target *BuildTarget) ProvideFor(other *BuildTarget) []BuildLabel {
if p := target.provideFor(other); len(p) > 0 {
func (target *BuildTarget) ProvideFor(other *BuildTarget, ffDefaultProvide bool) []BuildLabel {
if p := target.provideFor(other, ffDefaultProvide); len(p) > 0 {
return p
}
return []BuildLabel{target.Label}
}

// provideFor is like ProvideFor but returns an empty slice if there is a direct dependency.
// It's a small optimisation to save allocating extra slices.
func (target *BuildTarget) provideFor(other *BuildTarget) []BuildLabel {
func (target *BuildTarget) provideFor(other *BuildTarget, ffDefaultProvide bool) []BuildLabel {
target.mutex.RLock()
defer target.mutex.RUnlock()
var defaultValue []BuildLabel
if def, ok := target.Provides["default"]; ok && ffDefaultProvide {
defaultValue = []BuildLabel{def}
}

if target.Provides == nil || len(other.Requires) == 0 {
return nil
return defaultValue
}
// Never do this if the other target has a data or tool dependency on us.
for _, data := range other.Data {
Expand All @@ -1228,6 +1234,10 @@ func (target *BuildTarget) provideFor(other *BuildTarget) []BuildLabel {
ret = append(ret, label)
}
}

if len(ret) == 0 {
return defaultValue
}
return ret
}

Expand Down
12 changes: 6 additions & 6 deletions src/core/build_target_benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func BenchmarkProvideFor(b *testing.B) {
b.Run("Simple", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target1.provideFor(target2)
result = target1.provideFor(target2, true)
}
})

Expand All @@ -26,7 +26,7 @@ func BenchmarkProvideFor(b *testing.B) {
b.Run("NoMatch", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target2.provideFor(target1)
result = target2.provideFor(target1, true)
}
})

Expand All @@ -35,15 +35,15 @@ func BenchmarkProvideFor(b *testing.B) {
b.Run("OneMatch", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target2.provideFor(target1)
result = target2.provideFor(target1, true)
}
})

target1.Requires = []string{"go", "go_src"}
b.Run("TwoMatches", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target2.provideFor(target1)
result = target2.provideFor(target1, true)
}
})

Expand All @@ -52,15 +52,15 @@ func BenchmarkProvideFor(b *testing.B) {
b.Run("FourMatches", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target2.provideFor(target1)
result = target2.provideFor(target1, true)
}
})

target1.AddDatum(target2.Label)
b.Run("IsData", func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
result = target2.provideFor(target1)
result = target2.provideFor(target1, true)
}
})
}
31 changes: 27 additions & 4 deletions src/core/build_target_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,18 +219,41 @@ func TestProvideFor(t *testing.T) {
// target1 is provided directly since they have a simple dependency
target1 := makeTarget1("//src/core:target1", "PUBLIC")
target2 := makeTarget1("//src/core:target2", "PUBLIC", target1)
assert.Equal(t, []BuildLabel{target1.Label}, target1.ProvideFor(target2))
assert.Equal(t, []BuildLabel{target1.Label}, target1.ProvideFor(target2, true))
// Now have target2 provide target1. target3 will get target1 instead.
target2.Provides = map[string]BuildLabel{"whatevs": target1.Label}
target3 := makeTarget1("//src/core:target3", "PUBLIC", target2)
target3.Requires = append(target3.Requires, "whatevs")
assert.Equal(t, []BuildLabel{target1.Label}, target2.ProvideFor(target3))
assert.Equal(t, []BuildLabel{target1.Label}, target2.ProvideFor(target3, true))
// Now target4 has a data dependency on target2. It has the same requirement as target3 but
// it gets target2 instead of target1, because that's just how data deps work.
target4 := makeTarget1("//src/core:target4", "PUBLIC", target2)
target4.Data = append(target4.Data, target2.Label)
target4.Requires = append(target4.Requires, "whatevs")
assert.Equal(t, []BuildLabel{target2.Label}, target2.ProvideFor(target4))
assert.Equal(t, []BuildLabel{target2.Label}, target2.ProvideFor(target4, true))
}

func TestDefaultProvide(t *testing.T) {
goTarget := makeTarget1("//src/core:go", "PUBLIC")
defaultTarget := makeTarget1("//src/core:default", "PUBLIC")
providesGo := makeTarget1("//src/core:provider", "PUBLIC", goTarget, defaultTarget)

requireGo := makeTarget1("//src/core:req_go", "PUBLIC")
requirePython := makeTarget1("//src/core:req_python", "PUBLIC")
requireNothing := makeTarget1("//src/core:req_none", "PUBLIC")

requireGo.Requires = []string{"go"}
requirePython.Requires = []string{"python"}

providesGo.Provides = map[string]BuildLabel{
"go": goTarget.Label,
"default": defaultTarget.Label,
}

assert.Equal(t, []BuildLabel{defaultTarget.Label}, providesGo.ProvideFor(requirePython, true))
assert.Equal(t, []BuildLabel{defaultTarget.Label}, providesGo.ProvideFor(requireNothing, true))
assert.Equal(t, []BuildLabel{providesGo.Label}, providesGo.ProvideFor(requirePython, false))
assert.Equal(t, []BuildLabel{goTarget.Label}, providesGo.ProvideFor(requireGo, true))
}

func TestAddProvide(t *testing.T) {
Expand All @@ -240,7 +263,7 @@ func TestAddProvide(t *testing.T) {
target2.AddDependency(target1.Label)
target2.AddProvide("go", ParseBuildLabel(":target1", "src/core"))
target3.Requires = append(target3.Requires, "go")
assert.Equal(t, []BuildLabel{target1.Label}, target2.ProvideFor(target3))
assert.Equal(t, []BuildLabel{target1.Label}, target2.ProvideFor(target3, true))
}

func TestAddDatum(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions src/core/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ type Configuration struct {
buildEnvStored *storedBuildEnv

FeatureFlags struct {
FFDefaultProvides bool `help:"Allows setting 'default' in the provides map to control what happens when none of the provided targets match"`
} `help:"Flags controlling preview features for the next release. Typically these config options gate breaking changes and only have a lifetime of one major release."`
Metrics struct {
PrometheusGatewayURL string `help:"The gateway URL to push prometheus updates to."`
Expand Down
4 changes: 2 additions & 2 deletions src/core/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,10 @@ func NewGraph() *BuildGraph {

// DependentTargets returns the labels that 'from' should actually depend on when it declared a dependency on 'to'.
// This is normally just 'to' but could be otherwise given require/provide shenanigans.
func (graph *BuildGraph) DependentTargets(from, to BuildLabel) []BuildLabel {
func (graph *BuildGraph) DependentTargets(from, to BuildLabel, ffDefaultProvide bool) []BuildLabel {
fromTarget := graph.Target(from)
if toTarget := graph.Target(to); fromTarget != nil && toTarget != nil {
return toTarget.ProvideFor(fromTarget)
return toTarget.ProvideFor(fromTarget, ffDefaultProvide)
}
return []BuildLabel{to}
}
2 changes: 1 addition & 1 deletion src/core/graph_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestDependentTargets(t *testing.T) {
graph.AddTarget(target1)
graph.AddTarget(target2)
graph.AddTarget(target3)
assert.Equal(t, []BuildLabel{target3.Label}, graph.DependentTargets(target2.Label, target1.Label))
assert.Equal(t, []BuildLabel{target3.Label}, graph.DependentTargets(target2.Label, target1.Label, true))
}

func TestSubrepo(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions src/core/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ func (state *BuildState) ActivateTarget(pkg *Package, label, dependent BuildLabe
}
}
} else {
for _, l := range state.Graph.DependentTargets(dependent, label) {
for _, l := range state.Graph.DependentTargets(dependent, label, state.Config.FeatureFlags.FFDefaultProvides) {
// We use :all to indicate a dependency needed for parse.
if err := state.QueueTarget(l, dependent, dependent.IsAllTargets(), mode); err != nil {
return err
Expand Down Expand Up @@ -1088,7 +1088,7 @@ func (state *BuildState) queueTarget(label, dependent BuildLabel, forceBuild boo
if dependent.IsAllTargets() || dependent == OriginalTarget {
return state.queueResolvedTarget(target, forceBuild, mode)
}
for _, l := range target.ProvideFor(state.Graph.TargetOrDie(dependent)) {
for _, l := range target.ProvideFor(state.Graph.TargetOrDie(dependent), state.Config.FeatureFlags.FFDefaultProvides) {
if l == label {
if err := state.queueResolvedTarget(target, forceBuild, mode); err != nil {
return err
Expand Down Expand Up @@ -1167,7 +1167,7 @@ func (state *BuildState) queueTargetAsync(target *BuildTarget, forceBuild, build
if err := target.resolveDependencies(state.Graph, func(t *BuildTarget) error {
called.SetTrue()
return state.queueResolvedTarget(t, forceBuild, ParseModeNormal)
}); err != nil {
}, state.Config.FeatureFlags.FFDefaultProvides); err != nil {
state.asyncError(target.Label, err)
return
}
Expand Down
Loading

0 comments on commit 3269527

Please sign in to comment.