From d1fb6ddfb8e38ed687fd7c52d8176087ebcc5409 Mon Sep 17 00:00:00 2001 From: brandjon Date: Tue, 18 Dec 2018 12:51:19 -0800 Subject: [PATCH] Update clients of PythonOptions to respect old/new version semantics Clients that care about the Python version mode should use getters/setters that respect `--experimental_better_python_version_mixing`. This ensures that the correct field is updated and the correct transition semantics are used. select()s that examine "force_python" will still see the raw value of that flag. For partial compatibility we still update this flag when transitioning the Python version, but targets that are not underneath py_binary/py_test targets may still observe "force_python" values that are inconsistent with the actual Python mode. The future fix will be to use a new feature flag instead of any native flag. Work toward #6583, #6868. Design doc for new semantics here: https://github.com/bazelbuild/rules_python/blob/master/proposals/2018-10-25-selecting-between-python-2-and-3.md RELNOTES: None PiperOrigin-RevId: 226043392 --- .../analysis/config/BuildConfiguration.java | 9 - .../build/lib/rules/python/PyCommon.java | 11 +- .../lib/rules/python/PythonConfiguration.java | 63 ++-- .../python/PythonConfigurationLoader.java | 2 - .../build/lib/rules/python/PythonOptions.java | 34 ++- .../build/lib/rules/python/PythonVersion.java | 5 + .../rules/python/PythonVersionTransition.java | 53 ++-- .../devtools/build/lib/rules/python/BUILD | 16 + .../PyBaseConfiguredTargetTestBase.java | 13 +- .../python/PyBinaryConfiguredTargetTest.java | 6 +- .../PyExecutableConfiguredTargetTestBase.java | 235 +++++++++------ .../python/PyLibraryConfiguredTargetTest.java | 15 + .../rules/python/PythonConfigurationTest.java | 86 +++--- .../lib/rules/python/PythonTestUtils.java | 41 +++ src/test/shell/bazel/python_version_test.sh | 280 +++++++++++++++++- 15 files changed, 644 insertions(+), 225 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/rules/python/PythonTestUtils.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 4b9bb947014dc9..26dec354a303fb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -145,15 +145,6 @@ public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOption public String getOutputDirectoryName() { return null; } - - /** - * Returns { 'option name': 'alternative default' } entries for options where the - * "real default" should be something besides the default specified in the {@link Option} - * declaration. - */ - public Map lateBoundOptionDefaults() { - return ImmutableMap.of(); - } } public static final Label convertOptionsLabel(String input) throws OptionsParsingException { 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 761fe7f9fc4da4..ea30b597d7566b 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 @@ -96,8 +96,7 @@ public PyCommon(RuleContext ruleContext) { public void initCommon(PythonVersion defaultVersion) { this.sourcesVersion = getSrcsVersionAttr(ruleContext); - this.version = ruleContext.getFragment(PythonConfiguration.class) - .getPythonVersion(defaultVersion); + this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion(); this.transitivePythonSources = collectTransitivePythonSources(); checkSourceIsCompatible(this.version, this.sourcesVersion, ruleContext.getLabel()); } @@ -116,10 +115,6 @@ public void initBinary(List srcs) { } else { executable = ruleContext.createOutputArtifact(); } - if (this.version == PythonVersion.PY2AND3) { - // TODO(bazel-team): we need to create two actions - ruleContext.ruleError("PY2AND3 is not yet implemented"); - } NestedSetBuilder filesToBuildBuilder = NestedSetBuilder.stableOrder().addAll(srcs).add(executable); @@ -194,6 +189,10 @@ public static StructImpl createSourceProvider( "No such attribute '%s'"); } + /** + * Returns the parsed value of the "default_python_version" attribute, or null if it is not + * defined for this rule class. + */ public PythonVersion getDefaultPythonVersion() { return ruleContext.getRule().isAttrDefined("default_python_version", Type.STRING) ? getPythonVersionAttr(ruleContext) 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 b1d9a40e7456ce..621afa58c4dd3d 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 @@ -11,9 +11,11 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + package com.google.devtools.build.lib.rules.python; -import com.google.common.base.Joiner; +import com.google.common.base.Ascii; +import com.google.common.base.Preconditions; import com.google.common.base.Verify; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; @@ -24,7 +26,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.util.OS; import com.google.devtools.common.options.TriState; -import java.util.List; /** * The configuration fragment containing information about the various pieces of infrastructure @@ -33,59 +34,63 @@ @Immutable @SkylarkModule( name = "py", - doc = "A configuration fragment for SWIG.", + doc = "A configuration fragment for Python.", category = SkylarkModuleCategory.CONFIGURATION_FRAGMENT) public class PythonConfiguration extends BuildConfiguration.Fragment { - private final boolean ignorePythonVersionAttribute; - private final PythonVersion defaultPythonVersion; + + private final PythonVersion version; private final TriState buildPythonZip; private final boolean buildTransitiveRunfilesTrees; PythonConfiguration( PythonVersion defaultPythonVersion, - boolean ignorePythonVersionAttribute, TriState buildPythonZip, boolean buildTransitiveRunfilesTrees) { - this.ignorePythonVersionAttribute = ignorePythonVersionAttribute; - this.defaultPythonVersion = defaultPythonVersion; + this.version = defaultPythonVersion; this.buildPythonZip = buildPythonZip; this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees; } /** - * Returns the Python version to use. Command-line flag --force_python overrides - * the rule default, given as argument. + * Returns the Python version to use. + * + *

Specified using either the {@code --python_version} flag and {@code python_version} rule + * attribute (new API), or the {@code --force_python} flag and {@code default_python_version} rule + * attribute (old API). */ - public PythonVersion getPythonVersion(PythonVersion attributeVersion) { - return ignorePythonVersionAttribute || attributeVersion == null - ? defaultPythonVersion - : attributeVersion; + public PythonVersion getPythonVersion() { + return version; } @Override public String getOutputDirectoryName() { - List allowedVersions = PythonVersion.TARGET_VALUES; + // TODO(brandjon): Implement alternative semantics for controlling which python version(s) get + // suffixed roots. + Preconditions.checkState(version.isTargetValue()); + // The only possible Python target version values are PY2 and PY3. For now, PY2 gets the normal + // output directory name, and PY3 gets a "-py3" suffix. Verify.verify( - allowedVersions.size() == 2, // If allowedVersions.size() == 1, we don't need this method. - ">2 possible defaultPythonVersion values makes output directory clashes possible"); - // Skip this check if --force_python is set. That's because reportInvalidOptions reports - // bad --force_python settings with a clearer user error (and Bazel's configuration - // initialization logic calls reportInvalidOptions after this method). - if (!ignorePythonVersionAttribute && !allowedVersions.contains(defaultPythonVersion)) { - throw new IllegalStateException( - String.format("defaultPythonVersion=%s not allowed: must be in %s to prevent output " - + "directory clashes", defaultPythonVersion, Joiner.on(", ").join(allowedVersions))); + PythonVersion.TARGET_VALUES.size() == 2, // If there is only 1, we don't need this method. + "Detected a change in PythonVersion.TARGET_VALUES so that there are no longer two Python " + + "versions. Please check that PythonConfiguration#getOutputDirectoryName() is still " + + "needed and is still able to avoid output directory clashes, then update this " + + "canary message."); + if (version.equals(PythonVersion.PY2)) { + return null; + } else { + return Ascii.toLowerCase(version.toString()); } - return (defaultPythonVersion == PythonVersion.PY3) ? "py3" : null; } @Override public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) { PythonOptions pythonOptions = buildOptions.get(PythonOptions.class); - if (pythonOptions.forcePython != null - && pythonOptions.forcePython != PythonVersion.PY2 - && pythonOptions.forcePython != PythonVersion.PY3) { - reporter.handle(Event.error("'--force_python' argument must be 'PY2' or 'PY3'")); + if (pythonOptions.pythonVersion != null + && !pythonOptions.experimentalBetterPythonVersionMixing) { + reporter.handle( + Event.error( + "`--python_version` is only allowed with " + + "`--experimental_better_python_version_mixing`")); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java index 8261a663caeec1..93ab29a2fbcab3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java @@ -33,11 +33,9 @@ public ImmutableSet> requiredOptions() { public PythonConfiguration create(BuildOptions buildOptions) throws InvalidConfigurationException { PythonOptions pythonOptions = buildOptions.get(PythonOptions.class); - boolean ignorePythonVersionAttribute = pythonOptions.forcePython != null; PythonVersion pythonVersion = pythonOptions.getPythonVersion(); return new PythonConfiguration( pythonVersion, - ignorePythonVersionAttribute, pythonOptions.buildPythonZip, pythonOptions.buildTransitiveRunfilesTrees); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index 7a7014f747930c..4615e74c97e790 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -143,14 +143,6 @@ public String getTypeDescription() { + " Can be \"PY2\" or \"PY3\".") public PythonVersion hostForcePython; - public PythonVersion getPythonVersion() { - return getPythonVersion(PythonVersion.DEFAULT_TARGET_VALUE); - } - - public PythonVersion getPythonVersion(PythonVersion defaultVersion) { - return (forcePython == null) ? defaultVersion : forcePython; - } - /** * Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for. * @@ -164,9 +156,7 @@ public PythonVersion getPythonVersion(PythonVersion defaultVersion) { * except {@code --python_version} is ignored. * */ - // TODO(brandjon): Eliminate the NEW prefix, make public, use this to replace the above - // getPythonVersion() overloads. - PythonVersion NEWgetPythonVersion() { + public PythonVersion getPythonVersion() { if (experimentalBetterPythonVersionMixing) { if (pythonVersion != null) { return pythonVersion; @@ -180,7 +170,10 @@ PythonVersion NEWgetPythonVersion() { * *

Under the new semantics ({@link #experimentalBetterPythonVersionMixing} is true), version * transitions are always allowed, so this just returns whether the new version is different from - * the existing one. + * the existing one. However, as a compatibility measure for {@code select()}s that depend on + * {@code "force_python"}, transitioning is still done when {@code forcePython} is not in + * agreement with {@link #getPythonVersion}, even if {@code #getPythonVersion}'s value would be + * unaffected. * *

Under the old semantics ({@link #experimentalBetterPythonVersionMixing} is false), version * transitions are not allowed once the version has already been set ({@link #forcePython} is @@ -191,10 +184,10 @@ PythonVersion NEWgetPythonVersion() { * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3} */ public boolean canTransitionPythonVersion(PythonVersion version) { - Preconditions.checkArgument(PythonVersion.TARGET_VALUES.contains(version)); + Preconditions.checkArgument(version.isTargetValue()); if (experimentalBetterPythonVersionMixing) { - PythonVersion currentVersion = NEWgetPythonVersion(); - return !version.equals(currentVersion); + PythonVersion currentVersion = getPythonVersion(); + return !version.equals(currentVersion) || !version.equals(forcePython); } else { return forcePython == null && !version.equals(PythonVersion.DEFAULT_TARGET_VALUE); } @@ -212,12 +205,21 @@ public boolean canTransitionPythonVersion(PythonVersion version) { * false), after this method is called {@code transitionPythonVersion} will not be able to change * the version ({@code forcePython} will be non-null). * + *

To help avoid breaking {@code select()} expressions that check the value of {@code + * "force_python"}, under the new semantics both {@code pythonVersion} and {@code forcePython} are + * updated. Note that it is still not guaranteed that all instances of {@code PythonOptions} that + * use the new semantics have {@code forcePython} equal {@code pythonVersion} -- in particular, + * this might not be the case for targets that have not gone through a {@link + * PythonVersionTransition}. + * * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3} */ public void setPythonVersion(PythonVersion version) { - Preconditions.checkArgument(PythonVersion.TARGET_VALUES.contains(version)); + Preconditions.checkArgument(version.isTargetValue()); if (experimentalBetterPythonVersionMixing) { this.pythonVersion = version; + // Meaningless to getPythonVersion(), but read by select()s that depend on "force_python". + this.forcePython = version; } else { this.forcePython = version; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java index f051e2b74a018d..3e906dc1f2bc7c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java @@ -102,6 +102,11 @@ private static ImmutableList convertToStrings(List values public static final PythonVersion DEFAULT_SRCS_VALUE = PY2AND3; + /** Returns whether or not this value is a distinct Python version. */ + public boolean isTargetValue() { + return TARGET_VALUES.contains(this); + } + /** * Converts the string to a target {@code PythonVersion} value (case-sensitive). * diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java index 04419ef2a9f8bb..eacd06813886e1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java @@ -14,49 +14,46 @@ package com.google.devtools.build.lib.rules.python; +import com.google.common.base.Preconditions; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -/** - * A configuration transition that sets the Python version by setting {@link - * PythonOptions#forcePython}. - */ +/** A configuration transition that sets the Python version. */ @AutoCodec public class PythonVersionTransition implements PatchTransition { - private final PythonVersion defaultVersion; + + private final PythonVersion version; /** - * Creates a new transition that sets the given version if not already specified by - * {@link PythonOptions#forcePython}. + * Creates a new transition that sets the version to the given value, if transitioning is allowed. + * + *

See {@link PythonOptions#canTransitionPythonVersion} for information on when transitioning + * is allowed. + * + *

{@code version} must be a target version (either {@code PY2} or {@code PY3}), or else null, + * which means use the default value ({@link PythonVersion#DEFAULT_TARGET_VALUE}). + * + * @throws IllegalArgumentException if {@code version} is non-null and not {@code PY2} or {@code + * PY3} */ - PythonVersionTransition(PythonVersion defaultVersion) { - this.defaultVersion = defaultVersion; + PythonVersionTransition(PythonVersion version) { + if (version == null) { + version = PythonVersion.DEFAULT_TARGET_VALUE; + } + Preconditions.checkArgument(version.isTargetValue()); + this.version = version; } @Override - public BuildOptions patch(BuildOptions options) { - PythonOptions pyOptions = options.get(PythonOptions.class); - // The current Python version is either explicitly set by --force_python or a - // build-wide default. - PythonVersion currentVersion = pyOptions.getPythonVersion(); - // The new Python version is either explicitly set by --force_python or this transition's - // default. - PythonVersion newVersion = pyOptions.getPythonVersion(defaultVersion); - if (currentVersion == newVersion) { + public BuildOptions patch(BuildOptions options) { + PythonOptions opts = options.get(PythonOptions.class); + if (!opts.canTransitionPythonVersion(version)) { return options; } - - // forcePython must be one of PY2 or PY3 because these are the only values Blaze's output - // directories can safely distinguish. In other words, a configuration with forcePython=PY2 - // would have the same output directory prefix as another with forcePython=PY2AND3, which is a - // major correctness failure. - // - // Even though this transition doesn't enforce the above, it only gets called on - // "default_python_version" attribute values, which happen to honor this. Proper enforcement is - // done in PythonConfiguration#getOutputDirectoryName. BuildOptions newOptions = options.clone(); - newOptions.get(PythonOptions.class).forcePython = newVersion; + PythonOptions newOpts = newOptions.get(PythonOptions.class); + newOpts.setPythonVersion(version); return newOptions; } } 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 871c95b90c946d..b3efdc61f514fd 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 @@ -21,6 +21,16 @@ test_suite( ], ) +java_library( + name = "PythonTestUtils", + srcs = ["PythonTestUtils.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib:python-rules", + "//third_party:junit4", + "//third_party:truth", + ], +) + java_test( name = "BazelPythonConfigurationTest", srcs = ["BazelPythonConfigurationTest.java"], @@ -39,6 +49,7 @@ java_test( name = "PythonConfigurationTest", srcs = ["PythonConfigurationTest.java"], deps = [ + ":PythonTestUtils", "//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/common/options", @@ -53,6 +64,7 @@ java_library( name = "PyBaseTestBase", srcs = ["PyBaseConfiguredTargetTestBase.java"], deps = [ + ":PythonTestUtils", "//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", @@ -67,9 +79,11 @@ java_library( srcs = ["PyExecutableConfiguredTargetTestBase.java"], deps = [ ":PyBaseTestBase", + ":PythonTestUtils", "//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", + "//third_party:guava", "//third_party:junit4", "//third_party:truth", ], @@ -101,7 +115,9 @@ java_test( srcs = ["PyLibraryConfiguredTargetTest.java"], deps = [ ":PyBaseTestBase", + ":PythonTestUtils", "//src/main/java/com/google/devtools/build/lib:build-base", + "//src/main/java/com/google/devtools/build/lib:python-rules", "//src/test/java/com/google/devtools/build/lib:actions_testutil", "//src/test/java/com/google/devtools/build/lib:analysis_testutil", "//third_party:junit4", 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 38c4674d2454da..17687a082adfae 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.python; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; @@ -35,6 +36,7 @@ public final void setUpPython() throws Exception { analysisMock.pySupport().setup(mockToolsConfig); } + /** Retrieves the Python version of a configured target. */ protected PythonVersion getPythonVersion(ConfiguredTarget ct) { return getConfiguration(ct).getOptions().get(PythonOptions.class).getPythonVersion(); } @@ -79,6 +81,7 @@ public void srcsVersionClashesWithForcePythonFlag() throws Exception { @Test public void versionIs2IfUnspecified() throws Exception { + ensureDefaultIsPY2(); scratch.file("pkg/BUILD", ruleName + "(", " name = 'foo',", @@ -87,8 +90,14 @@ public void versionIs2IfUnspecified() throws Exception { } @Test - public void versionIs3IfForcedByFlag() throws Exception { - useConfiguration("--force_python=PY3"); + public void versionIs3IfForcedByFlagUnderOldSemantics() throws Exception { + // Under the old version semantics, --force_python takes precedence over the rule's own + // default_python_version attribute, so this test case applies equally well to py_library, + // py_binary, and py_test. Under the new semantics the rule attribute takes precedence, so this + // would only make sense for py_library; see PyLibraryConfiguredTargetTest for the analogous + // test. + ensureDefaultIsPY2(); + useConfiguration("--experimental_better_python_version_mixing=false", "--force_python=PY3"); scratch.file("pkg/BUILD", ruleName + "(", " name = 'foo',", 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 313d2bbfa8a4c6..6e9a7275c7e4a9 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 @@ -32,7 +32,7 @@ public PyBinaryConfiguredTargetTest() { } @Test - public void python2WithPy3Dependency() throws Exception { + public void python2WithPy3SrcsVersionDependency() throws Exception { eventCollector.clear(); reporter.removeHandler(failFastHandler); // expect errors scratch.file("python2/BUILD", @@ -49,7 +49,7 @@ public void python2WithPy3Dependency() throws Exception { } @Test - public void python2WithPy3OnlyDependency() throws Exception { + public void python2WithPy3OnlySrcsVersionDependency() throws Exception { eventCollector.clear(); reporter.removeHandler(failFastHandler); // expect errors scratch.file("python2/BUILD", @@ -68,7 +68,7 @@ public void python2WithPy3OnlyDependency() throws Exception { } @Test - public void python3WithPy2OnlyDependency() throws Exception { + public void python3WithPy2OnlySrcsVersionDependency() throws Exception { eventCollector.clear(); reporter.removeHandler(failFastHandler); // expect errors scratch.file("python3/BUILD", 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 857ed984c64a06..efe33762c27cc5 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 @@ -15,7 +15,10 @@ package com.google.devtools.build.lib.rules.python; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2; +import com.google.common.base.Joiner; import org.junit.Test; /** Tests that are common to {@code py_binary} and {@code py_test}. */ @@ -28,135 +31,188 @@ protected PyExecutableConfiguredTargetTestBase(String ruleName) { this.ruleName = ruleName; } + /** + * Sets the configuration, then asserts that a configured target has the given Python version. + * + *

The configuration is given as a series of "--flag=value" strings. + */ + protected void assertPythonVersionIs_UnderNewConfig( + String targetName, PythonVersion version, String... flags) throws Exception { + useConfiguration(flags); + assertThat(getPythonVersion(getConfiguredTarget(targetName))).isEqualTo(version); + } + + /** + * Asserts that a configured target has the given Python version under multiple configurations. + * + *

The configurations are given as a series of arrays of "--flag=value" strings. + * + *

This destructively changes the current configuration. + */ + protected void assertPythonVersionIs_UnderNewConfigs( + String targetName, PythonVersion version, String[]... configs) throws Exception { + for (String[] config : configs) { + useConfiguration(config); + assertWithMessage(String.format("Under config '%s'", Joiner.on(" ").join(config))) + .that(getPythonVersion(getConfiguredTarget(targetName))) + .isEqualTo(version); + } + } + + private String ruleDeclWithVersionAttr(String name, String version) { + return ruleName + "(\n" + + " name = '" + name + "',\n" + + " srcs = ['" + name + ".py'],\n" + + " default_python_version = '" + version + "'\n" + + ")\n"; + } + @Test - public void unknownDefaultPythonVersionValue() throws Exception { + public void versionAttr_UnknownValue() throws Exception { checkError("pkg", "foo", // error: "invalid value in 'default_python_version' attribute: " + "has to be one of 'PY2' or 'PY3' instead of 'doesnotexist'", // build file: - ruleName + "(", - " name = 'foo',", - " default_python_version = 'doesnotexist',", - " srcs = ['foo.py'])"); + ruleDeclWithVersionAttr("foo", "doesnotexist")); } @Test - public void badDefaultPythonVersionValue() throws Exception { + public void versionAttr_BadValue() throws Exception { checkError("pkg", "foo", // error: "invalid value in 'default_python_version' attribute: " + "has to be one of 'PY2' or 'PY3' instead of 'PY2AND3'", // build file: - ruleName + "(", - " name = 'foo',", - " default_python_version = 'PY2AND3',", - " srcs = ['foo.py'])"); + ruleDeclWithVersionAttr("foo", "PY2AND3")); } @Test - public void goodDefaultPythonVersionValue() throws Exception { - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo',", - " default_python_version = 'PY2',", - " srcs = ['foo.py'])"); - getConfiguredTarget("//foo:foo"); + public void versionAttr_GoodValue() throws Exception { + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2")); + getConfiguredTarget("//pkg:foo"); assertNoEvents(); } @Test - public void versionIs3WhenSetByDefaultPythonVersion() throws Exception { - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo',", - " srcs = ['foo.py'],", - " default_python_version = 'PY3')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo"))).isEqualTo(PythonVersion.PY3); + public void versionAttrWorksUnderOldAndNewSemantics_WhenNotDefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3")); + + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY3, + new String[] {"--experimental_better_python_version_mixing=false"}, + new String[] {"--experimental_better_python_version_mixing=true"}); } @Test - public void versionIs2WhenSetByDefaultPythonVersion() throws Exception { - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo',", - " srcs = ['foo.py'],", - " default_python_version = 'PY2')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo"))).isEqualTo(PythonVersion.PY2); + public void versionAttrWorksUnderOldAndNewSemantics_WhenSameAsDefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2")); + + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY2, + new String[] {"--experimental_better_python_version_mixing=false"}, + new String[] {"--experimental_better_python_version_mixing=true"}); } @Test - public void canBuildTwoTargetsSpecifyingDifferentVersions() throws Exception { - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo_v2',", - " srcs = ['foo_v2.py'],", - " default_python_version = 'PY2')", - ruleName + "(", - " name = 'foo_v3',", - " srcs = ['foo_v3.py'],", - " default_python_version = 'PY3')"); + public void flagTakesPrecedenceUnderOldSemantics_NonDefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2")); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY3, + "--experimental_better_python_version_mixing=false", + "--force_python=PY3"); + } - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v2"))).isEqualTo(PythonVersion.PY2); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v3"))).isEqualTo(PythonVersion.PY3); + @Test + public void flagTakesPrecedenceUnderOldSemantics_DefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3")); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY2, + "--experimental_better_python_version_mixing=false", + "--force_python=PY2"); } @Test - public void flagOverridesDefaultPythonVersionFrom2To3() throws Exception { - useConfiguration("--force_python=PY3"); - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo',", - " srcs = ['foo.py'],", - " default_python_version = 'PY2')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo"))).isEqualTo(PythonVersion.PY3); + public void versionAttrTakesPrecedenceUnderNewSemantics_NonDefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3")); + + // Test against both flags. + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY3, + new String[] {"--experimental_better_python_version_mixing=true", "--force_python=PY2"}, + new String[] {"--experimental_better_python_version_mixing=true", "--python_version=PY2"}); } @Test - public void flagOverridesDefaultPythonVersionFrom3To2() throws Exception { - useConfiguration("--force_python=PY2"); - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo',", - " srcs = ['foo.py'],", - " default_python_version = 'PY3')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo"))).isEqualTo(PythonVersion.PY2); + public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Exception { + ensureDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2")); + + // Test against both flags. + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY2, + new String[] {"--experimental_better_python_version_mixing=true", "--force_python=PY3"}, + new String[] {"--experimental_better_python_version_mixing=true", "--python_version=PY3"}); } @Test - public void canBuildTwoTargetsSpecifyingDifferentVersions_ForcedTo2() throws Exception { - useConfiguration("--force_python=PY2"); - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo_v2',", - " srcs = ['foo_v2.py'],", - " default_python_version = 'PY2')", - ruleName + "(", - " name = 'foo_v3',", - " srcs = ['foo_v3.py'],", - " default_python_version = 'PY3')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v2"))).isEqualTo(PythonVersion.PY2); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v3"))).isEqualTo(PythonVersion.PY2); + public void canBuildWithDifferentVersionAttrs_UnderOldAndNewSemantics() throws Exception { + scratch.file( + "pkg/BUILD", + ruleDeclWithVersionAttr("foo_v2", "PY2"), + ruleDeclWithVersionAttr("foo_v3", "PY3")); + + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo_v2", + PythonVersion.PY2, + new String[] {"--experimental_better_python_version_mixing=false"}, + new String[] {"--experimental_better_python_version_mixing=true"}); + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo_v3", + PythonVersion.PY3, + new String[] {"--experimental_better_python_version_mixing=false"}, + new String[] {"--experimental_better_python_version_mixing=true"}); } @Test - public void canBuildTwoTargetsSpecifyingDifferentVersions_ForcedTo3() throws Exception { - useConfiguration("--force_python=PY3"); - scratch.file("foo/BUILD", - ruleName + "(", - " name = 'foo_v2',", - " srcs = ['foo_v2.py'],", - " default_python_version = 'PY2')", - ruleName + "(", - " name = 'foo_v3',", - " srcs = ['foo_v3.py'],", - " default_python_version = 'PY3')"); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v2"))).isEqualTo(PythonVersion.PY3); - assertThat(getPythonVersion(getConfiguredTarget("//foo:foo_v3"))).isEqualTo(PythonVersion.PY3); + public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToDefault() + throws Exception { + ensureDefaultIsPY2(); + scratch.file( + "pkg/BUILD", + ruleDeclWithVersionAttr("foo_v2", "PY2"), + ruleDeclWithVersionAttr("foo_v3", "PY3")); + + assertPythonVersionIs_UnderNewConfig("//pkg:foo_v2", PythonVersion.PY2, "--force_python=PY2"); + assertPythonVersionIs_UnderNewConfig("//pkg:foo_v3", PythonVersion.PY2, "--force_python=PY2"); } @Test - public void srcsVersionClashesWithDefaultVersionAttr() throws Exception { + public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToNonDefault() + throws Exception { + ensureDefaultIsPY2(); + scratch.file( + "pkg/BUILD", + ruleDeclWithVersionAttr("foo_v2", "PY2"), + ruleDeclWithVersionAttr("foo_v3", "PY3")); + + assertPythonVersionIs_UnderNewConfig("//pkg:foo_v2", PythonVersion.PY3, "--force_python=PY3"); + assertPythonVersionIs_UnderNewConfig("//pkg:foo_v3", PythonVersion.PY3, "--force_python=PY3"); + } + + @Test + public void srcsVersionClashesWithVersionAttr() throws Exception { checkError("pkg", "foo", // error: "'//pkg:foo' can only be used with Python 2", @@ -169,11 +225,8 @@ public void srcsVersionClashesWithDefaultVersionAttr() throws Exception { } @Test - public void srcsVersionClashesWithDefaultVersionAttr_Implicitly() throws Exception { - // Canary assertion: This'll fail when we flip the default to PY3. At that point change this - // test to use srcs_version = 'PY2ONLY' instead. - assertThat(PythonVersion.DEFAULT_TARGET_VALUE).isEqualTo(PythonVersion.PY2); - + public void srcsVersionClashesWithVersionAttr_Implicitly() throws Exception { + ensureDefaultIsPY2(); // When changed to PY3, flip srcs_version below to be PY2ONLY. // 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 7a69977aa301da..26867c9d2cfa2b 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.python; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -32,6 +33,20 @@ public PyLibraryConfiguredTargetTest() { super("py_library"); } + @Test + public void versionIs3IfSetByFlagUnderNewSemantics() 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. + ensureDefaultIsPY2(); + useConfiguration("--experimental_better_python_version_mixing=true", "--python_version=PY3"); + scratch.file( + "pkg/BUILD", + "py_library(", + " name = 'foo',", + " srcs = ['foo.py'])"); + assertThat(getPythonVersion(getConfiguredTarget("//pkg:foo"))).isEqualTo(PythonVersion.PY3); + } + @Test public void filesToBuild() throws Exception { scratch.file("pkg/BUILD", diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java index a7854d554db3e4..cd71ecbb40f854 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java @@ -15,7 +15,7 @@ package com.google.devtools.build.lib.rules.python; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -30,17 +30,6 @@ @RunWith(JUnit4.class) public class PythonConfigurationTest extends ConfigurationTestCase { - private void ensureDefaultIsPY2() { - // Ensure that the expected value differs from the default value, so that if the code under test - // ever returns the default value where it shouldn't have, the test doesn't spuriously succeed. - assertWithMessage( - "This test case is written with the assumption that the default is Python 2. When " - + "updating the default to Python 3, flip all the PY2/PY3 constants in the test " - + "case and this helper.") - .that(PythonVersion.DEFAULT_TARGET_VALUE) - .isEqualTo(PythonVersion.PY2); - } - private PythonOptions parsePythonOptions(String... cmdline) throws Exception { BuildConfiguration config = create(cmdline); return config.getOptions().get(PythonOptions.class); @@ -62,24 +51,36 @@ public void invalidTargetPythonValue_UnknownValue() { assertThat(expected).hasMessageThat().contains("Not a valid Python major version"); } + @Test + public void newVersionFlagRequiresExperimentalFlag() throws Exception { + // Try both Python versions. + checkError( + "only allowed with `--experimental_better_python_version_mixing`", + "--experimental_better_python_version_mixing=false", + "--python_version=PY2"); + checkError( + "only allowed with `--experimental_better_python_version_mixing`", + "--experimental_better_python_version_mixing=false", + "--python_version=PY3"); + } + @Test public void getPythonVersion_OldApi_NewFlagIgnored() throws Exception { ensureDefaultIsPY2(); - // --python_version should be ignored by getPythonVersion (and in fact would be disallowed in - // the analysis phase). PythonOptions opts = parsePythonOptions( - "--experimental_better_python_version_mixing=false", - "--force_python=PY3", - "--python_version=PY2"); - assertThat(opts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY3); + "--experimental_better_python_version_mixing=false", "--force_python=PY3"); + // --python_version should be ignored by getPythonVersion(). (Set manually since parsing would + // fail reportInvalidOptions().) + opts.pythonVersion = PythonVersion.PY2; + assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY3); } @Test public void getPythonVersion_OldApi_HardcodedDefaultWhenOmitted() throws Exception { ensureDefaultIsPY2(); PythonOptions opts = parsePythonOptions("--experimental_better_python_version_mixing=false"); - assertThat(opts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY2); + assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY2); } @Test @@ -91,7 +92,7 @@ public void getPythonVersion_NewApi_NewFlagTakesPrecedence() throws Exception { "--experimental_better_python_version_mixing=true", "--force_python=PY2", "--python_version=PY3"); - assertThat(opts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY3); + assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY3); } @Test @@ -101,24 +102,23 @@ public void getPythonVersion_NewApi_FallBackOnOldFlag() throws Exception { PythonOptions opts = parsePythonOptions( "--experimental_better_python_version_mixing=true", "--force_python=PY3"); - assertThat(opts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY3); + assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY3); } @Test public void getPythonVersion_NewApi_HardcodedDefaultWhenOmitted() throws Exception { ensureDefaultIsPY2(); PythonOptions opts = parsePythonOptions("--experimental_better_python_version_mixing=true"); - assertThat(opts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY2); + assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY2); } @Test public void canTransitionPythonVersion_OldApi_Yes() throws Exception { ensureDefaultIsPY2(); - PythonOptions opts = - parsePythonOptions( - "--experimental_better_python_version_mixing=false", - // --python_version should be ignored. - "--python_version=PY2"); + PythonOptions opts = parsePythonOptions("--experimental_better_python_version_mixing=false"); + // Also test that --python_version should be ignored. (Set manually since parsing would fail + // reportInvalidOptions().) + opts.pythonVersion = PythonVersion.PY2; boolean result = opts.canTransitionPythonVersion(PythonVersion.PY3); assertThat(result).isTrue(); } @@ -165,13 +165,28 @@ public void canTransitionPythonVersion_NewApi_No() throws Exception { } @Test - public void setPythonVersion_OldApi() throws Exception { + public void canTransitionPythonVersion_NewApi_YesBecauseForcePythonDisagrees() throws Exception { PythonOptions opts = parsePythonOptions( - "--experimental_better_python_version_mixing=false", + "--experimental_better_python_version_mixing=true", + // Even though getPythonVersion() would be PY3 due to the value of --python_version, + // test that we should still transition in order to flip --force_python to PY3. This is + // needed for compatibility with old-style select()s that depend on config_settings that + // read "force_python". "--force_python=PY2", - // --python_version should be ignored. - "--python_version=PY2"); + "--python_version=PY3"); + boolean result = opts.canTransitionPythonVersion(PythonVersion.PY3); + assertThat(result).isTrue(); + } + + @Test + public void setPythonVersion_OldApi() throws Exception { + PythonOptions opts = + parsePythonOptions( + "--experimental_better_python_version_mixing=false", "--force_python=PY2"); + // Also test that --python_version should be ignored. (Set manually since parsing would fail + // reportInvalidOptions().) + opts.pythonVersion = PythonVersion.PY2; opts.setPythonVersion(PythonVersion.PY3); assertThat(opts.experimentalBetterPythonVersionMixing).isFalse(); assertThat(opts.forcePython).isEqualTo(PythonVersion.PY3); @@ -183,12 +198,13 @@ public void setPythonVersion_NewApi() throws Exception { PythonOptions opts = parsePythonOptions( "--experimental_better_python_version_mixing=true", - // --force_python should be ignored. + // --force_python should be updated too, for improved compatibility with old-style + // select()s that depend on config_settings that read "force_python". "--force_python=PY2", "--python_version=PY2"); opts.setPythonVersion(PythonVersion.PY3); assertThat(opts.experimentalBetterPythonVersionMixing).isTrue(); - assertThat(opts.forcePython).isEqualTo(PythonVersion.PY2); + assertThat(opts.forcePython).isEqualTo(PythonVersion.PY3); assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3); } @@ -204,7 +220,7 @@ public void getHost_OldSemantics() throws Exception { PythonOptions newOpts = (PythonOptions) opts.getHost(); assertThat(newOpts.experimentalBetterPythonVersionMixing).isFalse(); assertThat(newOpts.forcePython).isEqualTo(PythonVersion.PY3); - assertThat(newOpts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY3); + assertThat(newOpts.getPythonVersion()).isEqualTo(PythonVersion.PY3); assertThat(newOpts.buildPythonZip).isEqualTo(TriState.YES); } @@ -220,7 +236,7 @@ public void getHost_NewSemantics() throws Exception { PythonOptions newOpts = (PythonOptions) opts.getHost(); assertThat(newOpts.experimentalBetterPythonVersionMixing).isTrue(); assertThat(newOpts.pythonVersion).isEqualTo(PythonVersion.PY3); - assertThat(newOpts.NEWgetPythonVersion()).isEqualTo(PythonVersion.PY3); + assertThat(newOpts.getPythonVersion()).isEqualTo(PythonVersion.PY3); assertThat(newOpts.buildPythonZip).isEqualTo(TriState.YES); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonTestUtils.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonTestUtils.java new file mode 100644 index 00000000000000..916cf13acaae9c --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonTestUtils.java @@ -0,0 +1,41 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.python; + +import static com.google.common.truth.Truth.assertWithMessage; + +/** Helpers for Python tests. */ +public class PythonTestUtils { + + // Static utilities class. + private PythonTestUtils() {} + + /** + * Assert that {@link PythonVersion#DEFAULT_TARGET_VALUE} hasn't changed. + * + *

Use this to indicate that the PY2 and PY3 values of your test should be flipped if this + * default value is changed. In general, it is useful to write tests with expected values that + * differ from the default, so that they don't spuriously succeed if the default is erroneously + * returned. + */ + public static void ensureDefaultIsPY2() { + assertWithMessage( + "This test case is written with the assumption that the default is Python 2. When " + + "updating the default to Python 3, flip all the PY2/PY3 constants in the test " + + "case and this helper function.") + .that(PythonVersion.DEFAULT_TARGET_VALUE) + .isEqualTo(PythonVersion.PY2); + } +} diff --git a/src/test/shell/bazel/python_version_test.sh b/src/test/shell/bazel/python_version_test.sh index e0df9819acf176..dd83f897060731 100755 --- a/src/test/shell/bazel/python_version_test.sh +++ b/src/test/shell/bazel/python_version_test.sh @@ -128,12 +128,14 @@ py_runtime( EOF } +function set_up() { + use_system_python_2_3_runtimes +} + #### TESTS ############################################################# # Sanity test that our environment setup above works. function test_can_run_py_binaries() { - use_system_python_2_3_runtimes - mkdir -p test cat > test/BUILD << EOF @@ -168,8 +170,6 @@ EOF # Test that access to runfiles works (in general, and under our test environment # specifically). function test_can_access_runfiles() { - use_system_python_2_3_runtimes - mkdir -p test cat > test/BUILD << EOF @@ -203,4 +203,276 @@ EOF expect_log "File contents: abcdefg" } +function test_pybin_can_have_different_version_pybin_as_data_dep() { + mkdir -p test + + cat > test/BUILD << EOF +py_binary( + name = "py2bin", + srcs = ["py2bin.py"], + default_python_version = "PY2", +) +py_binary( + name = "py3bin", + srcs = ["py3bin.py"], + default_python_version = "PY3", +) +py_binary( + name = "py2bin_calling_py3bin", + srcs = ["py2bin_calling_py3bin.py"], + deps = ["@bazel_tools//tools/python/runfiles"], + data = [":py3bin"], + default_python_version = "PY2", +) +py_binary( + name = "py3bin_calling_py2bin", + srcs = ["py3bin_calling_py2bin.py"], + deps = ["@bazel_tools//tools/python/runfiles"], + data = [":py2bin"], + default_python_version = "PY3", +) +EOF + + cat > test/py2bin.py << EOF +import platform + +print("Inner bin uses Python " + platform.python_version_tuple()[0]) +EOF + chmod u+x test/py2bin.py + cp test/py2bin.py test/py3bin.py + + cat > test/py2bin_calling_py3bin.py << EOF +import platform +import subprocess +from bazel_tools.tools.python.runfiles import runfiles + +r = runfiles.Create() +bin_path = r.Rlocation("$WORKSPACE_NAME/test/py3bin") + +print("Outer bin uses Python " + platform.python_version_tuple()[0]) +subprocess.call([bin_path]) +EOF + sed s/py3bin/py2bin/ test/py2bin_calling_py3bin.py > test/py3bin_calling_py2bin.py + chmod u+x test/py2bin_calling_py3bin.py test/py3bin_calling_py2bin.py + + EXPFLAG="--experimental_better_python_version_mixing=true" + + bazel build $EXPFLAG //test:py2bin_calling_py3bin //test:py3bin_calling_py2bin \ + || fail "bazel build failed" + PY2_OUTER_BIN=$(bazel info bazel-bin $EXPFLAG)/test/py2bin_calling_py3bin + PY3_OUTER_BIN=$(bazel info bazel-bin $EXPFLAG --python_version=py3)/test/py3bin_calling_py2bin + + RUNFILES_MANIFEST_FILE= RUNFILES_DIR= $PY2_OUTER_BIN &> $TEST_log + expect_log "Outer bin uses Python 2" + expect_log "Inner bin uses Python 3" + + RUNFILES_MANIFEST_FILE= RUNFILES_DIR= $PY3_OUTER_BIN &> $TEST_log + expect_log "Outer bin uses Python 3" + expect_log "Inner bin uses Python 2" +} + +function test_shbin_can_have_different_version_pybins_as_data_deps() { + mkdir -p test + + cat > test/BUILD << EOF +py_binary( + name = "py2bin", + srcs = ["py2bin.py"], + default_python_version = "PY2", +) +py_binary( + name = "py3bin", + srcs = ["py3bin.py"], + default_python_version = "PY3", +) +sh_binary( + name = "shbin_calling_py23bins", + srcs = ["shbin_calling_py23bins.sh"], + deps = ["@bazel_tools//tools/bash/runfiles"], + data = [":py2bin", ":py3bin"], +) +EOF + + cat > test/py2bin.py << EOF +import platform + +print("py2bin uses Python " + platform.python_version_tuple()[0]) +EOF + sed s/py2bin/py3bin/ test/py2bin.py > test/py3bin.py + chmod u+x test/py2bin.py test/py3bin.py + + # The workspace name is initialized in testenv.sh; use that var rather than + # hardcoding it here. The extra sed pass is so we can selectively expand that + # one var while keeping the rest of the heredoc literal. + cat | sed "s/{{WORKSPACE_NAME}}/$WORKSPACE_NAME/" > test/shbin_calling_py23bins.sh << 'EOF' +# --- begin runfiles.bash initialization --- +# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash). +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +$(rlocation {{WORKSPACE_NAME}}/test/py2bin) +$(rlocation {{WORKSPACE_NAME}}/test/py3bin) +EOF + + chmod u+x test/shbin_calling_py23bins.sh + + EXPFLAG="--experimental_better_python_version_mixing=true" + + bazel build $EXPFLAG //test:shbin_calling_py23bins \ + || fail "bazel build failed" + SH_BIN=$(bazel info bazel-bin $EXPFLAG)/test/shbin_calling_py23bins + + RUNFILES_MANIFEST_FILE= RUNFILES_DIR= $SH_BIN &> $TEST_log + expect_log "py2bin uses Python 2" + expect_log "py3bin uses Python 3" +} + +function test_genrule_can_have_different_version_pybins_as_tools() { + # This test currently checks that we can use --host_force_python to get + # PY2 and PY3 binaries in tools. In the future we'll support both modes in the + # same build without a flag (#6443). + + mkdir -p test + + cat > test/BUILD << 'EOF' +py_binary( + name = "pybin", + srcs = ["pybin.py"], +) +genrule( + name = "genrule_calling_pybin", + outs = ["out.txt"], + tools = [":pybin"], + cmd = "$(location :pybin) > $(location out.txt)" +) +EOF + + cat > test/pybin.py << EOF +import platform + +print("pybin uses Python " + platform.python_version_tuple()[0]) +EOF + chmod u+x test/pybin.py + + # Run under both old and new semantics. + for EXPFLAG in \ + "--experimental_better_python_version_mixing=true" \ + "--experimental_better_python_version_mixing=false"; do + echo "Using $EXPFLAG" > $TEST_log + bazel build $EXPFLAG --host_force_python=PY2 //test:genrule_calling_pybin \ + || fail "bazel build failed" + ARTIFACT=$(bazel info bazel-genfiles $EXPFLAG)/test/out.txt + cat $ARTIFACT > $TEST_log + expect_log "pybin uses Python 2" + + echo "Using $EXPFLAG" > $TEST_log + bazel build $EXPFLAG --host_force_python=PY3 //test:genrule_calling_pybin \ + || fail "bazel build failed" + ARTIFACT=$(bazel info bazel-genfiles $EXPFLAG)/test/out.txt + cat $ARTIFACT > $TEST_log + expect_log "pybin uses Python 3" + done +} + +function test_can_build_same_target_for_both_versions_in_one_build() { + mkdir -p test + + cat > test/BUILD << EOF +py_library( + name = "common", + srcs = ["common.py"], +) +py_binary( + name = "py2bin", + srcs = ["py2bin.py"], + deps = [":common"], + default_python_version = "PY2", +) +py_binary( + name = "py3bin", + srcs = ["py3bin.py"], + deps = [":common"], + default_python_version = "PY3", +) +sh_binary( + name = "shbin", + srcs = ["shbin.sh"], + deps = ["@bazel_tools//tools/bash/runfiles"], + data = [":py2bin", ":py3bin"], +) +EOF + + cat > test/common.py << EOF +import platform + +print("common using Python " + platform.python_version_tuple()[0]) +EOF + + cat > test/py2bin.py << EOF +import common +EOF + chmod u+x test/py2bin.py + cp test/py2bin.py test/py3bin.py + + # The workspace name is initialized in testenv.sh; use that var rather than + # hardcoding it here. The extra sed pass is so we can selectively expand that + # one var while keeping the rest of the heredoc literal. + cat | sed "s/{{WORKSPACE_NAME}}/$WORKSPACE_NAME/" > test/shbin.sh << 'EOF' +# --- begin runfiles.bash initialization --- +# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash). +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +$(rlocation {{WORKSPACE_NAME}}/test/py2bin) +$(rlocation {{WORKSPACE_NAME}}/test/py3bin) +EOF + chmod u+x test/shbin.sh + + EXPFLAG="--experimental_better_python_version_mixing=true" + + bazel build $EXPFLAG //test:shbin \ + || fail "bazel build failed" + SH_BIN=$(bazel info bazel-bin)/test/shbin + + RUNFILES_MANIFEST_FILE= RUNFILES_DIR= $SH_BIN &> $TEST_log + expect_log "common using Python 2" + expect_log "common using Python 3" +} + run_suite "Tests for how the Python rules handle Python 2 vs Python 3"