From dfa160eadc1334378a91dd90aa673ea4a83bdbfe Mon Sep 17 00:00:00 2001 From: brandjon Date: Fri, 18 Jan 2019 13:45:19 -0800 Subject: [PATCH] Implement deferred srcs_version validation When --experimental_allow_python_version_transitions is enabled, srcs_version will now be checked by py_binary/py_test instead of at every py_library. This allows building multiple py_library targets in the same invocation (e.g. via bazel build //pkg/...) regardless of their srcs_versions constraints. The check occurs at analysis time, but any failure is reported at execution time. This allows a configured target to be created for diagnostic purposes (i.e. finding the bad transitive dependency). I've updated our analysis-time tests to account for this possibility by asserting that this deferred failure does not occur. The "py" provider now has fields `has_py2_only_sources` and `has_py3_only_sources`. Will add a couple tests for accessing these from Starlark code in a follow-up CL. Work towards #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229986854 --- .../build/lib/actions/FailAction.java | 10 +- .../build/lib/rules/python/PyCommon.java | 162 +++++++++++++++++- .../build/lib/rules/python/PyExecutable.java | 3 +- .../build/lib/rules/python/PyProvider.java | 62 +++++++ .../lib/rules/python/PythonConfiguration.java | 4 +- .../lib/rules/python/PythonSemantics.java | 2 + .../devtools/build/lib/rules/python/BUILD | 1 + .../PyBaseConfiguredTargetTestBase.java | 11 +- .../python/PyBinaryConfiguredTargetTest.java | 107 +++++++----- .../PyExecutableConfiguredTargetTestBase.java | 88 +++++++++- .../python/PyLibraryConfiguredTargetTest.java | 16 ++ .../lib/rules/python/PyProviderTest.java | 54 +++++- .../shell/integration/python_stub_test.sh | 24 +++ 13 files changed, 478 insertions(+), 66 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FailAction.java b/src/main/java/com/google/devtools/build/lib/actions/FailAction.java index eccc4ee2d8d3d6..cc79541e00e22a 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FailAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FailAction.java @@ -41,6 +41,10 @@ public Artifact getPrimaryInput() { return null; } + public String getErrorMessage() { + return errorMessage; + } + @Override public ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException { @@ -54,8 +58,10 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { @Override protected String getRawProgressMessage() { - return "Building unsupported rule " + getOwner().getLabel() - + " located at " + getOwner().getLocation(); + return "Reporting failed target " + + getOwner().getLabel() + + " located at " + + getOwner().getLocation(); } @Override 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 index f25cd7d5fb17ea..05a5a417d6ad88 100644 --- 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 @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FailAction; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.actions.extra.PythonInfo; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; @@ -28,6 +29,7 @@ import com.google.devtools.build.lib.analysis.PseudoAction; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.Util; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; @@ -40,7 +42,9 @@ import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.packages.StructImpl; +import com.google.devtools.build.lib.rules.cpp.CcInfo; import com.google.devtools.build.lib.rules.cpp.CppFileTypes; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.syntax.EvalException; @@ -107,6 +111,18 @@ public void collectMetadataArtifacts(Iterable artifacts, /** Extra Python module import paths propagated or used by this target. */ private final NestedSet imports; + /** + * Whether any of this target's transitive {@code deps} have PY2-only source files, including this + * target itself. + */ + private final boolean hasPy2OnlySources; + + /** + * Whether any of this target's transitive {@code deps} have PY3-only source files, including this + * target itself. + */ + private final boolean hasPy3OnlySources; + /** * Symlink map from root-relative paths to 2to3 converted source artifacts. * @@ -133,8 +149,10 @@ public PyCommon(RuleContext ruleContext, PythonSemantics semantics) { this.transitivePythonSources = initTransitivePythonSources(ruleContext); this.usesSharedLibraries = initUsesSharedLibraries(ruleContext); this.imports = initImports(ruleContext, semantics); + this.hasPy2OnlySources = initHasPy2OnlySources(ruleContext, this.sourcesVersion); + this.hasPy3OnlySources = initHasPy3OnlySources(ruleContext, this.sourcesVersion); this.convertedFiles = makeAndInitConvertedFiles(ruleContext, version, this.sourcesVersion); - validateSourceIsCompatible(); + maybeValidateVersionCompatibleWithOwnSourcesAttr(); validatePythonVersionAttr(); } @@ -268,6 +286,53 @@ private static NestedSet initImports(RuleContext ruleContext, PythonSema return builder.build(); } + /** + * Returns true if any of {@code deps} has a py provider with {@code has_py2_only_sources} set, or + * this target has a {@code srcs_version} of {@code PY2ONLY}. + */ + // TODO(#1393): For Bazel, deprecate 2to3 support and treat PY2 the same as PY2ONLY. + private static boolean initHasPy2OnlySources( + RuleContext ruleContext, PythonVersion sourcesVersion) { + if (sourcesVersion == PythonVersion.PY2ONLY) { + return true; + } + for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps", Mode.TARGET)) { + if (PyProvider.hasProvider(dep)) { + try { + if (PyProvider.getHasPy2OnlySources(PyProvider.getProvider(dep))) { + return true; + } + } catch (EvalException e) { + ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + } + } + } + return false; + } + + /** + * Returns true if any of {@code deps} has a py provider with {@code has_py3_only_sources} set, or + * this target has {@code srcs_version} of {@code PY3} or {@code PY3ONLY}. + */ + private static boolean initHasPy3OnlySources( + RuleContext ruleContext, PythonVersion sourcesVersion) { + if (sourcesVersion == PythonVersion.PY3 || sourcesVersion == PythonVersion.PY3ONLY) { + return true; + } + for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps", Mode.TARGET)) { + if (PyProvider.hasProvider(dep)) { + try { + if (PyProvider.getHasPy3OnlySources(PyProvider.getProvider(dep))) { + return true; + } + } catch (EvalException e) { + ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + } + } + } + return false; + } + /** * If 2to3 conversion is to be done, creates the 2to3 actions and returns the map of converted * files; otherwise returns null. @@ -288,8 +353,22 @@ private static Map makeAndInitConvertedFiles( } } - /** Checks that the source file version is compatible with the Python interpreter. */ - private void validateSourceIsCompatible() { + /** + * Under the old version semantics ({@code + * --experimental_allow_python_version_transitions=false}), checks that the {@code srcs_version} + * attribute is compatible with the Python version as determined by the configuration. + * + *

A failure is reported as a rule error. + * + *

This check is local to the current target and intended to be enforced by each {@code + * py_library} up the dependency chain. + * + *

No-op under the new version semantics. + */ + private void maybeValidateVersionCompatibleWithOwnSourcesAttr() { + if (ruleContext.getFragment(PythonConfiguration.class).useNewPyVersionSemantics()) { + return; + } // Treat PY3 as PY3ONLY: we'll never implement 3to2. if ((version == PythonVersion.PY2 || version == PythonVersion.PY2AND3) && (sourcesVersion == PythonVersion.PY3 || sourcesVersion == PythonVersion.PY3ONLY)) { @@ -324,10 +403,53 @@ private void validatePythonVersionAttr() { } } + /** + * Under the new version semantics ({@code --experimental_allow_python_version_transitions=true}), + * if the Python version (as determined by the configuration) is inconsistent with {@link + * #hasPy2OnlySources} or {@link #hasPy3OnlySources}, emits a {@link FailAction} that "generates" + * the executable. + * + *

If the version is consistent, or if we are using the old semantics, no such action is + * emitted. + * + *

We use a {@code FailAction} rather than a rule error because we want to defer the error + * until the execution phase. This way, we still get a configured target that the user can query + * over with an aspect to find the exact transitive dependency that introduced the offending + * version constraint. + * + * @return true if a {@link FailAction} was created + */ + private boolean maybeCreateFailActionDueToTransitiveSourcesVersion() { + if (!ruleContext.getFragment(PythonConfiguration.class).useNewPyVersionSemantics()) { + return false; + } + // TODO(brandjon): Add hints to the error message about how to locate the offending + // dependencies. + String error = null; + if (version == PythonVersion.PY2 && hasPy3OnlySources) { + error = + "target is being built for Python 2 but (transitively) includes Python 3-only sources"; + } else if (version == PythonVersion.PY3 && hasPy2OnlySources) { + error = + "target is being built for Python 3 but (transitively) includes Python 2-only sources"; + } + if (error == null) { + return false; + } else { + ruleContext.registerAction( + new FailAction(ruleContext.getActionOwner(), ImmutableList.of(executable), error)); + return true; + } + } + public PythonVersion getVersion() { return version; } + public PythonVersion getSourcesVersion() { + return sourcesVersion; + } + /** * Returns the transitive Python sources collected from the deps attribute, not including sources * from the srcs attribute (unless they were separately reached via deps). @@ -349,6 +471,14 @@ public NestedSet getImports() { return imports; } + public boolean hasPy2OnlySources() { + return hasPy2OnlySources; + } + + public boolean hasPy3OnlySources() { + return hasPy3OnlySources; + } + public Map getConvertedFiles() { return convertedFiles; } @@ -413,6 +543,8 @@ public void addCommonTransitiveInfoProviders( .setTransitiveSources(transitivePythonSources) .setUsesSharedLibraries(usesSharedLibraries) .setImports(imports) + .setHasPy2OnlySources(hasPy2OnlySources) + .setHasPy3OnlySources(hasPy3OnlySources) .build()) // Python targets are not really compilable. The best we can do is make sure that all // generated source files are ready. @@ -562,6 +694,30 @@ public NestedSet getFilesToBuild() { return filesToBuild; } + /** + * Creates the actual executable artifact, i.e., emits a generating action for {@link + * #getExecutable()}. + * + *

If there is a transitive sources version conflict, may produce a {@link FailAction} to + * trigger an execution-time failure. See {@link + * #maybeCreateFailActionDueToTransitiveSourcesVersion}. + */ + public Artifact createExecutable( + CcInfo ccInfo, NestedSet givenImports, Runfiles.Builder defaultRunfilesBuilder) + throws InterruptedException, RuleErrorException { + boolean failed = maybeCreateFailActionDueToTransitiveSourcesVersion(); + if (failed) { + return executable; + } else { + // TODO(#7054): We pass imports as an arg instead of taking them from the PyCommon field + // because the imports logic is a little inconsistent, and passing it explicitly may help + // avoid creating bugs that make the situation worse. We can eliminate this arg when we + // straighten up the other imports logic. + return semantics.createExecutable( + ruleContext, this, ccInfo, givenImports, defaultRunfilesBuilder); + } + } + private static String buildMultipleMainMatchesErrorText(boolean explicit, String proposedMainName, String match1, String match2) { String errorText; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyExecutable.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyExecutable.java index 78ffc98c031b06..2fc9974889c695 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyExecutable.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyExecutable.java @@ -74,8 +74,7 @@ public ConfiguredTarget create(RuleContext ruleContext) .merge(commonRunfiles); semantics.collectDefaultRunfilesForBinary(ruleContext, defaultRunfilesBuilder); - Artifact realExecutable = - semantics.createExecutable(ruleContext, common, ccInfo, imports, defaultRunfilesBuilder); + Artifact realExecutable = common.createExecutable(ccInfo, imports, defaultRunfilesBuilder); Runfiles defaultRunfiles = defaultRunfilesBuilder.build(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java index fb6e3f609e15f5..92e2e279a6bd46 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java @@ -60,6 +60,18 @@ private PyProvider() {} // with the one expected by the rules. public static final String IMPORTS = "imports"; + /** + * Name of field holding a boolean indicating whether there are any transitive sources that + * require a Python 2 runtime. + */ + public static final String HAS_PY2_ONLY_SOURCES = "has_py2_only_sources"; + + /** + * Name of field holding a boolean indicating whether there are any transitive sources that + * require a Python 3 runtime. + */ + public static final String HAS_PY3_ONLY_SOURCES = "has_py3_only_sources"; + private static final ImmutableMap DEFAULTS; static { @@ -69,6 +81,8 @@ private PyProvider() {} builder.put( IMPORTS, SkylarkNestedSet.of(String.class, NestedSetBuilder.compileOrder().build())); + builder.put(HAS_PY2_ONLY_SOURCES, false); + builder.put(HAS_PY3_ONLY_SOURCES, false); DEFAULTS = builder.build(); } @@ -160,6 +174,40 @@ public static NestedSet getImports(StructImpl info) throws EvalException return castValue.getSet(String.class); } + /** + * Casts and returns the py2-only-sources field (or its default value). + * + * @throws EvalException if the field exists and is not a boolean + */ + public static boolean getHasPy2OnlySources(StructImpl info) throws EvalException { + Object fieldValue = getValue(info, HAS_PY2_ONLY_SOURCES); + return SkylarkType.cast( + fieldValue, + Boolean.class, + null, + "'%s' provider's '%s' field should be a boolean (got a '%s')", + PROVIDER_NAME, + HAS_PY2_ONLY_SOURCES, + EvalUtils.getDataTypeNameFromClass(fieldValue.getClass())); + } + + /** + * Casts and returns the py3-only-sources field (or its default value). + * + * @throws EvalException if the field exists and is not a boolean + */ + public static boolean getHasPy3OnlySources(StructImpl info) throws EvalException { + Object fieldValue = getValue(info, HAS_PY3_ONLY_SOURCES); + return SkylarkType.cast( + fieldValue, + Boolean.class, + null, + "'%s' provider's '%s' field should be a boolean (got a '%s')", + PROVIDER_NAME, + HAS_PY3_ONLY_SOURCES, + EvalUtils.getDataTypeNameFromClass(fieldValue.getClass())); + } + public static Builder builder() { return new Builder(); } @@ -169,6 +217,8 @@ public static class Builder { SkylarkNestedSet transitiveSources = null; Boolean usesSharedLibraries = null; SkylarkNestedSet imports = null; + Boolean hasPy2OnlySources = null; + Boolean hasPy3OnlySources = null; // Use the static builder() method instead. private Builder() {} @@ -188,6 +238,16 @@ public Builder setImports(NestedSet imports) { return this; } + public Builder setHasPy2OnlySources(boolean hasPy2OnlySources) { + this.hasPy2OnlySources = hasPy2OnlySources; + return this; + } + + public Builder setHasPy3OnlySources(boolean hasPy3OnlySources) { + this.hasPy3OnlySources = hasPy3OnlySources; + return this; + } + private static void put( ImmutableMap.Builder fields, String fieldName, Object value) { fields.put(fieldName, value != null ? value : DEFAULTS.get(fieldName)); @@ -199,6 +259,8 @@ public StructImpl build() { put(fields, TRANSITIVE_SOURCES, transitiveSources); put(fields, USES_SHARED_LIBRARIES, usesSharedLibraries); put(fields, IMPORTS, imports); + put(fields, HAS_PY2_ONLY_SOURCES, hasPy2OnlySources); + put(fields, HAS_PY3_ONLY_SOURCES, hasPy3OnlySources); return StructProvider.STRUCT.create(fields.build(), "No such attribute '%s'"); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index 7f2b9a04cb3d05..cefd6659895dda 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -47,12 +47,12 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { private final boolean useNewPyVersionSemantics; PythonConfiguration( - PythonVersion defaultPythonVersion, + PythonVersion version, TriState buildPythonZip, boolean buildTransitiveRunfilesTrees, boolean oldPyVersionApiAllowed, boolean useNewPyVersionSemantics) { - this.version = defaultPythonVersion; + this.version = version; this.buildPythonZip = buildPythonZip; this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees; this.oldPyVersionApiAllowed = oldPyVersionApiAllowed; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java index 2f37f56570ae8b..4de2239ee76fa8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java @@ -75,6 +75,8 @@ Collection precompiledPythonFiles( * *

This should create a generating action for {@code common.getExecutable()}. */ + // TODO(brandjon): I believe this always returns common.getExecutable(), so we should be able to + // eliminate the return as redundant. Artifact createExecutable( RuleContext ruleContext, PyCommon common, diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD index 186a90877ce272..18b32986863573 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD @@ -85,6 +85,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:bazel-rules", "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:python-rules", + "//src/main/java/com/google/devtools/build/lib/actions", "//third_party:guava", "//third_party:junit4", "//third_party:truth", diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java index d7d1a96263041f..c5579d41b4cdbd 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java @@ -67,8 +67,15 @@ public void goodSrcsVersionValue() throws Exception { } @Test - public void srcsVersionClashesWithForcePythonFlag() throws Exception { - useConfiguration("--force_python=PY3"); + public void srcsVersionClashesWithForcePythonFlagUnderOldSemantics() throws Exception { + // Under the old version semantics, we fail on any Python target the moment a conflict between + // srcs_version and the configuration is detected. Under the new semantics, py_binary and + // py_test care if there's a conflict but py_library does not. This test case checks the old + // semantics; the new semantics are checked in PyLibraryConfiguredTargetTest and + // PyExecutableConfiguredTargetTestBase. Note that under the new semantics py_binary and + // py_library ignore the version flag, so those tests use the attribute to set the version + // instead. + useConfiguration("--experimental_allow_python_version_transitions=false", "--force_python=PY3"); checkError("pkg", "foo", // error: "'//pkg:foo' can only be used with Python 2", diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyBinaryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyBinaryConfiguredTargetTest.java index 6e9a7275c7e4a9..31309aa72c6b13 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyBinaryConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyBinaryConfiguredTargetTest.java @@ -31,59 +31,72 @@ public PyBinaryConfiguredTargetTest() { super("py_binary"); } + /** + * Creates a target //pkg:bin with the given version attr and that depends on a target //pkg:lib + * having the given sources version attr. + */ + private void declareBinDependingOnLibWithVersions(String binVersion, String libSrcsVersion) + throws Exception { + scratch.file( + "pkg/BUILD", + "py_library(name = 'lib',", + " srcs = [],", + " srcs_version = '" + libSrcsVersion + "')", + "py_binary(name = 'bin',", + " srcs = ['bin.py'],", + " deps = [':lib'],", + " default_python_version = '" + binVersion + "')"); + } + @Test - public void python2WithPy3SrcsVersionDependency() throws Exception { - eventCollector.clear(); + public void python2WithPy3SrcsVersionDependency_OldSemantics() throws Exception { reporter.removeHandler(failFastHandler); // expect errors - scratch.file("python2/BUILD", - "py_library(name = 'py3lib',", - " srcs = [],", - " srcs_version = 'PY3')", - "py_binary(name = 'pybin',", - " srcs = ['pybin.py'],", - " deps = [':py3lib'],", - " srcs_version = 'PY2',", - " default_python_version = 'PY2')"); - assertThat(view.hasErrors(getConfiguredTarget("//python2:pybin"))).isTrue(); - assertContainsEvent("//python2:py3lib: Rule '//python2:py3lib' can only be used with Python 3"); + useConfiguration("--experimental_allow_python_version_transitions=false"); + declareBinDependingOnLibWithVersions("PY2", "PY3"); + assertThat(view.hasErrors(getConfiguredTarget("//pkg:bin"))).isTrue(); + assertContainsEvent("//pkg:lib: Rule '//pkg:lib' can only be used with Python 3"); + } + + @Test + public void python2WithPy3SrcsVersionDependency_NewSemantics() throws Exception { + useConfiguration("--experimental_allow_python_version_transitions=true"); + declareBinDependingOnLibWithVersions("PY2", "PY3"); + assertThat(getPyExecutableDeferredError("//pkg:bin")) + .contains("being built for Python 2 but (transitively) includes Python 3-only sources"); } @Test - public void python2WithPy3OnlySrcsVersionDependency() throws Exception { - eventCollector.clear(); + public void python2WithPy3OnlySrcsVersionDependency_OldSemantics() throws Exception { reporter.removeHandler(failFastHandler); // expect errors - scratch.file("python2/BUILD", - "py_library(", - " name = 'py3lib',", - " srcs = [],", - " srcs_version = 'PY3ONLY')", - "py_binary(", - " name = 'pybin',", - " srcs = ['pybin.py'],", - " deps = [':py3lib'],", - " srcs_version = 'PY2',", - " default_python_version = 'PY2')"); - assertThat(view.hasErrors(getConfiguredTarget("//python2:pybin"))).isTrue(); - assertContainsEvent("//python2:py3lib: Rule '//python2:py3lib' can only be used with Python 3"); + useConfiguration("--experimental_allow_python_version_transitions=false"); + declareBinDependingOnLibWithVersions("PY2", "PY3ONLY"); + assertThat(view.hasErrors(getConfiguredTarget("//pkg:bin"))).isTrue(); + assertContainsEvent("//pkg:lib: Rule '//pkg:lib' can only be used with Python 3"); } @Test - public void python3WithPy2OnlySrcsVersionDependency() throws Exception { - eventCollector.clear(); + public void python2WithPy3OnlySrcsVersionDependency_NewSemantics() throws Exception { + useConfiguration("--experimental_allow_python_version_transitions=true"); + declareBinDependingOnLibWithVersions("PY2", "PY3ONLY"); + assertThat(getPyExecutableDeferredError("//pkg:bin")) + .contains("being built for Python 2 but (transitively) includes Python 3-only sources"); + } + + @Test + public void python3WithPy2OnlySrcsVersionDependency_OldSemantics() throws Exception { reporter.removeHandler(failFastHandler); // expect errors - scratch.file("python3/BUILD", - "py_library(", - " name = 'py2lib',", - " srcs = [],", - " srcs_version = 'PY2ONLY')", - "py_binary(", - " name = 'pybin',", - " srcs = ['pybin.py'],", - " deps = [':py2lib'],", - " srcs_version = 'PY3',", - " default_python_version = 'PY3')"); - assertThat(view.hasErrors(getConfiguredTarget("//python3:pybin"))).isTrue(); - assertContainsEvent("//python3:py2lib: Rule '//python3:py2lib' can only be used with Python 2"); + useConfiguration("--experimental_allow_python_version_transitions=false"); + declareBinDependingOnLibWithVersions("PY3", "PY2ONLY"); + assertThat(view.hasErrors(getConfiguredTarget("//pkg:bin"))).isTrue(); + assertContainsEvent("//pkg:lib: Rule '//pkg:lib' can only be used with Python 2"); + } + + @Test + public void python3WithPy2OnlySrcsVersionDependency_NewSemantics() throws Exception { + useConfiguration("--experimental_allow_python_version_transitions=true"); + declareBinDependingOnLibWithVersions("PY3", "PY2ONLY"); + assertThat(getPyExecutableDeferredError("//pkg:bin")) + .contains("being built for Python 3 but (transitively) includes Python 2-only sources"); } @Test @@ -92,7 +105,7 @@ public void filesToBuild() throws Exception { "py_binary(", " name = 'foo',", " srcs = ['foo.py'])"); - ConfiguredTarget target = getConfiguredTarget("//pkg:foo"); + ConfiguredTarget target = getOkPyTarget("//pkg:foo"); FileConfiguredTarget srcFile = getFileConfiguredTarget("//pkg:foo.py"); assertThat(getFilesToBuild(target)) .containsExactly(getExecutable(target), srcFile.getArtifact()); @@ -134,7 +147,7 @@ public void explicitMain() throws Exception { " name = 'foo',", " main = 'foo.py',", " srcs = ['foo.py', 'bar.py'])"); - getConfiguredTarget("//pkg:foo"); // should not fail + getOkPyTarget("//pkg:foo"); // should not fail } @Test @@ -203,7 +216,7 @@ public void defaultMainCanBeGenerated() throws Exception { "py_binary(", " name = 'foo',", " srcs = [':gen_py'])"); - getConfiguredTarget("//pkg:foo"); // should not fail + getOkPyTarget("//pkg:foo"); // should not fail } @Test @@ -213,7 +226,7 @@ public void defaultMainCanHaveMultiplePathSegments() throws Exception { "py_binary(", " name = 'foo/bar',", " srcs = ['foo/bar.py'])"); - getConfiguredTarget("//pkg:foo/bar"); // should not fail + getOkPyTarget("//pkg:foo/bar"); // should not fail } // TODO(brandjon): Add tests for content of stub Python script (particularly for choosing python diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java index a4d48d0bc3d802..9c8b03257b48b7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java @@ -19,6 +19,11 @@ import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FailAction; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; import org.junit.Test; /** Tests that are common to {@code py_binary} and {@code py_test}. */ @@ -31,6 +36,49 @@ protected PyExecutableConfiguredTargetTestBase(String ruleName) { this.ruleName = ruleName; } + /** + * Returns the configured target with the given label while asserting that, if it is an executable + * target, the executable is not produced by {@link FailAction}. + * + *

This serves as a drop-in replacement for {@link #getConfiguredTarget} that will also catch + * unexpected deferred failures (e.g. {@code srcs_versions} validation failures) in {@code + * py_binary} and {@code py_library} targets. + */ + protected ConfiguredTarget getOkPyTarget(String label) throws Exception { + ConfiguredTarget target = getConfiguredTarget(label); + // It can be null without events due to b/26382502. + Preconditions.checkNotNull(target, "target was null (is it misspelled or in error?)"); + Artifact executable = getExecutable(target); + if (executable != null) { + Action action = getGeneratingAction(executable); + if (action instanceof FailAction) { + throw new AssertionError( + String.format( + "execution of target would fail with error '%s'", + ((FailAction) action).getErrorMessage())); + } + } + return target; + } + + /** + * Gets the configured target for an executable Python rule (generally {@code py_binary} or {@code + * py_library}) and asserts that it produces a deferred error via {@link FailAction}. + * + * @return the deferred error string + */ + protected String getPyExecutableDeferredError(String label) throws Exception { + ConfiguredTarget target = getConfiguredTarget(label); + // It can be null without events due to b/26382502. + Preconditions.checkNotNull(target, "target was null (is it misspelled or in error?)"); + Artifact executable = getExecutable(target); + Preconditions.checkNotNull( + executable, "executable was null (is this a py_binary/py_test target?)"); + Action action = getGeneratingAction(executable); + assertThat(action).isInstanceOf(FailAction.class); + return ((FailAction) action).getErrorMessage(); + } + /** * Sets the configuration, then asserts that a configured target has the given Python version. * @@ -39,7 +87,7 @@ protected PyExecutableConfiguredTargetTestBase(String ruleName) { protected void assertPythonVersionIs_UnderNewConfig( String targetName, PythonVersion version, String... flags) throws Exception { useConfiguration(flags); - assertThat(getPythonVersion(getConfiguredTarget(targetName))).isEqualTo(version); + assertThat(getPythonVersion(getOkPyTarget(targetName))).isEqualTo(version); } /** @@ -54,7 +102,7 @@ protected void assertPythonVersionIs_UnderNewConfigs( for (String[] config : configs) { useConfiguration(config); assertWithMessage(String.format("Under config '%s'", Joiner.on(" ").join(config))) - .that(getPythonVersion(getConfiguredTarget(targetName))) + .that(getPythonVersion(getOkPyTarget(targetName))) .isEqualTo(version); } } @@ -135,14 +183,14 @@ public void newVersionAttr_BadValue() throws Exception { public void oldVersionAttr_GoodValue() throws Exception { useConfiguration("--experimental_remove_old_python_version_api=false"); scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2")); - getConfiguredTarget("//pkg:foo"); + getOkPyTarget("//pkg:foo"); assertNoEvents(); } @Test public void newVersionAttr_GoodValue() throws Exception { scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); - getConfiguredTarget("//pkg:foo"); + getOkPyTarget("//pkg:foo"); assertNoEvents(); } @@ -303,8 +351,11 @@ public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToNonDefa } @Test - public void srcsVersionClashesWithVersionAttr() throws Exception { - checkError("pkg", "foo", + public void incompatibleSrcsVersion_OldSemantics() throws Exception { + useConfiguration("--experimental_allow_python_version_transitions=false"); + checkError( + "pkg", + "foo", // error: "'//pkg:foo' can only be used with Python 2", // build file: @@ -316,8 +367,31 @@ public void srcsVersionClashesWithVersionAttr() throws Exception { } @Test - public void srcsVersionClashesWithVersionAttr_Implicitly() throws Exception { + public void incompatibleSrcsVersion_NewSemantics() throws Exception { + reporter.removeHandler(failFastHandler); // We assert below that we don't fail at analysis. + useConfiguration("--experimental_allow_python_version_transitions=true"); + scratch.file( + "pkg/BUILD", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = [':foo.py'],", + " srcs_version = 'PY2ONLY',", + " default_python_version = 'PY3')"); + // Under the new semantics, this is an execution-time error, not an analysis-time one. We fail + // by setting the generating action to FailAction. + assertNoEvents(); + assertThat(getPyExecutableDeferredError("//pkg:foo")) + .contains("being built for Python 3 but (transitively) includes Python 2-only sources"); + } + + @Test + public void incompatibleSrcsVersion_DueToVersionAttrDefault() throws Exception { ensureDefaultIsPY2(); // When changed to PY3, flip srcs_version below to be PY2ONLY. + + // This test doesn't care whether we use old and new semantics, but it affects how we assert. + useConfiguration("--experimental_allow_python_version_transitions=false"); + // Fails because default_python_version is PY2 by default, so the config is set to PY2 // regardless of srcs_version. checkError("pkg", "foo", diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyLibraryConfiguredTargetTest.java index 30a44522858ec2..1c39537c0c05cd 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyLibraryConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyLibraryConfiguredTargetTest.java @@ -33,6 +33,22 @@ public PyLibraryConfiguredTargetTest() { super("py_library"); } + @Test + public void canBuildWithIncompatibleSrcsVersionUnderNewSemantics() throws Exception { + // See PyBaseConfiguredTargetTestBase for the analogous test under the old semantics, which + // applies not just to py_library but also to py_binary and py_test. + useConfiguration("--experimental_allow_python_version_transitions=true", "--force_python=PY3"); + scratch.file( + "pkg/BUILD", + "py_library(", + " name = 'foo',", + " srcs = [':foo.py'],", + " srcs_version = 'PY2ONLY')"); + // Under the new semantics, errors are only reported at the binary target, not the library, and + // even then they'd be deferred to execution time, so there should be nothing wrong here. + assertThat(view.hasErrors(getConfiguredTarget("//pkg:foo"))).isFalse(); + } + @Test public void versionIs3IfSetByFlagUnderNewSemantics() throws Exception { // See PyBaseConfiguredTargetTestBase for the analogous test under the old semantics, which diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java index d91a657fed8505..b50bd26965a30e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java @@ -57,6 +57,8 @@ private StructImpl makeStruct(Map overrides) { fields.put( PyProvider.IMPORTS, SkylarkNestedSet.of(String.class, NestedSetBuilder.emptySet(Order.COMPILE_ORDER))); + fields.put(PyProvider.HAS_PY2_ONLY_SOURCES, false); + fields.put(PyProvider.HAS_PY3_ONLY_SOURCES, false); fields.putAll(overrides); return StructProvider.STRUCT.create(fields, "No such attribute '%s'"); } @@ -71,6 +73,8 @@ private void definePyTarget() throws IOException { " transitive_sources = depset(direct=ctx.files.transitive_sources),", " uses_shared_libraries = ctx.attr.uses_shared_libraries,", " imports = depset(direct=ctx.attr.imports),", + " has_py2_only_sources = ctx.attr.has_py2_only_sources,", + " has_py3_only_sources = ctx.attr.has_py3_only_sources,", " )", " return struct(py=info)", "", @@ -80,6 +84,8 @@ private void definePyTarget() throws IOException { " 'transitive_sources': attr.label_list(allow_files=True),", " 'uses_shared_libraries': attr.bool(default=False),", " 'imports': attr.string_list(),", + " 'has_py2_only_sources': attr.bool(),", + " 'has_py3_only_sources': attr.bool(),", " },", ")"); scratch.file( @@ -90,7 +96,9 @@ private void definePyTarget() throws IOException { " name = 'pytarget',", " transitive_sources = ['a.py'],", " uses_shared_libraries = True,", - " imports = ['b']", + " imports = ['b'],", + " has_py2_only_sources = True,", + " has_py3_only_sources = True,", ")"); scratch.file("pytarget/a.py"); } @@ -296,6 +304,44 @@ public void getImports_WrongType() { assertHasWrongTypeMessage(() -> PyProvider.getImports(info), "imports", "depset of strings"); } + @Test + public void getHasPy2OnlySources_Good() throws Exception { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.HAS_PY2_ONLY_SOURCES, true)); + assertThat(PyProvider.getHasPy2OnlySources(info)).isTrue(); + } + + @Test + public void getHasPy2OnlySources_Missing() throws Exception { + StructImpl info = StructProvider.STRUCT.createEmpty(null); + assertThat(PyProvider.getHasPy2OnlySources(info)).isFalse(); + } + + @Test + public void getHasPy2OnlySources_WrongType() { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.HAS_PY2_ONLY_SOURCES, 123)); + assertHasWrongTypeMessage( + () -> PyProvider.getHasPy2OnlySources(info), "has_py2_only_sources", "boolean"); + } + + @Test + public void getHasPy3OnlySources_Good() throws Exception { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.HAS_PY3_ONLY_SOURCES, true)); + assertThat(PyProvider.getHasPy3OnlySources(info)).isTrue(); + } + + @Test + public void getHasPy3OnlySources_Missing() throws Exception { + StructImpl info = StructProvider.STRUCT.createEmpty(null); + assertThat(PyProvider.getHasPy3OnlySources(info)).isFalse(); + } + + @Test + public void getHasPy3OnlySources_WrongType() { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.HAS_PY3_ONLY_SOURCES, 123)); + assertHasWrongTypeMessage( + () -> PyProvider.getHasPy3OnlySources(info), "has_py3_only_sources", "boolean"); + } + /** Checks values set by the builder. */ @Test public void builder() throws Exception { @@ -307,6 +353,8 @@ public void builder() throws Exception { .setTransitiveSources(sources) .setUsesSharedLibraries(true) .setImports(imports) + .setHasPy2OnlySources(true) + .setHasPy3OnlySources(true) .build(); // Assert using struct operations, not PyProvider accessors, which aren't necessarily trusted to // be correct. @@ -319,6 +367,8 @@ public void builder() throws Exception { ((SkylarkNestedSet) info.getValue(PyProvider.IMPORTS)).getSet(String.class), Order.COMPILE_ORDER, "abc"); + assertThat((Boolean) info.getValue(PyProvider.HAS_PY2_ONLY_SOURCES)).isTrue(); + assertThat((Boolean) info.getValue(PyProvider.HAS_PY3_ONLY_SOURCES)).isTrue(); } /** Checks the defaults set by the builder. */ @@ -334,5 +384,7 @@ public void builderDefaults() throws Exception { assertHasOrderAndContainsExactly( ((SkylarkNestedSet) info.getValue(PyProvider.IMPORTS)).getSet(String.class), Order.COMPILE_ORDER); + assertThat((Boolean) info.getValue(PyProvider.HAS_PY2_ONLY_SOURCES)).isFalse(); + assertThat((Boolean) info.getValue(PyProvider.HAS_PY3_ONLY_SOURCES)).isFalse(); } } diff --git a/src/test/shell/integration/python_stub_test.sh b/src/test/shell/integration/python_stub_test.sh index 09abb45e0a785b..2be52481cc56cc 100755 --- a/src/test/shell/integration/python_stub_test.sh +++ b/src/test/shell/integration/python_stub_test.sh @@ -105,4 +105,28 @@ EOF expect_log "I am Python 3" } +function test_can_build_py_library_at_top_level_regardless_of_version() { + mkdir -p test + cat > test/BUILD << EOF +py_library( + name = "lib2", + srcs = ["lib2.py"], + srcs_version = "PY2ONLY", +) +py_library( + name = "lib3", + srcs = ["lib3.py"], + srcs_version = "PY3ONLY", +) +EOF + touch test/lib2.py test/lib3.py + + EXPFLAG="--experimental_allow_python_version_transitions=true" + + bazel build --python_version=PY2 $EXPFLAG //test:* \ + &> $TEST_log || fail "bazel build failed" + bazel build --python_version=PY3 $EXPFLAG //test:* \ + &> $TEST_log || fail "bazel build failed" +} + run_suite "Tests for the Python rules without Python execution"