Skip to content

Commit

Permalink
Make Bazel tests compatible with Starlark implementation of Python rules
Browse files Browse the repository at this point in the history
Summary of changes:

* Remove usage of Python rules where not necessary; the Python rules
  were/are being used to test features that aren't specific to the Python
  rules.
* Use regex matching for failures instead of exact strings. The Starlark
  rules have slightly different phrasing or orderings of elements that
  don't affect behavior. This also makes the tests less brittle overall.
* Disable attempting to use the Starlark implementation in all the tests
  requiring Python 2 (e.g. srcs_version checking etc). The Starlark
  implementation doesn't currently (and probably never will) support Python
  2. #15684 filed to track removal
  of it in Bazel.
* Disable attempting to use the Starlark implementation where toolchain
  resolution is required. This will eventually be implemented, but isn't
  is use at Google and otherwise blocks switching internally.
* Tests verifying warnings are printed were removed; Starlark doesn't
  provide a warning facility.
* Stamping is disabled in various tests because, with the Starlark implementation,
  it requires remote execution of actions, which some tests aren't setup for
  (they never needed it previously). Rather than set this up, stamping was
  disabled (affected tests don't require stamping anyways).

PiperOrigin-RevId: 455695015
Change-Id: I82822eff05b6c0a66e65f131c9e1c8784b1573ac
  • Loading branch information
rickeylev authored and copybara-github committed Jun 17, 2022
1 parent 8b5ed8a commit 7a230fd
Show file tree
Hide file tree
Showing 13 changed files with 132 additions and 79 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ public CcCompilationOutputs getCcCompilationOutputs(RuleContext ruleContext) {
return ccCompilationOutputs;
}

@VisibleForTesting
public CcCompilationOutputs getCcCompilationOutputsForTesting() {
return ccCompilationOutputs;
}

@StarlarkMethod(name = "compilation_outputs", documented = false, useStarlarkThread = true)
public CcCompilationOutputs getCcCompilationOutputsStarlark(StarlarkThread thread)
throws EvalException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import java.util.regex.Pattern;
import org.junit.Before;
import org.junit.Test;

Expand All @@ -42,11 +43,11 @@ protected PythonVersion getPythonVersion(ConfiguredTarget ct) {

@Test
public void badSrcsVersionValue() throws Exception {
checkError("pkg", "foo",
checkError(
"pkg",
"foo",
// error:
"invalid value in 'srcs_version' attribute: "
+ "has to be one of 'PY2', 'PY3', 'PY2AND3', 'PY2ONLY' "
+ "or 'PY3ONLY' instead of 'doesnotexist'",
Pattern.compile(".*invalid value.*srcs_version.*"),
// build file:
ruleName + "(",
" name = 'foo',",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget;
import com.google.devtools.build.lib.testutil.TestConstants;
import java.util.regex.Pattern;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -50,6 +51,7 @@ private void declareBinDependingOnLibWithVersions(String binVersion, String libS

@Test
public void python2WithPy3SrcsVersionDependency() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
declareBinDependingOnLibWithVersions("PY2", "PY3");
assertThat(getPyExecutableDeferredError("//pkg:bin"))
.startsWith(
Expand All @@ -59,13 +61,15 @@ public void python2WithPy3SrcsVersionDependency() throws Exception {

@Test
public void python2WithPy3OnlySrcsVersionDependency() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
declareBinDependingOnLibWithVersions("PY2", "PY3ONLY");
assertThat(getPyExecutableDeferredError("//pkg:bin"))
.contains("being built for Python 2 but (transitively) includes Python 3-only sources");
}

@Test
public void python3WithPy2OnlySrcsVersionDependency_NewSemantics() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
declareBinDependingOnLibWithVersions("PY3", "PY2ONLY");
assertThat(getPyExecutableDeferredError("//pkg:bin"))
.contains("being built for Python 3 but (transitively) includes Python 2-only sources");
Expand Down Expand Up @@ -140,10 +144,11 @@ public void defaultMainCannotBeAmbiguous() throws Exception {
"py_binary(",
" name = 'foo',",
" srcs = ['bar.py'])");
checkError("pkg2", "bar",
checkError(
"pkg2",
"bar",
// error:
"default main file name 'bar.py' matches multiple files. Perhaps specify an explicit file "
+ "with 'main' attribute? Matches were: 'pkg2/bar.py' and 'pkg1/bar.py'",
Pattern.compile(".*bar.py.*matches multiple.*"),
// build file:
"py_binary(",
" name = 'bar',",
Expand All @@ -156,10 +161,11 @@ public void explicitMainCannotBeAmbiguous() throws Exception {
"py_binary(",
" name = 'foo',",
" srcs = ['bar.py'])");
checkError("pkg2", "foo",
checkError(
"pkg2",
"foo",
// error:
"file name 'bar.py' specified by 'main' attribute matches multiple files: e.g., "
+ "'pkg2/bar.py' and 'pkg1/bar.py'",
Pattern.compile(".*bar.py.*matches multiple.*"),
// build file:
"py_binary(",
" name = 'foo',",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

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.assumesDefaultIsPY2;

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -127,6 +126,8 @@ private String ruleDeclWithPyVersionAttr(String name, String version) {

@Test
public void pyRuntimeInfoIsPresent() throws Exception {
// Starlark implementation doesn't yet support toolchain resolution.
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
useConfiguration("--incompatible_use_python_toolchains=true");
scratch.file(
"pkg/BUILD", //
Expand Down Expand Up @@ -163,13 +164,14 @@ public void versionAttr_BadValue() throws Exception {

@Test
public void versionAttr_GoodValue() throws Exception {
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));
getOkPyTarget("//pkg:foo");
assertNoEvents();
}

@Test
public void py3IsDefaultFlag_SetsDefaultPythonVersion() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file(
"pkg/BUILD", //
ruleName + "(",
Expand All @@ -190,6 +192,7 @@ public void py3IsDefaultFlag_SetsDefaultPythonVersion() throws Exception {

@Test
public void py3IsDefaultFlag_DoesntOverrideExplicitVersion() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));
assertPythonVersionIs_UnderNewConfig(
"//pkg:foo",
Expand All @@ -203,38 +206,38 @@ public void py3IsDefaultFlag_DoesntOverrideExplicitVersion() throws Exception {

@Test
public void versionAttrWorks_WhenNotDefaultValue() throws Exception {
assumesDefaultIsPY2();
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));

assertPythonVersionIs("//pkg:foo", PythonVersion.PY3);
assertPythonVersionIs("//pkg:foo", PythonVersion.PY2);
}

@Test
public void versionAttrWorks_WhenSameAsDefaultValue() throws Exception {
assumesDefaultIsPY2();
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));

assertPythonVersionIs("//pkg:foo", PythonVersion.PY2);
assertPythonVersionIs("//pkg:foo", PythonVersion.PY3);
}

@Test
public void versionAttrTakesPrecedence_NonDefaultValue() throws Exception {
assumesDefaultIsPY2();
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));

assertPythonVersionIs_UnderNewConfig("//pkg:foo", PythonVersion.PY3, "--python_version=PY2");
}

@Test
public void versionAttrTakesPrecedence_DefaultValue() throws Exception {
assumesDefaultIsPY2();
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));

assertPythonVersionIs_UnderNewConfig("//pkg:foo", PythonVersion.PY2, "--python_version=PY3");
assertPythonVersionIs_UnderNewConfig("//pkg:foo", PythonVersion.PY3, "--python_version=PY2");
}

@Test
public void canBuildWithDifferentVersionAttrs() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file(
"pkg/BUILD",
ruleDeclWithPyVersionAttr("foo_v2", "PY2"),
Expand All @@ -246,6 +249,7 @@ public void canBuildWithDifferentVersionAttrs() throws Exception {

@Test
public void incompatibleSrcsVersion() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
reporter.removeHandler(failFastHandler); // We assert below that we don't fail at analysis.
scratch.file(
"pkg/BUILD",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ public void whatIfSrcsContainsRuleGeneratingNoPyFiles() throws Exception {
"'//pkg:bar' does not produce any py_binary srcs files",
// build file:
lines);
} else {
checkWarning(
"pkg",
"foo",
// warning:
"rule '//pkg:bar' does not produce any Python source files",
// build file:
lines);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ private String evaluateAspectFor(String label) throws Exception {

@Test
public void simpleCase() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file(
"pkg/BUILD",
"py_library(",
Expand Down Expand Up @@ -115,6 +116,7 @@ public void noRequirements() throws Exception {

@Test
public void twoContradictoryRequirements() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file(
"pkg/BUILD",
"py_library(",
Expand Down Expand Up @@ -150,6 +152,7 @@ public void twoContradictoryRequirements() throws Exception {

@Test
public void toplevelSelfContradictory() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
scratch.file(
"pkg/BUILD",
"py_binary(",
Expand All @@ -175,6 +178,7 @@ public void toplevelSelfContradictory() throws Exception {

@Test
public void indirectDependencies() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
// A <- B <- C <- bin, where only B has the constraint.
scratch.file(
"pkg/BUILD",
Expand Down Expand Up @@ -215,6 +219,7 @@ public void indirectDependencies() throws Exception {

@Test
public void onlyReportTopmost() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
// A <- B <- C <- bin, where A and C have the constraint.
scratch.file(
"pkg/BUILD",
Expand Down Expand Up @@ -256,6 +261,7 @@ public void onlyReportTopmost() throws Exception {

@Test
public void oneTopmostReachesAnother() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
// A <- B <- C, where A and C have the constraint.
// A <- bin and C <- bin, so both A and C are top-most even though C has a path to A.
scratch.file(
Expand Down Expand Up @@ -300,6 +306,7 @@ public void oneTopmostReachesAnother() throws Exception {

@Test
public void multiplePathsToRequirement() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
// Diamond graph A <- B, A <- C, B <- bin, C <- bin, where only A has the constraint.
// A is reached through two different paths but reported only once.
scratch.file(
Expand Down Expand Up @@ -341,6 +348,7 @@ public void multiplePathsToRequirement() throws Exception {

@Test
public void noSrcsVersionButIntroducesRequirement() throws Exception {
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
// A <- B <- C <- bin, B introduces the requirement but not via srcs_version.
// dummy_rule propagates sources and sets the py3-only bit.
scratch.file(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ public void runtimeSandwich() throws Exception {
" name = 'pybin',",
" srcs = ['pybin.py'],",
")");
useConfiguration("--extra_toolchains=//pkg:usertoolchain");
useConfiguration(
"--extra_toolchains=//pkg:usertoolchain", "--incompatible_use_python_toolchains=true");
// Starlark implementation doesn't yet support toolchain resolution
setBuildLanguageOptions("--experimental_builtins_injection_override=-py_test,-py_binary");
ConfiguredTarget target = getConfiguredTarget("//pkg:pybin");
assertThat(collectRunfiles(target).toList())
.containsAtLeast(getSourceArtifact("pkg/data.txt"), getSourceArtifact("pkg/userdata.txt"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1675,11 +1675,6 @@ public void testAccessingRunfiles() throws Exception {
assertThat(filenames).isInstanceOf(Sequence.class);
Sequence<?> filenamesList = (Sequence) filenames;
assertThat(filenamesList).containsAtLeast("test/lib.py", "test/lib2.py");
Object emptyFilenames =
ev.eval("ruleContext.attr.dep.default_runfiles.empty_filenames.to_list()");
assertThat(emptyFilenames).isInstanceOf(Sequence.class);
Sequence<?> emptyFilenamesList = (Sequence) emptyFilenames;
assertThat(emptyFilenamesList).containsExactly("test/__init__.py");

setRuleContext(createRuleContext("//test:foo_with_init"));
Object noEmptyFilenames =
Expand Down
1 change: 0 additions & 1 deletion src/test/shell/integration/aquery_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1434,7 +1434,6 @@ EOF
|| fail "Expected success"
cat output >> "$TEST_log"

assert_contains "PYTHON_BINARY = '%python_binary%'" output
assert_contains "{%python_binary%:" output

bazel aquery --output=jsonproto ${QUERY} > output 2> "$TEST_log" \
Expand Down
6 changes: 2 additions & 4 deletions src/test/shell/integration/modify_execution_info_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ TestRunner=+requires-test-runner,\
Turbine=+requires-turbine,\
JavaSourceJar=+requires-java-source-jar,\
Javac=+requires-javac,\
PyTinypar=+requires-py-tinypar,\
Py.*=+requires-py,\
Action=+requires-action \
> output 2> "$TEST_log" || fail "Expected success"

Expand All @@ -260,9 +260,7 @@ Action=+requires-action \
assert_contains "requires-java-source-jar: ''" output
assert_contains "requires-proto: ''" output # GenProtoDescriptorSet should match
if [[ "$PRODUCT_NAME" != "bazel" ]]; then
# Python rules generate some cpp actions and local actions, but py-tinypar
# is the main unique-to-python rule which runs remotely for a py_binary.
assert_contains "requires-py-tinypar: ''" output
assert_contains "requires-py: ''" output
fi
}

Expand Down
20 changes: 16 additions & 4 deletions src/test/shell/integration/python_stub_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,18 @@ py_binary(name = "main3",
)
EOF

bazel run //test:main2 \
&> $TEST_log || fail "bazel run failed"
expect_log "I am Python 2"
bazel run //test:main3 \
# Google builds don't support Python 2
if [[ "$PRODUCT_NAME" == "bazel" ]]; then
bazel run //test:main2 \
&> $TEST_log || fail "bazel run failed"
expect_log "I am Python 2"
fi

# Stamping is disabled so that the invocation doesn't time out. What
# happens is Google has stamping enabled by default, which causes the
# Starlark rule implementation to run an action, which then tries to run
# remotely, but network access is disabled by default, so it times out.
bazel run --nostamp //test:main3 \
&> $TEST_log || fail "bazel run failed"
expect_log "I am Python 3"
}
Expand Down Expand Up @@ -117,6 +125,10 @@ EOF
# to a value different from the top-level configuration, and then changing it
# back again, is able to reuse the top-level configuration.
function test_no_action_conflicts_from_version_transition() {
# Requires Python 2 support, which doesn't work for Google-internal builds
if [[ "$PRODUCT_NAME" != "bazel" ]]; then
return 0
fi
mkdir -p test

# To repro, we need to build a C++ target in two different ways in the same
Expand Down
Loading

0 comments on commit 7a230fd

Please sign in to comment.