Introduce flag --incompatible_disallow_empty_glob=true

Implements bazelbuild#8195 When the flag is enabled, the default value of allow_empty in glob is False. RELNOTES: New flag `--incompatible_disallow_empty_glob`. See bazelbuild#8195 PiperOrigin-RevId: 250263059
laszlocsomor · May 28, 2019 · 9bc841e · 9bc841e
1 parent 934045f
commit 9bc841e
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 18 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -534,13 +534,13 @@ private ImmutableMap<String, PackageArgument<?>> createPackageArguments() {
         @Param(
             name = "allow_empty",
             type = Boolean.class,
-            defaultValue = "True",
+            defaultValue = "unbound",
             positional = false,
             named = true,
             doc =
                 "Whether we allow glob patterns to match nothing. If `allow_empty` is False, each"
                     + " individual include pattern must match something and also the final"
-                    + " resultmust be non-empty (after the matches of the `exclude` patterns are"
+                    + " result must be non-empty (after the matches of the `exclude` patterns are"
                     + " excluded).")
       },
       documented = false,
@@ -554,7 +554,7 @@ public SkylarkList invoke(
                 SkylarkList include,
                 SkylarkList exclude,
                 Integer excludeDirectories,
-                Boolean allowEmpty,
+                Object allowEmpty,
                 FuncallExpression ast,
                 Environment env)
                 throws EvalException, ConversionException, InterruptedException {
@@ -570,7 +570,7 @@ static SkylarkList<Object> callGlob(
       Object include,
       Object exclude,
       boolean excludeDirs,
-      boolean allowEmpty,
+      Object allowEmptyArgument,
       FuncallExpression ast,
       Environment env)
       throws EvalException, ConversionException, InterruptedException {
@@ -587,6 +587,17 @@ static SkylarkList<Object> callGlob(
     List<String> excludes = Type.STRING_LIST.convert(exclude, "'glob' argument");
 
     List<String> matches;
+    boolean allowEmpty;
+    if (allowEmptyArgument == Runtime.UNBOUND) {
+      allowEmpty = !env.getSemantics().incompatibleDisallowEmptyGlob();
+    } else if (allowEmptyArgument instanceof Boolean) {
+      allowEmpty = (Boolean) allowEmptyArgument;
+    } else {
+      throw new EvalException(
+          ast.getLocation(),
+          "expected boolean for argument `allow_empty`, got `" + allowEmptyArgument + "`");
+    }
+
     try {
       Globber.Token globToken =
           context.globber.runAsync(includes, excludes, excludeDirs, allowEmpty);

diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
@@ -37,7 +37,7 @@ public SkylarkList<?> glob(
       SkylarkList<?> include,
       SkylarkList<?> exclude,
       Integer excludeDirectories,
-      Boolean allowEmpty,
+      Object allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException, ConversionException, InterruptedException {

diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -254,6 +254,19 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
       help = "If set to true, the `+` becomes disabled for dicts.")
   public boolean incompatibleDisallowDictPlus;
 
+  @Option(
+      name = "incompatible_disallow_empty_glob",
+      defaultValue = "false",
+      category = "incompatible changes",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help = "If set to true, the default value of the `allow_empty` argument of glob() is False.")
+  public boolean incompatibleDisallowEmptyGlob;
+
   @Option(
       name = "incompatible_disallow_filetype",
       defaultValue = "true",
@@ -613,6 +626,7 @@ public StarlarkSemantics toSkylarkSemantics() {
         .incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams)
         .incompatibleDisableObjcProviderResources(incompatibleDisableObjcProviderResources)
         .incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
+        .incompatibleDisallowEmptyGlob(incompatibleDisallowEmptyGlob)
         .incompatibleDisallowFileType(incompatibleDisallowFileType)
         .incompatibleDisallowLegacyJavaInfo(incompatibleDisallowLegacyJavaInfo)
         .incompatibleDisallowLegacyJavaProvider(incompatibleDisallowLegacyJavaProvider)

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkNativeModuleApi.java
@@ -77,13 +77,13 @@ public interface SkylarkNativeModuleApi {
             doc = "A flag whether to exclude directories or not."),
         @Param(
             name = "allow_empty",
-            type = Boolean.class,
-            defaultValue = "True",
+            type = Object.class,
+            defaultValue = "unbound",
             named = true,
             doc =
                 "Whether we allow glob patterns to match nothing. If `allow_empty` is False, each"
                     + " individual include pattern must match something and also the final"
-                    + " resultmust be non-empty (after the matches of the `exclude` patterns are"
+                    + " result must be non-empty (after the matches of the `exclude` patterns are"
                     + " excluded).")
       },
       useAst = true,
@@ -92,7 +92,7 @@ public SkylarkList<?> glob(
       SkylarkList<?> include,
       SkylarkList<?> exclude,
       Integer excludeDirectories,
-      Boolean allowEmpty,
+      Object allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException, InterruptedException;

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -147,6 +147,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleDisallowDictPlus();
 
+  public abstract boolean incompatibleDisallowEmptyGlob();
+
   public abstract boolean incompatibleDisallowFileType();
 
   public abstract boolean incompatibleDisallowLegacyJavaProvider();
@@ -227,6 +229,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDisableDeprecatedAttrParams(true)
           .incompatibleDisableObjcProviderResources(true)
           .incompatibleDisallowDictPlus(true)
+          .incompatibleDisallowEmptyGlob(false)
           .incompatibleDisallowFileType(true)
           .incompatibleDisallowLegacyJavaProvider(false)
           .incompatibleDisallowLegacyJavaInfo(false)
@@ -291,6 +294,8 @@ public abstract static class Builder {
 
     public abstract Builder incompatibleDisallowFileType(boolean value);
 
+    public abstract Builder incompatibleDisallowEmptyGlob(boolean value);
+
     public abstract Builder incompatibleDisallowLegacyJavaProvider(boolean value);
 
     public abstract Builder incompatibleDisallowLegacyJavaInfo(boolean value);

diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java
@@ -34,7 +34,7 @@ public SkylarkList<?> glob(
       SkylarkList<?> include,
       SkylarkList<?> exclude,
       Integer excludeDirectories,
-      Boolean allowEmpty,
+      Object allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException, InterruptedException {

diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -661,7 +661,7 @@ public void testInsufficientArgumentGlobErrors() throws Exception {
         "glob()",
         "insufficient arguments received by glob(include: sequence of strings, "
             + "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
-            + "allow_empty: bool = True) (got 0, expected at least 1)");
+            + "allow_empty: bool = <unbound>) (got 0, expected at least 1)");
   }
 
   @Test
@@ -671,7 +671,7 @@ public void testGlobUnamedExclude() throws Exception {
         "glob(['a'], ['b'])",
         "too many (2) positional arguments in call to glob(include: sequence of strings, "
             + "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
-            + "allow_empty: bool = True)");
+            + "allow_empty: bool = <unbound>)");
   }
 
   @Test
@@ -681,7 +681,7 @@ public void testTooManyArgumentsGlobErrors() throws Exception {
         "glob(1,2,3,4)",
         "too many (4) positional arguments in call to glob(include: sequence of strings, "
             + "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
-            + "allow_empty: bool = True)");
+            + "allow_empty: bool = <unbound>)");
   }
 
   @Test
@@ -808,6 +808,48 @@ public void testGlobWithIOErrors() throws Exception {
     events.assertContainsError("Directory is not readable");
   }
 
+  @Test
+  public void testGlobDisallowEmpty() throws Exception {
+    Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'])");
+    Package pkg =
+        packages.createPackage(
+            "pkg",
+            RootedPath.toRootedPath(root, buildFile),
+            "--incompatible_disallow_empty_glob=false");
+    assertThat(pkg.containsErrors()).isFalse();
+  }
+
+  @Test
+  public void testGlobAllowEmpty() throws Exception {
+    events.setFailFast(false);
+
+    Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'])");
+    Package pkg =
+        packages.createPackage(
+            "pkg",
+            RootedPath.toRootedPath(root, buildFile),
+            "--incompatible_disallow_empty_glob=true");
+
+    assertThat(pkg.containsErrors()).isTrue();
+    events.assertContainsError(
+        "glob pattern '*.foo' didn't match anything, but allow_empty is set to False");
+  }
+
+  @Test
+  public void testGlobNotBoolean() throws Exception {
+    events.setFailFast(false);
+
+    Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty = 5)");
+    Package pkg =
+        packages.createPackage(
+            "pkg",
+            RootedPath.toRootedPath(root, buildFile),
+            "--incompatible_disallow_empty_glob=true");
+
+    assertThat(pkg.containsErrors()).isTrue();
+    events.assertContainsError("expected boolean for argument `allow_empty`, got `5`");
+  }
+
   @Test
   public void testNativeModuleIsAvailable() throws Exception {
     Path buildFile = scratch.file("/pkg/BUILD", "native.cc_library(name='bar')");

diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -140,6 +140,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_disable_objc_provider_resources=" + rand.nextBoolean(),
         "--incompatible_disable_third_party_license_checking=" + rand.nextBoolean(),
         "--incompatible_disallow_dict_plus=" + rand.nextBoolean(),
+        "--incompatible_disallow_empty_glob=" + rand.nextBoolean(),
         "--incompatible_disallow_filetype=" + rand.nextBoolean(),
         "--incompatible_disallow_legacy_javainfo=" + rand.nextBoolean(),
         "--incompatible_disallow_legacy_java_provider=" + rand.nextBoolean(),
@@ -191,6 +192,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleDisableObjcProviderResources(rand.nextBoolean())
         .incompatibleDisableThirdPartyLicenseChecking(rand.nextBoolean())
         .incompatibleDisallowDictPlus(rand.nextBoolean())
+        .incompatibleDisallowEmptyGlob(rand.nextBoolean())
         .incompatibleDisallowFileType(rand.nextBoolean())
         .incompatibleDisallowLegacyJavaInfo(rand.nextBoolean())
         .incompatibleDisallowLegacyJavaProvider(rand.nextBoolean())

diff --git a/tools/cpp/BUILD b/tools/cpp/BUILD
@@ -400,10 +400,13 @@ filegroup(
 
 filegroup(
     name = "compile-x64_windows",
-    srcs = glob([
-        "wrapper/bin/msvc_*",
-        "wrapper/bin/pydir/msvc*",
-    ]),
+    srcs = glob(
+        [
+            "wrapper/bin/msvc_*",
+            "wrapper/bin/pydir/msvc*",
+        ],
+        allow_empty = True,
+    ),
 )
 
 filegroup(

diff --git a/tools/cpp/BUILD.tpl b/tools/cpp/BUILD.tpl
@@ -37,7 +37,7 @@ filegroup(
 
 filegroup(
     name = "compiler_deps",
-    srcs = glob(["extra_tools/**"]) + ["%{cc_compiler_deps}"],
+    srcs = glob(["extra_tools/**"], allow_empty = True) + ["%{cc_compiler_deps}"],
 )
 
 # This is the entry point for --crosstool_top.  Toolchains are found