From d6a98282e229b311dd56e65b72003197120f299a Mon Sep 17 00:00:00 2001 From: carmi Date: Tue, 13 Mar 2018 19:19:16 -0700 Subject: Allow BazelPackageLoader to load external repositories. Also, disallow BazelPackageLoader from fetching missing external repos. Integration tests for BazelPackageLoader wrt external repos will be left for a follow-up CL. RELNOTES: None. PiperOrigin-RevId: 188967694 --- src/test/java/com/google/devtools/build/lib/BUILD | 20 +++- .../build/lib/analysis/mock/BazelAnalysisMock.java | 2 +- .../build/lib/analysis/util/BuildViewTestCase.java | 13 ++- .../build/lib/packages/util/LoadingMock.java | 2 +- .../build/lib/pkgcache/PackageCacheTest.java | 126 +++++++++++---------- .../build/lib/rules/android/AndroidBinaryTest.java | 11 ++ .../build/lib/rules/objc/AppleBinaryTest.java | 12 ++ .../rules/repository/RepositoryDelegatorTest.java | 2 +- .../lib/skyframe/ArtifactFunctionTestCase.java | 2 +- .../build/lib/skyframe/FileFunctionTest.java | 2 +- .../lib/skyframe/FilesystemValueCheckerTest.java | 11 +- .../lib/skyframe/RecursivePkgFunctionTest.java | 9 +- .../lib/skyframe/TimestampBuilderTestCase.java | 2 +- .../packages/AbstractPackageLoaderTest.java | 52 +++++---- .../devtools/build/lib/skyframe/packages/BUILD | 3 + .../skyframe/packages/BazelPackageLoaderTest.java | 60 +++++++++- .../BazelPackageBuilderHelperForTesting.java | 70 ++++++------ ...kageFactoryBuilderFactoryForBazelUnitTests.java | 27 +++-- .../devtools/build/lib/testutil/TestConstants.java | 2 +- 19 files changed, 280 insertions(+), 148 deletions(-) (limited to 'src/test/java/com/google/devtools/build/lib') diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index aa77f34f59..438a480860 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -77,10 +77,14 @@ filegroup( java_library( name = "testutil", testonly = 1, - srcs = glob(["testutil/*.java"]), + srcs = glob( + ["testutil/*.java"], + exclude = ["testutil/BazelPackageBuilderHelperForTesting.java"], + ), visibility = ["//visibility:public"], deps = [ ":guava_junit_truth", + ":testutil/BazelPackageBuilderHelperForTesting", "//src/main/java/com/google/devtools/build/lib:bazel-main", "//src/main/java/com/google/devtools/build/lib:bazel-rules", "//src/main/java/com/google/devtools/build/lib:build-base", @@ -88,6 +92,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:io", "//src/main/java/com/google/devtools/build/lib:os_util", "//src/main/java/com/google/devtools/build/lib:packages", + "//src/main/java/com/google/devtools/build/lib:packages/BuilderFactoryForTesting", "//src/main/java/com/google/devtools/build/lib:util", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/clock", @@ -101,6 +106,19 @@ java_library( ], ) +java_library( + name = "testutil/BazelPackageBuilderHelperForTesting", + testonly = 0, + srcs = ["testutil/BazelPackageBuilderHelperForTesting.java"], + visibility = ["//visibility:public"], + deps = [ + "//src/main/java/com/google/devtools/build/lib:build-base", + "//src/main/java/com/google/devtools/build/lib:packages", + "//src/main/java/com/google/devtools/build/lib/skyframe/packages", + "//third_party:guava", + ], +) + java_library( name = "foundations_testutil", testonly = 1, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index eb6605a649..a9960b0ecd 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -271,8 +271,8 @@ public final class BazelAnalysisMock extends AnalysisMock { @Override public void setupMockWorkspaceFiles(Path embeddedBinariesRoot) throws IOException { + embeddedBinariesRoot.createDirectoryAndParents(); Path jdkWorkspacePath = embeddedBinariesRoot.getRelative("jdk.WORKSPACE"); - FileSystemUtils.createDirectoryAndParents(jdkWorkspacePath.getParentDirectory()); FileSystemUtils.writeContentAsLatin1(jdkWorkspacePath, ""); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index a60b3ecdbe..70842812a8 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -205,6 +205,10 @@ public abstract class BuildViewTestCase extends FoundationTestCase { @Before public final void initializeSkyframeExecutor() throws Exception { + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ true); + } + + public void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Exception { analysisMock = getAnalysisMock(); directories = new BlazeDirectories( @@ -227,13 +231,16 @@ public abstract class BuildViewTestCase extends FoundationTestCase { PrecomputedValue.injected( RepositoryDelegatorFunction.REPOSITORY_OVERRIDES, ImmutableMap.of())); - pkgFactory = + PackageFactory.BuilderForTesting pkgFactoryBuilder = analysisMock .getPackageFactoryBuilderForTesting(directories) .setExtraPrecomputeValues(extraPrecomputedValues) .setEnvironmentExtensions(getEnvironmentExtensions()) - .setPlatformSetRegexps(getPlatformSetRegexps()) - .build(ruleClassProvider, scratch.getFileSystem()); + .setPlatformSetRegexps(getPlatformSetRegexps()); + if (!doPackageLoadingChecks) { + pkgFactoryBuilder.disableChecks(); + } + pkgFactory = pkgFactoryBuilder.build(ruleClassProvider, scratch.getFileSystem()); tsgm = new TimestampGranularityMonitor(BlazeClock.instance()); skyframeExecutor = SequencedSkyframeExecutor.create( diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/LoadingMock.java b/src/test/java/com/google/devtools/build/lib/packages/util/LoadingMock.java index 0c924aaec3..d50b43d642 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/LoadingMock.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/LoadingMock.java @@ -33,7 +33,7 @@ public class LoadingMock { public PackageFactoryBuilderWithSkyframeForTesting getPackageFactoryBuilderForTesting( BlazeDirectories directories) { return (PackageFactoryBuilderWithSkyframeForTesting) - TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING.builder(); + TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING.builder(directories); } public ConfiguredRuleClassProvider createRuleClassProvider() { diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java index b934efe85e..4863530e00 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java @@ -60,9 +60,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for package loading. - */ +/** Tests for package loading. */ @RunWith(JUnit4.class) public class PackageCacheTest extends FoundationTestCase { @@ -76,6 +74,10 @@ public class PackageCacheTest extends FoundationTestCase { initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ true); } + /** + * @param doPackageLoadingChecks when true, a PackageLoader will be called after each package load + * this test performs, and the results compared to SkyFrame's result. + */ private void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Exception { analysisMock = AnalysisMock.get(); ruleClassProvider = analysisMock.createRuleClassProvider(); @@ -110,8 +112,7 @@ public class PackageCacheTest extends FoundationTestCase { } private void setUpSkyframe( - PackageCacheOptions packageCacheOptions, - SkylarkSemanticsOptions skylarkSemanticsOptions) { + PackageCacheOptions packageCacheOptions, SkylarkSemanticsOptions skylarkSemanticsOptions) { PathPackageLocator pkgLocator = PathPackageLocator.create( null, @@ -136,8 +137,8 @@ public class PackageCacheTest extends FoundationTestCase { } private OptionsParser parse(String... options) throws Exception { - OptionsParser parser = OptionsParser.newOptionsParser( - PackageCacheOptions.class, SkylarkSemanticsOptions.class); + OptionsParser parser = + OptionsParser.newOptionsParser(PackageCacheOptions.class, SkylarkSemanticsOptions.class); parser.parse("--default_visibility=public"); parser.parse(options); @@ -160,9 +161,7 @@ public class PackageCacheTest extends FoundationTestCase { } protected void setOptions(String... options) throws Exception { - setUpSkyframe( - parsePackageCacheOptions(options), - parseSkylarkSemanticsOptions(options)); + setUpSkyframe(parsePackageCacheOptions(options), parseSkylarkSemanticsOptions(options)); } private PackageManager getPackageManager() { @@ -176,8 +175,8 @@ public class PackageCacheTest extends FoundationTestCase { private Package getPackage(String packageName) throws NoSuchPackageException, InterruptedException { - return getPackageManager().getPackage(reporter, - PackageIdentifier.createInMainRepo(packageName)); + return getPackageManager() + .getPackage(reporter, PackageIdentifier.createInMainRepo(packageName)); } private Target getTarget(Label label) @@ -222,9 +221,8 @@ public class PackageCacheTest extends FoundationTestCase { @Test public void testGetNonexistentPackage() throws Exception { - checkGetPackageFails("not-there", - "no such package 'not-there': " - + "BUILD file not found on package path"); + checkGetPackageFails( + "not-there", "no such package 'not-there': " + "BUILD file not found on package path"); } @Test @@ -250,34 +248,32 @@ public class PackageCacheTest extends FoundationTestCase { getTarget("//pkg1:not-there"); fail(); } catch (NoSuchTargetException e) { - assertThat(e).hasMessage("no such target '//pkg1:not-there': target 'not-there' " - + "not declared in package 'pkg1' defined by /workspace/pkg1/BUILD"); + assertThat(e) + .hasMessage( + "no such target '//pkg1:not-there': target 'not-there' " + + "not declared in package 'pkg1' defined by /workspace/pkg1/BUILD"); } } /** - * A missing package is one for which no BUILD file can be found. The - * PackageCache caches failures of this kind until the next sync. + * A missing package is one for which no BUILD file can be found. The PackageCache caches failures + * of this kind until the next sync. */ @Test public void testRepeatedAttemptsToParseMissingPackage() throws Exception { - checkGetPackageFails("missing", - "no such package 'missing': " - + "BUILD file not found on package path"); + checkGetPackageFails( + "missing", "no such package 'missing': " + "BUILD file not found on package path"); // Still missing: - checkGetPackageFails("missing", - "no such package 'missing': " - + "BUILD file not found on package path"); + checkGetPackageFails( + "missing", "no such package 'missing': " + "BUILD file not found on package path"); // Update the BUILD file on disk so "missing" is no longer missing: - scratch.file("missing/BUILD", - "# an ok build file"); + scratch.file("missing/BUILD", "# an ok build file"); // Still missing: - checkGetPackageFails("missing", - "no such package 'missing': " - + "BUILD file not found on package path"); + checkGetPackageFails( + "missing", "no such package 'missing': " + "BUILD file not found on package path"); invalidatePackages(); @@ -297,7 +293,7 @@ public class PackageCacheTest extends FoundationTestCase { * *

Note: since the PackageCache.setStrictPackageCreation method was deleted (since it wasn't * used by any significant clients) creating a "broken" build file got trickier--syntax errors are - * not enough. For now, we create an unreadable BUILD file, which will cause an IOException to be + * not enough. For now, we create an unreadable BUILD file, which will cause an IOException to be * thrown. This test seems less valuable than it once did. */ @Test @@ -316,8 +312,7 @@ public class PackageCacheTest extends FoundationTestCase { eventCollector.clear(); // Update the BUILD file on disk so "broken" is no longer broken: - scratch.overwriteFile("broken/BUILD", - "# an ok build file"); + scratch.overwriteFile("broken/BUILD", "# an ok build file"); invalidatePackages(); // resets cache of failures @@ -328,10 +323,11 @@ public class PackageCacheTest extends FoundationTestCase { @Test public void testMovedBuildFileCausesReloadAfterSync() throws Exception { - Path buildFile1 = scratch.file("pkg/BUILD", - "cc_library(name = 'foo')"); - Path buildFile2 = scratch.file("/otherroot/pkg/BUILD", - "cc_library(name = 'bar')"); + // PackageLoader doesn't support --package_path. + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); + + Path buildFile1 = scratch.file("pkg/BUILD", "cc_library(name = 'foo')"); + Path buildFile2 = scratch.file("/otherroot/pkg/BUILD", "cc_library(name = 'bar')"); setOptions("--package_path=/workspace:/otherroot"); Package oldPkg = getPackage("pkg"); @@ -386,8 +382,8 @@ public class PackageCacheTest extends FoundationTestCase { setOptions("--package_path=/workspace:/otherroot"); } - protected Path createBuildFile(Path workspace, String packageName, - String... targets) throws IOException { + protected Path createBuildFile(Path workspace, String packageName, String... targets) + throws IOException { String[] lines = new String[targets.length]; for (int i = 0; i < targets.length; i++) { @@ -397,8 +393,7 @@ public class PackageCacheTest extends FoundationTestCase { return scratch.file(workspace + "/" + packageName + "/BUILD", lines); } - private void assertLabelValidity(boolean expected, String labelString) - throws Exception { + private void assertLabelValidity(boolean expected, String labelString) throws Exception { Label label = Label.parseAbsolute(labelString); boolean actual = false; @@ -410,9 +405,16 @@ public class PackageCacheTest extends FoundationTestCase { error = e.getMessage(); } if (actual != expected) { - fail("assertLabelValidity(" + label + ") " - + actual + ", not equal to expected value " + expected - + " (error=" + error + ")"); + fail( + "assertLabelValidity(" + + label + + ") " + + actual + + ", not equal to expected value " + + expected + + " (error=" + + error + + ")"); } } @@ -425,9 +427,7 @@ public class PackageCacheTest extends FoundationTestCase { @Test public void testLocationForLabelCrossingSubpackage() throws Exception { scratch.file("e/f/BUILD"); - scratch.file("e/BUILD", - "# Whatever", - "filegroup(name='fg', srcs=['f/g'])"); + scratch.file("e/BUILD", "# Whatever", "filegroup(name='fg', srcs=['f/g'])"); reporter.removeHandler(failFastHandler); List events = getPackage("e").getEvents(); assertThat(events).hasSize(1); @@ -437,6 +437,9 @@ public class PackageCacheTest extends FoundationTestCase { /** Static tests (i.e. no changes to filesystem, nor calls to sync). */ @Test public void testLabelValidity() throws Exception { + // PackageLoader doesn't support --package_path. + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); + reporter.removeHandler(failFastHandler); setUpCacheWithTwoRootLocator(); @@ -476,9 +479,8 @@ public class PackageCacheTest extends FoundationTestCase { @Test public void testAddedBuildFileCausesLabelToBecomeInvalid() throws Exception { reporter.removeHandler(failFastHandler); - scratch.file("pkg/BUILD", - " cc_library(name = 'foo', ", - " srcs = ['x/y.cc'])"); + scratch.file( + "pkg/BUILD", " cc_library(name = 'foo', ", " srcs = ['x/y.cc'])"); assertLabelValidity(true, "//pkg:x/y.cc"); @@ -491,9 +493,10 @@ public class PackageCacheTest extends FoundationTestCase { invalidatePackages(); // now: - assertPackageLoadingFails("pkg", + assertPackageLoadingFails( + "pkg", "Label '//pkg:x/y.cc' crosses boundary of subpackage 'pkg/x' " - + "(perhaps you meant to put the colon here: '//pkg/x:y.cc'?)"); + + "(perhaps you meant to put the colon here: '//pkg/x:y.cc'?)"); } @Test @@ -513,9 +516,10 @@ public class PackageCacheTest extends FoundationTestCase { assertLabelValidity(true, "//c/d:foo.txt"); // ...and this crosses package boundaries: assertLabelValidity(false, "//c:d/x"); - assertPackageLoadingFails("c", + assertPackageLoadingFails( + "c", "Label '//c:d/x' crosses boundary of subpackage 'c/d' (have you deleted c/d/BUILD? " - + "If so, use the --deleted_packages=c/d option)"); + + "If so, use the --deleted_packages=c/d option)"); assertThat(getPackageManager().isPackage(reporter, PackageIdentifier.createInMainRepo("c/d"))) .isTrue(); @@ -532,8 +536,9 @@ public class PackageCacheTest extends FoundationTestCase { getPackage("c/d"); fail(); } catch (NoSuchPackageException e) { - assertThat(e).hasMessage( - "no such package 'c/d': Package is considered deleted due to --deleted_packages"); + assertThat(e) + .hasMessage( + "no such package 'c/d': Package is considered deleted due to --deleted_packages"); } // Labels in the subpackage are no longer valid... @@ -544,7 +549,8 @@ public class PackageCacheTest extends FoundationTestCase { @Test public void testPackageFeatures() throws Exception { - scratch.file("peach/BUILD", + scratch.file( + "peach/BUILD", "package(features = ['crosstool_default_false'])", "cc_library(name = 'cc', srcs = ['cc.cc'])"); Rule cc = (Rule) getTarget("//peach:cc"); @@ -556,11 +562,7 @@ public class PackageCacheTest extends FoundationTestCase { reporter.removeHandler(failFastHandler); setOptions("--package_path=.:."); scratch.file("x/y/BUILD"); - scratch.file("x/BUILD", - "genrule(name = 'x',", - "srcs = [],", - "outs = ['y/z.h'],", - "cmd = '')"); + scratch.file("x/BUILD", "genrule(name = 'x',", "srcs = [],", "outs = ['y/z.h'],", "cmd = '')"); Package p = getPackage("x"); assertThat(p.containsErrors()).isTrue(); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java index d685a464bd..478149674c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java @@ -72,6 +72,17 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class AndroidBinaryTest extends AndroidBuildViewTestCase { + @Before + public final void turnOffPackageLoadingChecks() throws Exception { + // By default, PackageLoader loads every package the test harness loads, in order to verify + // the PackageLoader works correctly. In this test, however, PackageLoader sometimes fails to + // load packages and causes the test to become flaky. + // Since PackageLoader gets generally good coverage from the rest of Bazel's tests, and because + // we believe there's nothing special from the point of view of package loading in this test, + // we disable this verification here. + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); + } + @Before public void createFiles() throws Exception { scratch.file("java/android/BUILD", diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java index cc23c7cf83..2c832dd367 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java @@ -44,6 +44,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -51,6 +52,17 @@ import org.junit.runners.JUnit4; /** Test case for apple_binary. */ @RunWith(JUnit4.class) public class AppleBinaryTest extends ObjcRuleTestCase { + @Before + public final void turnOffPackageLoadingChecks() throws Exception { + // By default, PackageLoader loads every package the test harness loads, in order to verify + // the PackageLoader works correctly. In this test, however, PackageLoader sometimes fails to + // load packages and causes the test to become flaky. + // Since PackageLoader gets generally good coverage from the rest of Bazel's tests, and because + // we believe there's nothing special from the point of view of package loading in this test, + // we disable this verification here. + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); + } + static final RuleType RULE_TYPE = new RuleType("apple_binary") { @Override Iterable requiredAttributes(Scratch scratch, String packageDir, diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index f3ab343655..0c9357d943 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -117,7 +117,7 @@ public class RepositoryDelegatorTest extends FoundationTestCase { new WorkspaceFileFunction( TestRuleClassProvider.getRuleClassProvider(), TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING - .builder() + .builder(directories) .build( TestRuleClassProvider.getRuleClassProvider(), root.getFileSystem()), directories)) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java index 3b83fa89cf..681e4aac59 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java @@ -111,7 +111,7 @@ abstract class ArtifactFunctionTestCase { new WorkspaceFileFunction( TestRuleClassProvider.getRuleClassProvider(), TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING - .builder() + .builder(directories) .build( TestRuleClassProvider.getRuleClassProvider(), root.getFileSystem()), directories)) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 6a8543fd55..2c0c77264e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -166,7 +166,7 @@ public class FileFunctionTest { new WorkspaceFileFunction( TestRuleClassProvider.getRuleClassProvider(), TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING - .builder() + .builder(directories) .build(TestRuleClassProvider.getRuleClassProvider(), fs), directories)) .put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 0edfea3023..a0c16e4972 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -132,10 +132,13 @@ public class FilesystemValueCheckerTest { BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY)); skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(TestRuleClassProvider.getRuleClassProvider())); - skyFunctions.put(SkyFunctions.WORKSPACE_FILE, - new WorkspaceFileFunction(TestRuleClassProvider.getRuleClassProvider(), - TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING.builder().build( - TestRuleClassProvider.getRuleClassProvider(), fs), + skyFunctions.put( + SkyFunctions.WORKSPACE_FILE, + new WorkspaceFileFunction( + TestRuleClassProvider.getRuleClassProvider(), + TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING + .builder(directories) + .build(TestRuleClassProvider.getRuleClassProvider(), fs), directories)); skyFunctions.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java index ac45a7e371..679d276e48 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunctionTest.java @@ -51,6 +51,7 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase { @Before public final void createSkyframeExecutor() throws Exception { + initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); skyframeExecutor = getSkyframeExecutor(); } @@ -63,7 +64,7 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase { private RecursivePkgValue buildRecursivePkgValue(Path root, PathFragment rootRelativePath) throws Exception { - return buildRecursivePkgValue(root, rootRelativePath, ImmutableSet.of()); + return buildRecursivePkgValue(root, rootRelativePath, ImmutableSet.of()); } private RecursivePkgValue buildRecursivePkgValue( @@ -139,8 +140,7 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase { WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph()); assertThat( exists( - buildRecursivePkgKey( - rootDirectory, excludedPathFragment, ImmutableSet.of()), + buildRecursivePkgKey(rootDirectory, excludedPathFragment, ImmutableSet.of()), graph)) .isFalse(); @@ -148,8 +148,7 @@ public class RecursivePkgFunctionTest extends BuildViewTestCase { // because that key was evaluated. assertThat( exists( - buildRecursivePkgKey( - rootDirectory, PathFragment.create("a/c"), ImmutableSet.of()), + buildRecursivePkgKey(rootDirectory, PathFragment.create("a/c"), ImmutableSet.of()), graph)) .isTrue(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 33485277c7..8554b25097 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -220,7 +220,7 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { new WorkspaceFileFunction( TestRuleClassProvider.getRuleClassProvider(), TestConstants.PACKAGE_FACTORY_BUILDER_FACTORY_FOR_TESTING - .builder() + .builder(directories) .build( TestRuleClassProvider.getRuleClassProvider(), scratch.getFileSystem()), diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java index 1ac5266802..31c25f5baf 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java @@ -37,28 +37,26 @@ import org.junit.Test; /** Abstract base class of a unit test for a {@link AbstractPackageLoader} implementation. */ public abstract class AbstractPackageLoaderTest { - private Path pkgRoot; + private Path workspaceDir; protected StoredEventHandler handler; - protected PackageLoader pkgLoader; + protected FileSystem fs; + private Reporter reporter; @Before public final void init() throws Exception { - FileSystem fs = new InMemoryFileSystem(); - pkgRoot = fs.getPath("/").getRelative("pkgRoot"); - FileSystemUtils.createDirectoryAndParents(pkgRoot); - Reporter reporter = new Reporter(new EventBus()); + fs = new InMemoryFileSystem(); + workspaceDir = fs.getPath("/workspace/"); + workspaceDir.createDirectoryAndParents(); + reporter = new Reporter(new EventBus()); handler = new StoredEventHandler(); reporter.addHandler(handler); - pkgLoader = makeFreshBuilder(pkgRoot) - .useDefaultSkylarkSemantics() - .setReporter(reporter) - .build(); } - protected abstract AbstractPackageLoader.Builder makeFreshBuilder(Path pkgRoot); + protected abstract AbstractPackageLoader.Builder newPackageLoaderBuilder(Path workspaceDir); @Test public void simpleNoPackage() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("nope")); try { pkgLoader.loadPackage(pkgId); @@ -73,6 +71,7 @@ public abstract class AbstractPackageLoaderTest { @Test public void simpleBadPackage() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); file("bad/BUILD", "invalidBUILDsyntax"); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("bad")); Package badPkg = pkgLoader.loadPackage(pkgId); @@ -83,18 +82,20 @@ public abstract class AbstractPackageLoaderTest { @Test public void simpleGoodPackage() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); file("good/BUILD", "sh_library(name = 'good')"); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("good")); Package goodPkg = pkgLoader.loadPackage(pkgId); assertThat(goodPkg.containsErrors()).isFalse(); - assertThat( - goodPkg.getTarget("good").getAssociatedRule().getRuleClass()).isEqualTo("sh_library"); + assertThat(goodPkg.getTarget("good").getAssociatedRule().getRuleClass()) + .isEqualTo("sh_library"); assertNoEvents(goodPkg.getEvents()); assertNoEvents(handler.getEvents()); } @Test public void simpleMultipleGoodPackage() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); file("good1/BUILD", "sh_library(name = 'good1')"); file("good2/BUILD", "sh_library(name = 'good2')"); PackageIdentifier pkgId1 = PackageIdentifier.createInMainRepo(PathFragment.create("good1")); @@ -114,6 +115,7 @@ public abstract class AbstractPackageLoaderTest { @Test public void loadPackagesToleratesDuplicates() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); file("good1/BUILD", "sh_library(name = 'good1')"); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("good1")); ImmutableMap pkgs = @@ -127,29 +129,33 @@ public abstract class AbstractPackageLoaderTest { @Test public void simpleGoodPackage_Skylark() throws Exception { - file("good/good.bzl", - "def f(x):", - " native.sh_library(name = x)"); - file("good/BUILD", - "load('//good:good.bzl', 'f')", - "f('good')"); + PackageLoader pkgLoader = newPackageLoader(); + file("good/good.bzl", "def f(x):", " native.sh_library(name = x)"); + file("good/BUILD", "load('//good:good.bzl', 'f')", "f('good')"); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("good")); Package goodPkg = pkgLoader.loadPackage(pkgId); assertThat(goodPkg.containsErrors()).isFalse(); - assertThat( - goodPkg.getTarget("good").getAssociatedRule().getRuleClass()).isEqualTo("sh_library"); + assertThat(goodPkg.getTarget("good").getAssociatedRule().getRuleClass()) + .isEqualTo("sh_library"); assertNoEvents(goodPkg.getEvents()); assertNoEvents(handler.getEvents()); } protected Path path(String rootRelativePath) { - return pkgRoot.getRelative(PathFragment.create(rootRelativePath)); + return workspaceDir.getRelative(PathFragment.create(rootRelativePath)); } protected Path file(String fileName, String... contents) throws Exception { Path path = path(fileName); - FileSystemUtils.createDirectoryAndParents(path.getParentDirectory()); + path.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContentAsLatin1(path, Joiner.on("\n").join(contents)); return path; } + + protected PackageLoader newPackageLoader() { + return newPackageLoaderBuilder(workspaceDir) + .useDefaultSkylarkSemantics() + .setReporter(reporter) + .build(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/packages/BUILD index 105843b6de..137ae9d90d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -17,11 +17,14 @@ java_test( "BazelPackageLoaderTest.java", ], deps = [ + "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:events", "//src/main/java/com/google/devtools/build/lib:packages", "//src/main/java/com/google/devtools/build/lib/skyframe/packages", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", + "//src/test/java/com/google/devtools/build/lib:analysis_testutil", + "//src/test/java/com/google/devtools/build/lib:packages_testutil", "//src/test/java/com/google/devtools/build/lib:testutil", "//third_party:guava", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java index fb53082a35..a676586e5b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java @@ -16,11 +16,15 @@ package com.google.devtools.build.lib.skyframe.packages; import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents; +import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -33,13 +37,47 @@ import org.junit.runners.JUnit4; */ @RunWith(JUnit4.class) public final class BazelPackageLoaderTest extends AbstractPackageLoaderTest { + + private Path installBase; + private Path outputBase; + + @Before + public void setUp() throws Exception { + installBase = fs.getPath("/installBase/"); + installBase.createDirectoryAndParents(); + outputBase = fs.getPath("/outputBase/"); + outputBase.createDirectoryAndParents(); + Path embeddedBinaries = ServerDirectories.getEmbeddedBinariesRoot(installBase); + embeddedBinaries.createDirectoryAndParents(); + + mockEmbeddedTools(embeddedBinaries); + } + + private void mockEmbeddedTools(Path embeddedBinaries) throws IOException { + Path tools = embeddedBinaries.getRelative("embedded_tools"); + tools.getRelative("tools/cpp").createDirectoryAndParents(); + tools.getRelative("tools/osx").createDirectoryAndParents(); + FileSystemUtils.writeIsoLatin1(tools.getRelative("WORKSPACE"), ""); + FileSystemUtils.writeIsoLatin1(tools.getRelative("tools/cpp/BUILD"), ""); + FileSystemUtils.writeIsoLatin1( + tools.getRelative("tools/cpp/cc_configure.bzl"), + "def cc_configure(*args, **kwargs):", + " pass"); + FileSystemUtils.writeIsoLatin1(tools.getRelative("tools/osx/BUILD"), ""); + FileSystemUtils.writeIsoLatin1( + tools.getRelative("tools/osx/xcode_configure.bzl"), + "def xcode_configure(*args, **kwargs):", + " pass"); + } + @Override - protected BazelPackageLoader.Builder makeFreshBuilder(Path pkgRoot) { - return BazelPackageLoader.builder(pkgRoot); + protected AbstractPackageLoader.Builder newPackageLoaderBuilder(Path workspaceDir) { + return BazelPackageLoader.builder(workspaceDir, installBase, outputBase); } @Test public void simpleLocalRepositoryPackage() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); file("WORKSPACE", "local_repository(name = 'r', path='r')"); file("r/WORKSPACE", "workspace(name = 'r')"); file("r/good/BUILD", "sh_library(name = 'good')"); @@ -52,4 +90,22 @@ public final class BazelPackageLoaderTest extends AbstractPackageLoaderTest { assertNoEvents(goodPkg.getEvents()); assertNoEvents(handler.getEvents()); } + + @Test + public void newLocalRepository() throws Exception { + PackageLoader pkgLoader = newPackageLoader(); + file( + "WORKSPACE", + "new_local_repository(name = 'r', path = '/r', " + + "build_file_content = 'sh_library(name = \"good\")')"); + fs.getPath("/r").createDirectoryAndParents(); + PackageIdentifier pkgId = + PackageIdentifier.create(RepositoryName.create("@r"), PathFragment.create("")); + Package goodPkg = pkgLoader.loadPackage(pkgId); + assertThat(goodPkg.containsErrors()).isFalse(); + assertThat(goodPkg.getTarget("good").getAssociatedRule().getRuleClass()) + .isEqualTo("sh_library"); + assertNoEvents(goodPkg.getEvents()); + assertNoEvents(handler.getEvents()); + } } diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java b/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java index 2bd6cf6d8e..6f9cd6170f 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java @@ -17,18 +17,16 @@ import com.google.common.base.Function; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.packages.BazelPackageLoader; import com.google.devtools.build.lib.skyframe.packages.PackageLoader; import com.google.devtools.build.lib.syntax.SkylarkSemantics; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; /** * A Package.Builder.Helper for use in tests that a sanity check with {@link BazelPackageLoader} for @@ -36,9 +34,12 @@ import com.google.devtools.build.lib.vfs.PathFragment; */ public class BazelPackageBuilderHelperForTesting implements Package.Builder.Helper { private final RuleClassProvider ruleClassProvider; + private final BlazeDirectories directories; - public BazelPackageBuilderHelperForTesting(RuleClassProvider ruleClassProvider) { + public BazelPackageBuilderHelperForTesting( + RuleClassProvider ruleClassProvider, BlazeDirectories directories) { this.ruleClassProvider = ruleClassProvider; + this.directories = directories; } @Override @@ -66,23 +67,14 @@ public class BazelPackageBuilderHelperForTesting implements Package.Builder.Help RuleClassProvider ruleClassProvider, SkylarkSemantics skylarkSemantics) { PackageIdentifier pkgId = pkg.getPackageIdentifier(); - if (pkgId.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER) - || !pkg.getPackageIdentifier().getRepository().isMain() - || PackageFunction.isDefaultsPackage(pkg.getPackageIdentifier())) { - // TODO(nharmata): Support these packages. - return; - } - int numNameSegments = pkg.getNameFragment().segmentCount(); - PathFragment fullFilenameFragment = pkg.getFilename().asFragment(); - int numFullFilenameFragmentSegments = fullFilenameFragment.segmentCount(); - Path workspaceRoot = pkg.getFilename().getFileSystem().getPath( - fullFilenameFragment.subFragment( - 0, - numFullFilenameFragmentSegments - (numNameSegments + 1))); - PackageLoader packageLoader = BazelPackageLoader.builder(workspaceRoot) - .setSkylarkSemantics(skylarkSemantics) - .setRuleClassProvider(ruleClassProvider) - .build(); + PackageLoader packageLoader = + BazelPackageLoader.builder( + directories.getWorkspace(), + directories.getInstallBase(), + directories.getOutputBase()) + .setSkylarkSemantics(skylarkSemantics) + .setRuleClassProvider(ruleClassProvider) + .build(); Package newlyLoadedPkg; try { newlyLoadedPkg = packageLoader.loadPackage(pkg.getPackageIdentifier()); @@ -97,17 +89,31 @@ public class BazelPackageBuilderHelperForTesting implements Package.Builder.Help ImmutableSet.copyOf( Iterables.transform(newlyLoadedPkg.getTargets().values(), TARGET_TO_LABEL)); if (!targetsInPkg.equals(targetsInNewlyLoadedPkg)) { - throw new IllegalStateException(String.format( - "The Package for %s had a different set of targets ( - " - + " = %s, - = %s) when " - + "loaded normally during execution of the current test than it did when loaded via " - + "BazelPackageLoader (done automatically by the BazelPackageBuilderHelperForTesting " - + "hook). This either means: (i) Skyframe package loading semantics have diverged from " - + "BazelPackageLoader semantics (ii) The test in question is doing something that " - + "confuses BazelPackageBuilderHelperForTesting.", - pkgId, - Sets.difference(targetsInPkg, targetsInNewlyLoadedPkg), - Sets.difference(targetsInNewlyLoadedPkg, targetsInPkg))); + Sets.SetView