From 6af390e3bb89735d8037d53832bba315be764b8f Mon Sep 17 00:00:00 2001 From: laurentlb Date: Tue, 28 May 2019 05:59:35 -0700 Subject: [PATCH] Introduce flag --incompatible_disallow_empty_glob=true Implements https://github.com/bazelbuild/bazel/issues/8195 When the flag is enabled, the default value of allow_empty in glob is False. RELNOTES: New flag `--incompatible_disallow_empty_glob`. See https://github.com/bazelbuild/bazel/issues/8195 PiperOrigin-RevId: 250263059 --- .../build/lib/packages/PackageFactory.java | 19 ++++++-- .../lib/packages/SkylarkNativeModule.java | 2 +- .../packages/StarlarkSemanticsOptions.java | 14 ++++++ .../SkylarkNativeModuleApi.java | 8 ++-- .../build/lib/syntax/StarlarkSemantics.java | 5 ++ .../FakeSkylarkNativeModuleApi.java | 2 +- .../lib/packages/PackageFactoryTest.java | 48 +++++++++++++++++-- .../SkylarkSemanticsConsistencyTest.java | 2 + tools/cpp/BUILD | 11 +++-- tools/cpp/BUILD.tpl | 2 +- 10 files changed, 95 insertions(+), 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 index 54cc3b2cfca6ab..f6aa4b659d370c 100644 --- 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> 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 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 callGlob( List excludes = Type.STRING_LIST.convert(exclude, "'glob' argument"); List 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 index 7b9bdcafdce91f..6a8eef123fa013 100644 --- 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 index ba7ecc585af721..e9b26c272fb63e 100644 --- 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 index c2c2c73afe9f9c..2660411229f3dc 100644 --- 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 index 7a879d7db72e77..183fe5536b4b4b 100644 --- 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 index 49a1783bdb2cb5..5939c69e7571a1 100644 --- 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 index bdd7d35490e0e2..7583924e236d42 100644 --- 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 = ) (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 = )"); } @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 = )"); } @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 index d202f70b56c3a3..bdd8af98ef8762 100644 --- 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 index fbab3f3a7261ea..34c428999b999e 100644 --- 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 index a91952412b0688..97538ab342114d 100644 --- 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