Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow 'default' entry in provide map for when there's no match #3138

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably requires a bit of thought


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
Loading