Move depset order validation from PyCommon to PyStructUtils

The fact that the field is a depset is already an invariant of the provider, so we may as well include the order in that requirement. This is not a breaking change because it's currently enforced by PyCommon. Moving the validation out of PyCommon will make it easier to refactor the "get this field from that dep" helpers out of PyCommon and into a shared PyProviderUtils helper. Work toward #7010. RELNOTES: None PiperOrigin-RevId: 230780935
bazelbuild · Jan 24, 2019 · 14c1793 · 14c1793
1 parent 50dfd73
commit 14c1793
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 43 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
@@ -234,13 +234,7 @@ private static void collectTransitivePythonSourcesFromDeps(
         try {
           StructImpl info = PyStructUtils.getProvider(dep);
           NestedSet<Artifact> sources = PyStructUtils.getTransitiveSources(info);
-          if (!builder.getOrder().isCompatible(sources.getOrder())) {
-            ruleContext.ruleError(
-                getOrderErrorMessage(
-                    PyStructUtils.TRANSITIVE_SOURCES, builder.getOrder(), sources.getOrder()));
-          } else {
-            builder.addTransitive(sources);
-          }
+          builder.addTransitive(sources);
         } catch (EvalException e) {
           // Either the provider type or field type is bad.
           ruleContext.ruleError(e.getMessage());

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyStructUtils.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyStructUtils.java
@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.packages.StructProvider;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -38,8 +39,8 @@ private PyStructUtils() {}
   public static final String PROVIDER_NAME = "py";
 
   /**
-   * Name of field holding a postorder depset of transitive sources (i.e., .py files in srcs and in
-   * srcs of transitive deps).
+   * Name of field holding a postorder-compatible depset of transitive sources (i.e., .py files in
+   * {@code srcs} and in {@code srcs} of transitive {@code deps}).
    */
   public static final String TRANSITIVE_SOURCES = "transitive_sources";
 
@@ -132,7 +133,16 @@ public static NestedSet<Artifact> getTransitiveSources(StructImpl info) throws E
             PROVIDER_NAME,
             TRANSITIVE_SOURCES,
             EvalUtils.getDataTypeNameFromClass(fieldValue.getClass()));
-    return castValue.getSet(Artifact.class);
+    NestedSet<Artifact> unwrappedValue = castValue.getSet(Artifact.class);
+    if (!unwrappedValue.getOrder().isCompatible(Order.COMPILE_ORDER)) {
+      throw new EvalException(
+          /*location=*/ null,
+          String.format(
+              "Incompatible depset order for 'transitive_sources': expected 'default' or "
+                  + "'postorder', but got '%s'",
+              unwrappedValue.getOrder().getSkylarkName()));
+    }
+    return unwrappedValue;
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
@@ -231,39 +231,15 @@ public void getTransitiveSources_WrongType() {
 
   @Test
   public void getTransitiveSources_OrderMismatch() throws Exception {
-    reporter.removeHandler(failFastHandler);
-    // Depset order mismatches should be caught as rule errors.
-    scratch.file("rules/BUILD");
-    scratch.file(
-        "rules/badorderrule.bzl",
-        "def _badorderrule_impl(ctx):",
-        // Native rules use "compile" / "postorder", so using "preorder" here creates a conflict.
-        "    info = struct(transitive_sources=depset(direct=[], order='preorder'))",
-        "    return struct(py=info)",
-        "",
-        "badorderrule = rule(",
-        "    implementation = _badorderrule_impl",
-        ")");
-    scratch.file(
-        "badordertarget/BUILD",
-        "load('//rules:badorderrule.bzl', 'badorderrule')",
-        "",
-        "badorderrule(",
-        "    name = 'badorderdep',",
-        ")",
-        "py_library(",
-        "    name = 'pylib',",
-        "    srcs = ['pylib.py'],",
-        ")",
-        "py_binary(",
-        "    name = 'pybin',",
-        "    srcs = ['pybin.py'],",
-        "    deps = [':pylib', ':badorderdep'],",
-        ")");
-    getConfiguredTarget("//badordertarget:pybin");
-    assertContainsEvent(
-        "Incompatible order for transitive_sources: expected 'default' or 'postorder', got "
-            + "'preorder'");
+    NestedSet<Artifact> sources = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
+    StructImpl info =
+        makeStruct(
+            ImmutableMap.of(
+                PyStructUtils.TRANSITIVE_SOURCES, SkylarkNestedSet.of(Artifact.class, sources)));
+    assertThrowsEvalExceptionContaining(
+        () -> PyStructUtils.getTransitiveSources(info),
+        "Incompatible depset order for 'transitive_sources': expected 'default' or 'postorder', "
+            + "but got 'preorder'");
   }
 
   @Test