From 6ae8e2ecd2431175345cd00c9d9386cefdcbc08f Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 3 Dec 2024 16:42:32 +0000 Subject: [PATCH] Fix potential subinclude lockup (#3305) * test case * Ban it * make it fail * make work again * Attempt fix * comment --- src/core/package.go | 3 +++ src/core/state.go | 7 ++++++- src/parse/asp/builtins.go | 2 +- test/double_local_subinclude/BUILD | 8 ++++++++ .../double_local_subinclude/test_repo/.plzconfig | 0 .../test_repo/a/1.build_defs | 1 + .../test_repo/a/2.build_defs | 3 +++ .../test_repo/a/BUILD_FILE | 16 ++++++++++++++++ .../test_repo/b/BUILD_FILE | 3 +++ .../test_repo/c/BUILD_FILE | 3 +++ 10 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 test/double_local_subinclude/BUILD create mode 100644 test/double_local_subinclude/test_repo/.plzconfig create mode 100644 test/double_local_subinclude/test_repo/a/1.build_defs create mode 100644 test/double_local_subinclude/test_repo/a/2.build_defs create mode 100644 test/double_local_subinclude/test_repo/a/BUILD_FILE create mode 100644 test/double_local_subinclude/test_repo/b/BUILD_FILE create mode 100644 test/double_local_subinclude/test_repo/c/BUILD_FILE diff --git a/src/core/package.go b/src/core/package.go index 93c9a85c6c..2ba00a911b 100644 --- a/src/core/package.go +++ b/src/core/package.go @@ -240,6 +240,9 @@ func FindOwningPackage(state *BuildState, file string) BuildLabel { // suggestTargets suggests the targets in the given package that might be misspellings of // the requested one. func suggestTargets(pkg *Package, label, dependent BuildLabel) string { + if pkg == nil { + return "" + } // The initial haystack only contains target names haystack := []string{} for _, t := range pkg.AllTargets() { diff --git a/src/core/state.go b/src/core/state.go index f98b22881a..69d1490260 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -1017,7 +1017,9 @@ func (state *BuildState) ActivateTarget(pkg *Package, label, dependent BuildLabe // Bazel allows some things that look like build targets but aren't - notably the syntax // to load(). It suits us to treat that as though it is one, but we now have to // implicitly make it available. - exportFile(state, pkg, label) + if pkg != nil { + exportFile(state, pkg, label) + } } else { msg := fmt.Sprintf("Parsed build file %s but it doesn't contain target %s", pkg.Filename, label.Name) if dependent != OriginalTarget { @@ -1029,6 +1031,9 @@ func (state *BuildState) ActivateTarget(pkg *Package, label, dependent BuildLabe if state.ParsePackageOnly && !mode.IsForSubinclude() { return nil // Some kinds of query don't need a full recursive parse. } else if label.IsAllTargets() { + if pkg == nil { + return fmt.Errorf("Cannot use :all in this context") + } if dependent == OriginalTarget { for _, target := range pkg.AllTargets() { // Don't activate targets that were added in a post-build function; that causes a race condition diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index c8bd6253b1..acef0b934e 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -392,7 +392,7 @@ func subincludeTarget(s *scope, l core.BuildLabel) *core.BuildTarget { // If the subinclude is local to this package, it must already exist in the graph. If it already exists in the graph // but isn't activated, we should activate it otherwise WaitForSubincludedTarget might block. This can happen when // another package also subincludes this target, and queues it first. - if isLocal && s.pkg != nil { + if isLocal { t := s.state.Graph.Target(l) if t == nil { s.Error("Target :%s is not defined in this package; it has to be defined before the subinclude() call", l.Name) diff --git a/test/double_local_subinclude/BUILD b/test/double_local_subinclude/BUILD new file mode 100644 index 0000000000..290c88e2a3 --- /dev/null +++ b/test/double_local_subinclude/BUILD @@ -0,0 +1,8 @@ +subinclude("//test/build_defs") + +please_repo_e2e_test( + name = "double_local_subinclude_test", + # This isn't 100% deterministic but 20 clean runs should be reasonably sufficient + plz_command = "for i in `seq 1 20`; do plz build -p -v 2 //...; done", + repo = "test_repo", +) diff --git a/test/double_local_subinclude/test_repo/.plzconfig b/test/double_local_subinclude/test_repo/.plzconfig new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/double_local_subinclude/test_repo/a/1.build_defs b/test/double_local_subinclude/test_repo/a/1.build_defs new file mode 100644 index 0000000000..500549eefb --- /dev/null +++ b/test/double_local_subinclude/test_repo/a/1.build_defs @@ -0,0 +1 @@ +THE_QUESTION = "what is the answer to life, the universe and everything?" diff --git a/test/double_local_subinclude/test_repo/a/2.build_defs b/test/double_local_subinclude/test_repo/a/2.build_defs new file mode 100644 index 0000000000..eec3aabb4c --- /dev/null +++ b/test/double_local_subinclude/test_repo/a/2.build_defs @@ -0,0 +1,3 @@ +subinclude("//a:defs_1") + +THE_ANSWER = 42 diff --git a/test/double_local_subinclude/test_repo/a/BUILD_FILE b/test/double_local_subinclude/test_repo/a/BUILD_FILE new file mode 100644 index 0000000000..d6b75525cd --- /dev/null +++ b/test/double_local_subinclude/test_repo/a/BUILD_FILE @@ -0,0 +1,16 @@ +filegroup( + name = "defs_1", + srcs = ["1.build_defs"], + visibility = ["PUBLIC"], +) + +filegroup( + name = "defs_2", + srcs = ["2.build_defs"], + visibility = ["PUBLIC"], +) + +subinclude(":defs_2") + +assert THE_QUESTION.endswith("?"), "not a question" +assert THE_ANSWER == 42, "wrong answer to life, the universe and everything" diff --git a/test/double_local_subinclude/test_repo/b/BUILD_FILE b/test/double_local_subinclude/test_repo/b/BUILD_FILE new file mode 100644 index 0000000000..ff4038bbbe --- /dev/null +++ b/test/double_local_subinclude/test_repo/b/BUILD_FILE @@ -0,0 +1,3 @@ +subinclude("//a:defs_2") + +assert THE_ANSWER == 42, "wrong answer to life, the universe and everything" diff --git a/test/double_local_subinclude/test_repo/c/BUILD_FILE b/test/double_local_subinclude/test_repo/c/BUILD_FILE new file mode 100644 index 0000000000..d95ba18cdb --- /dev/null +++ b/test/double_local_subinclude/test_repo/c/BUILD_FILE @@ -0,0 +1,3 @@ +subinclude("//a:defs_1") + +assert THE_QUESTION.endswith("?"), "not a question"