[engine] Allow diamond dependencies
Previously, this dependency graph was incorrectly considered a cycle and
was not allowed, because `common_dependency.star` is imported twice
during the loading of `shac.star`:
shac.star
| |
V |
dep.star |
| |
V V
common_dependency.star
However, there isn't actually a cycle here; it's still a DAG and all
DAGs should be considered valid dependency graphs.
Change-Id: Ic3bd5969eeb74ce0b9ee238bc9902b4c7e363836
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/911234
Reviewed-by: Danielle Kay <danikay@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
diff --git a/internal/engine/env.go b/internal/engine/env.go
index 63fe943..a3d4cd3 100644
--- a/internal/engine/env.go
+++ b/internal/engine/env.go
@@ -161,17 +161,24 @@
e.mu.Unlock()
if source.th == th {
- // This source is currently being loaded by this very thread,
- // meaning we encountered a dependency cycle.
- return nil, fmt.Errorf("%s was loaded in a cycle dependency graph", sk.String())
+ // `loadInner` will not be called concurrently with the same
+ // Starlark thread, so if the current thread already has a lock on
+ // this source it means there's a dependency cycle that would cause
+ // a deadlock.
+ if !source.mu.TryLock() {
+ return nil, fmt.Errorf("%s was loaded in a cycle dependency graph", sk.String())
+ }
+ } else {
+ // The source may be concurrently processed by another starlark
+ // thread that's traversing the dependency graph of a separate
+ // shac.star file, wait for the processing to complete by taking the
+ // lock.
+ //
+ // This block is hard to cover since it's a race condition. We'd
+ // have to inject a builtin that would hang the starlark execution
+ // to force concurrency.
+ source.mu.Lock()
}
- // The following lines are hard to cover since it's a race condition.
- // We'd have to inject a builtin that would hang the starlark execution
- // to force concurrency.
-
- // The source may be concurrently processed by another starlark thread,
- // wait for the processing to complete by taking the lock.
- source.mu.Lock()
defer source.mu.Unlock()
return source.globals, source.err
}
diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go
index 6c2b1ba..81d1ddb 100644
--- a/internal/engine/run_test.go
+++ b/internal/engine/run_test.go
@@ -1752,6 +1752,11 @@
"[//dir-shac.star:15] [\"check\", \"commit_hash\", \"register_check\", \"version\"]\n",
},
{
+ "load-diamond_dependency.star",
+ "[//load-diamond_dependency.star:18] i am a constant\n" +
+ "[//load-diamond_dependency.star:19] i am a constant #2\n",
+ },
+ {
"print-shac-version.star",
"[//print-shac-version.star:15] " + v + "\n",
},
diff --git a/internal/engine/runtime_shac.go b/internal/engine/runtime_shac.go
index a4b0b61..bdc5efb 100644
--- a/internal/engine/runtime_shac.go
+++ b/internal/engine/runtime_shac.go
@@ -27,7 +27,7 @@
// Version is the current tool version.
//
// TODO(maruel): Add proper version, preferably from git tag.
- Version = [...]int{0, 1, 0}
+ Version = [...]int{0, 1, 1}
)
// getShac returns the global shac object.
diff --git a/internal/engine/testdata/print/lib/a_library.star b/internal/engine/testdata/print/lib/a_library.star
new file mode 100644
index 0000000..70d1365
--- /dev/null
+++ b/internal/engine/testdata/print/lib/a_library.star
@@ -0,0 +1,15 @@
+# Copyright 2023 The Shac Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+CONSTANT = "i am a constant"
diff --git a/internal/engine/testdata/print/lib/dependent_of_a_library.star b/internal/engine/testdata/print/lib/dependent_of_a_library.star
new file mode 100644
index 0000000..33fc6ae
--- /dev/null
+++ b/internal/engine/testdata/print/lib/dependent_of_a_library.star
@@ -0,0 +1,17 @@
+# Copyright 2023 The Shac Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load("./a_library.star", "CONSTANT")
+
+ANOTHER_CONSTANT = CONSTANT + " #2"
diff --git a/internal/engine/testdata/print/load-diamond_dependency.star b/internal/engine/testdata/print/load-diamond_dependency.star
new file mode 100644
index 0000000..ca2f06e
--- /dev/null
+++ b/internal/engine/testdata/print/load-diamond_dependency.star
@@ -0,0 +1,19 @@
+# Copyright 2023 The Shac Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load("./lib/a_library.star", "CONSTANT")
+load("./lib/dependent_of_a_library.star", "ANOTHER_CONSTANT")
+
+print(CONSTANT)
+print(ANOTHER_CONSTANT)
