C++: Change Skylark API whitelisting to be part of flag.
This uses SkylarkSemantics now instead of the C++ configuration. The flag is:
--experimental_cc_skylark_api_enabled_packages
RELNOTES:none
PiperOrigin-RevId: 207235431
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java
index 3b3d944..0195eda 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java
@@ -36,8 +36,8 @@
/**
* A module that contains Skylark utilities for C++ support.
*
- * <p>This is a work in progress. The API is guarded behind --experimental_enable_cc_skylark_api.
- * The API is under development and unstable.
+ * <p>This is a work in progress. The API is guarded behind
+ * --experimental_cc_skylark_api_enabled_packages. The API is under development and unstable.
*/
public class BazelCcModule extends CcModule
implements BazelCcModuleApi<
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index e48c1be..3277c38 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -15,12 +15,14 @@
package com.google.devtools.build.lib.packages;
import com.google.devtools.build.lib.syntax.SkylarkSemantics;
+import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsBase;
import java.io.Serializable;
+import java.util.List;
/**
* Contains options that affect Skylark's semantics.
@@ -41,9 +43,6 @@
* should be that name written in snake_case. Add a line to set the new field in {@link
* #toSkylarkSemantics}.
*
- * <li>Add a line to read and write the new field in {@link SkylarkSemanticsCodec#serialize} and
- * {@link SkylarkSemanticsCodec#deserialize}.
- *
* <li>Add a line to set the new field in both {@link
* SkylarkSemanticsConsistencyTest#buildRandomOptions} and {@link
* SkylarkSemanticsConsistencyTest#buildRandomSemantics}.
@@ -69,6 +68,18 @@
public boolean experimentalEnableRepoMapping;
@Option(
+ name = "experimental_cc_skylark_api_enabled_packages",
+ converter = CommaSeparatedOptionListConverter.class,
+ defaultValue = "",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+ metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+ help =
+ "Passes list of packages that can use the C++ Skylark API. Don't enable this flag yet, "
+ + "we will be making breaking changes.")
+ public List<String> experimentalCcSkylarkApiEnabledPackages;
+
+ @Option(
name = "incompatible_bzl_disallow_load_after_statement",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
@@ -350,6 +361,7 @@
public SkylarkSemantics toSkylarkSemantics() {
return SkylarkSemantics.builder()
// <== Add new options here in alphabetic order ==>
+ .experimentalCcSkylarkApiEnabledPackages(experimentalCcSkylarkApiEnabledPackages)
.experimentalEnableRepoMapping(experimentalEnableRepoMapping)
.incompatibleBzlDisallowLoadAfterStatement(incompatibleBzlDisallowLoadAfterStatement)
.incompatibleDepsetIsNotIterable(incompatibleDepsetIsNotIterable)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index b6c55be..9fc2f5e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -57,6 +57,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.Pair;
@@ -108,10 +109,6 @@
}
};
- private static final ImmutableList<PathFragment> WHITELISTED_PACKAGES =
- ImmutableList.of(
- PathFragment.create("tools/build_defs"), PathFragment.create("experimental"));
-
public static final ImmutableSet<String> ALL_COMPILE_ACTIONS =
ImmutableSet.of(
CppActionNames.C_COMPILE,
@@ -209,40 +206,44 @@
throws EvalException {
RuleContext context = skylarkRuleContext.getRuleContext();
Rule rule = context.getRule();
- if (!context.getFragment(CppConfiguration.class).getEnableCcSkylarkApi()) {
- throw new EvalException(
- rule.getLocation(),
- "Pass --experimental_enable_cc_skylark_api in "
- + "order to use the C++ API. Beware that we will be making breaking "
- + "changes to this API without prior warning.");
- }
+
RuleClass ruleClass = rule.getRuleClassObject();
Label label = ruleClass.getRuleDefinitionEnvironmentLabel();
- if (label != null
- && WHITELISTED_PACKAGES
- .stream()
- .noneMatch(path -> label.getPackageFragment().startsWith(path))) {
- throwWhiteListError(rule.getLocation(), label.getPackageFragment().toString());
+ try {
+ if (label != null) {
+ checkLocationWhitelisted(
+ context.getAnalysisEnvironment().getSkylarkSemantics(),
+ rule.getLocation(),
+ label.getPackageFragment().toString());
+ }
+ } catch (InterruptedException e) {
+ throw new EvalException(rule.getLocation(), e);
}
}
- public static void checkLocationWhitelisted(Location location) throws EvalException {
- String bzlPath = location.getPath().toString();
- if (WHITELISTED_PACKAGES.stream().noneMatch(path -> bzlPath.contains(path.toString()))) {
- throwWhiteListError(location, bzlPath);
+ public static void checkLocationWhitelisted(
+ SkylarkSemantics semantics, Location location, String callPath) throws EvalException {
+ List<String> whitelistedPackagesList = semantics.experimentalCcSkylarkApiEnabledPackages();
+ if (whitelistedPackagesList
+ .stream()
+ .noneMatch(
+ path -> callPath.startsWith(path) || callPath.startsWith("/workspace/" + path))) {
+ throwWhiteListError(location, callPath, whitelistedPackagesList);
}
}
- private static void throwWhiteListError(Location location, String bzlPath) throws EvalException {
- String whitelistedPackages =
- WHITELISTED_PACKAGES.stream().map(p -> p.toString()).collect(Collectors.joining(", "));
+ private static void throwWhiteListError(
+ Location location, String callPath, List<String> whitelistedPackagesList)
+ throws EvalException {
+ String whitelistedPackages = whitelistedPackagesList.stream().collect(Collectors.joining(", "));
throw new EvalException(
location,
String.format(
- "the C++ Skylark API is for the time being only allowed for rules in in '//%s/...'; "
- + "but this is defined in '//%s'. Contact blaze-rules@google.com for more "
- + "information.",
- whitelistedPackages, bzlPath));
+ "the C++ Skylark API is for the time being only allowed for rules in '%s'; "
+ + "but this is defined in '%s'. You can try it out by passing "
+ + "--experimental_cc_skylark_api_enabled_packages=<list of packages>. Beware that "
+ + "we will be making breaking changes to this API without prior warning.",
+ whitelistedPackages, callPath));
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java
index d6806ad..ca3a3d0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java
@@ -78,7 +78,7 @@
@SuppressWarnings("unchecked")
protected CcCompilationInfo createInstanceFromSkylark(
Object[] args, Environment env, Location loc) throws EvalException {
- CcCommon.checkLocationWhitelisted(loc);
+ CcCommon.checkLocationWhitelisted(env.getSemantics(), loc, loc.getPath().toString());
CcCompilationInfo.Builder ccCompilationInfoBuilder = CcCompilationInfo.Builder.create();
CcCompilationContext.Builder ccCompilationContext =
new CcCompilationContext.Builder(/* ruleContext= */ null);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingInfo.java
index a8a2751..4157930 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingInfo.java
@@ -78,7 +78,7 @@
@SuppressWarnings("unchecked")
protected CcLinkingInfo createInstanceFromSkylark(
Object[] args, Environment env, Location loc) throws EvalException {
- CcCommon.checkLocationWhitelisted(loc);
+ CcCommon.checkLocationWhitelisted(env.getSemantics(), loc, loc.getPath().toString());
int i = 0;
CcLinkParams staticModeParamsForDynamicLibrary = (CcLinkParams) nullIfNone(args[i++]);
CcLinkParams staticModeParamsForExecutable = (CcLinkParams) nullIfNone(args[i++]);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 300a4c9..df698ed 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -982,10 +982,6 @@
return cppOptions.useInterfaceSharedObjects;
}
- public boolean getEnableCcSkylarkApi() {
- return cppOptions.enableCcSkylarkApi;
- }
-
/**
* Returns the path to the GNU binutils 'objcopy' binary to use for this build. (Corresponds to
* $(OBJCOPY) in make-dbg.) Relative paths are relative to the execution root.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index 4e4e863..e403f33 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -784,17 +784,6 @@
public boolean enableLegacyToolchainSkylarkApi;
@Option(
- name = "experimental_enable_cc_skylark_api",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
- metadataTags = {OptionMetadataTag.EXPERIMENTAL},
- help =
- "If true, the C++ Skylark API can be used. Don't enable this flag yet, we will be making "
- + "breaking changes.")
- public boolean enableCcSkylarkApi;
-
- @Option(
name = "experimental_disable_legacy_cc_linking_api",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
@@ -875,8 +864,6 @@
public FragmentOptions getHost() {
CppOptions host = (CppOptions) getDefault();
- host.enableCcSkylarkApi = enableCcSkylarkApi;
-
// The crosstool options are partially copied from the target configuration.
if (hostCrosstoolTop == null) {
host.cppCompiler = cppCompiler;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index 814a847..5893ac49 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -15,6 +15,8 @@
package com.google.devtools.build.lib.syntax;
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import java.util.List;
/**
* Options that affect Skylark semantics.
@@ -39,6 +41,8 @@
AutoValue_SkylarkSemantics.class;
// <== Add new options here in alphabetic order ==>
+ public abstract List<String> experimentalCcSkylarkApiEnabledPackages();
+
public abstract boolean experimentalEnableRepoMapping();
public abstract boolean incompatibleBzlDisallowLoadAfterStatement();
@@ -96,6 +100,7 @@
public static final SkylarkSemantics DEFAULT_SEMANTICS =
builder()
// <== Add new options here in alphabetic order ==>
+ .experimentalCcSkylarkApiEnabledPackages(ImmutableList.of())
.experimentalEnableRepoMapping(false)
.incompatibleBzlDisallowLoadAfterStatement(false)
.incompatibleDepsetIsNotIterable(false)
@@ -124,6 +129,8 @@
public abstract static class Builder {
// <== Add new options here in alphabetic order ==>
+ public abstract Builder experimentalCcSkylarkApiEnabledPackages(List<String> value);
+
public abstract Builder experimentalEnableRepoMapping(boolean value);
public abstract Builder incompatibleBzlDisallowLoadAfterStatement(boolean value);
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 ed22d1e..21328c8 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
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
import com.google.devtools.build.lib.skyframe.serialization.DynamicCodec;
@@ -36,26 +37,22 @@
*
* <p>When adding a new option, it is trivial to make a transposition error or a copy/paste error.
* These tests guard against such errors. The following possible bugs are considered:
+ *
* <ul>
* <li>If a new option is added to {@code SkylarkSemantics} but not to {@code
* SkylarkSemanticsOptions}, or vice versa, then the programmer will either be unable to
* implement its behavior, or unable to test it from the command line and add user
* documentation. We hope that the programmer notices this on their own.
- *
- * <li>If {@link SkylarkSemanticsOptions#toSkylarkSemantics} or {@link
- * SkylarkSemanticsCodec#deserialize} is not updated to set all fields of {@code
- * SkylarkSemantics}, then it will fail immediately because all fields of {@link
+ * <li>If {@link SkylarkSemanticsOptions#toSkylarkSemantics} is not updated to set all fields of
+ * {@code SkylarkSemantics}, then it will fail immediately because all fields of {@link
* SkylarkSemantics.Builder} are mandatory.
- *
* <li>To catch a copy/paste error where the wrong field's data is threaded through {@code
* toSkylarkSemantics()} or {@code deserialize(...)}, we repeatedly generate matching random
* instances of the input and expected output objects.
- *
* <li>The {@link #checkDefaultsMatch} test ensures that there is no divergence between the
* default values of the two classes.
- *
- * <li>There is no test coverage for failing to update the non-generated webpage documentation.
- * So don't forget that!
+ * <li>There is no test coverage for failing to update the non-generated webpage documentation. So
+ * don't forget that!
* </ul>
*/
@RunWith(JUnit4.class)
@@ -121,6 +118,10 @@
private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Exception {
return parseOptions(
// <== Add new options here in alphabetic order ==>
+ "--experimental_cc_skylark_api_enabled_packages="
+ + rand.nextDouble()
+ + ","
+ + rand.nextDouble(),
"--experimental_enable_repo_mapping=" + rand.nextBoolean(),
"--incompatible_bzl_disallow_load_after_statement=" + rand.nextBoolean(),
"--incompatible_depset_is_not_iterable=" + rand.nextBoolean(),
@@ -151,6 +152,8 @@
private static SkylarkSemantics buildRandomSemantics(Random rand) {
return SkylarkSemantics.builder()
// <== Add new options here in alphabetic order ==>
+ .experimentalCcSkylarkApiEnabledPackages(
+ ImmutableList.of(String.valueOf(rand.nextDouble()), String.valueOf(rand.nextDouble())))
.experimentalEnableRepoMapping(rand.nextBoolean())
.incompatibleBzlDisallowLoadAfterStatement(rand.nextBoolean())
.incompatibleDepsetIsNotIterable(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
index e15d458..71cfffb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
@@ -96,6 +96,7 @@
name = "CcImportBaseConfiguredTargetTest",
srcs = ["CcImportBaseConfiguredTargetTest.java"],
deps = [
+ ":SkylarkCcCommonTestHelper",
"//src/main/java/com/google/devtools/build/lib:build-base",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java
index 548bf8e..41bdb56 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java
@@ -34,7 +34,9 @@
protected String skylarkImplementationLoadStatement = "";
@Before
- public void setSkylarkImplementationLoadStatement() {
+ public void setSkylarkImplementationLoadStatement() throws Exception {
+ setSkylarkSemanticsOptions(SkylarkCcCommonTestHelper.CC_SKYLARK_WHITELIST_FLAG);
+ invalidatePackages();
setIsSkylarkImplementation();
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
index 61f2058..6bd4f03 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
@@ -45,8 +45,9 @@
public class SkylarkCcCommonTest extends BuildViewTestCase {
@Before
- public void setConfiguration() throws Exception {
- useConfiguration("--experimental_enable_cc_skylark_api");
+ public void setSkylarkSemanticsOptions() throws Exception {
+ setSkylarkSemanticsOptions(SkylarkCcCommonTestHelper.CC_SKYLARK_WHITELIST_FLAG);
+ invalidatePackages();
}
@Test
@@ -134,7 +135,7 @@
AnalysisMock.get()
.ccSupport()
.setupCrosstool(mockToolsConfig, "feature { name: 'foo_feature' }");
- useConfiguration("--features=foo_feature", "--experimental_enable_cc_skylark_api");
+ useConfiguration("--features=foo_feature");
scratch.file(
"a/BUILD",
"load(':rule.bzl', 'crule')",
@@ -733,7 +734,7 @@
@Test
public void testIsLinkingDynamicLibraryLinkVariables() throws Exception {
- useConfiguration("--linkopt=-pie", "--experimental_enable_cc_skylark_api");
+ useConfiguration("--linkopt=-pie");
assertThat(
commandLineForVariables(
CppActionNames.CPP_LINK_EXECUTABLE,
@@ -758,9 +759,7 @@
@Test
public void testIsUsingLinkerLinkVariables() throws Exception {
- useConfiguration(
- "--linkopt=-i_dont_want_to_see_this_on_archiver_command_line",
- "--experimental_enable_cc_skylark_api");
+ useConfiguration("--linkopt=-i_dont_want_to_see_this_on_archiver_command_line");
assertThat(
commandLineForVariables(
CppActionNames.CPP_LINK_EXECUTABLE,
@@ -1064,15 +1063,15 @@
}
@Test
- public void testCcLinkingProviderParamsWithoutFlag() throws Exception {
- useConfiguration("--noexperimental_enable_cc_skylark_api");
- setUpCcLinkingProviderParamsTest();
+ public void testFlagWhitelist() throws Exception {
+ setSkylarkSemanticsOptions("--experimental_cc_skylark_api_enabled_packages=\"\"");
+ SkylarkCcCommonTestHelper.createFiles(scratch, "foo/bar");
reporter.removeHandler(failFastHandler);
- getConfiguredTarget("//a:r");
+ getConfiguredTarget("//foo:bin");
assertContainsEvent(
- "Pass --experimental_enable_cc_skylark_api in order to "
- + "use the C++ API. Beware that we will be making breaking changes to this API "
- + "without prior warning.");
+ "You can try it out by passing "
+ + "--experimental_cc_skylark_api_enabled_packages=<list of packages>. Beware that we "
+ + "will be making breaking changes to this API without prior warning.");
}
@Test
@@ -1082,7 +1081,7 @@
.setupCrosstool(
mockToolsConfig,
"supports_interface_shared_objects: false");
- useConfiguration("--experimental_enable_cc_skylark_api");
+ useConfiguration();
setUpCcLinkingProviderParamsTest();
ConfiguredTarget r = getConfiguredTarget("//a:r");
@@ -1251,14 +1250,6 @@
}
@Test
- public void testWhitelist() throws Exception {
- SkylarkCcCommonTestHelper.createFiles(scratch, "foo/bar");
- reporter.removeHandler(failFastHandler);
- getConfiguredTarget("//foo:bin");
- assertContainsEvent("C++ Skylark API is for the time being");
- }
-
- @Test
public void testCopts() throws Exception {
SkylarkCcCommonTestHelper.createFilesForTestingCompilation(
scratch, "tools/build_defs/foo", "copts=depset(['-COMPILATION_OPTION'])");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTestHelper.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTestHelper.java
index 1aafab1..0a4dc6b 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTestHelper.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTestHelper.java
@@ -19,6 +19,9 @@
/** Methods useful for tests testing the C++ Skylark API. */
public final class SkylarkCcCommonTestHelper {
+ public static final String CC_SKYLARK_WHITELIST_FLAG =
+ "--experimental_cc_skylark_api_enabled_packages=tools/build_defs,experimental";
+
public static void createFilesForTestingCompilation(
Scratch scratch, String bzlFilePath, String compileProviderLines) throws Exception {
createFiles(scratch, bzlFilePath, compileProviderLines, "");
