From 45932965c08e1080f1d53cf09352bec0a24cb40f Mon Sep 17 00:00:00 2001 From: John Cater Date: Wed, 6 Sep 2017 21:57:46 +0200 Subject: Update ContainingPackageLookupFunction to properly handle cases where a path crosses into a local repository and correctly report the repository-relative package. Fixes #3553. Change-Id: Ib912e69d546fb740ef8fe4c426dba30fa7776bda PiperOrigin-RevId: 167760229 --- .../skyframe/ContainingPackageLookupFunction.java | 11 +++ .../ContainingPackageLookupFunctionTest.java | 99 ++++++++++++++++++++-- .../build/lib/skylark/SkylarkRuleContextTest.java | 23 ++++- 3 files changed, 123 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java index 68c9ff8353..f3ca9881c6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.skyframe.PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; @@ -38,6 +39,16 @@ public class ContainingPackageLookupFunction implements SkyFunction { return ContainingPackageLookupValue.withContainingPackage(dir, pkgLookupValue.getRoot()); } + // Does the requested package cross into a sub-repository, which we should report via the + // correct package identifier? + if (pkgLookupValue instanceof IncorrectRepositoryReferencePackageLookupValue) { + IncorrectRepositoryReferencePackageLookupValue incorrectPackageLookupValue = + (IncorrectRepositoryReferencePackageLookupValue) pkgLookupValue; + PackageIdentifier correctPackageIdentifier = + incorrectPackageLookupValue.getCorrectedPackageIdentifier(); + return env.getValue(ContainingPackageLookupValue.key(correctPackageIdentifier)); + } + PathFragment parentDir = dir.getPackageFragment().getParentDirectory(); if (parentDir == null) { return ContainingPackageLookupValue.NONE; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java index d65779f372..bcbeb54f31 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java @@ -16,17 +16,27 @@ package com.google.devtools.build.lib.skyframe; import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.packages.PackageFactory; +import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension; +import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction; +import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule; +import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; +import com.google.devtools.build.lib.rules.repository.RepositoryFunction; +import com.google.devtools.build.lib.rules.repository.RepositoryLoaderFunction; import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; import com.google.devtools.build.lib.testutil.FoundationTestCase; -import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator; @@ -39,6 +49,7 @@ import com.google.devtools.build.skyframe.SkyKey; import java.util.HashMap; import java.util.Map; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.junit.Before; import org.junit.Test; @@ -54,26 +65,35 @@ public class ContainingPackageLookupFunctionTest extends FoundationTestCase { private AtomicReference> deletedPackages; private MemoizingEvaluator evaluator; private SequentialBuildDriver driver; + private RecordingDifferencer differencer; @Before public final void setUp() throws Exception { + AnalysisMock analysisMock = AnalysisMock.get(); + AtomicReference pkgLocator = new AtomicReference<>(new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory))); deletedPackages = new AtomicReference<>(ImmutableSet.of()); - ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper( - pkgLocator, - ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, + BlazeDirectories directories = new BlazeDirectories( - rootDirectory, rootDirectory, rootDirectory, TestConstants.PRODUCT_NAME)); + rootDirectory, outputBase, rootDirectory, analysisMock.getProductName()); + ExternalFilesHelper externalFilesHelper = + new ExternalFilesHelper( + pkgLocator, + ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, + directories); Map skyFunctions = new HashMap<>(); + skyFunctions.put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()); + skyFunctions.put( SkyFunctions.PACKAGE_LOOKUP, new PackageLookupFunction( deletedPackages, CrossRepositoryLabelViolationStrategy.ERROR, ImmutableList.of(BuildFileName.BUILD_DOT_BAZEL, BuildFileName.BUILD))); - skyFunctions.put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()); + skyFunctions.put( + SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null)); skyFunctions.put(SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES, new BlacklistedPackagePrefixesFunction()); skyFunctions.put(SkyFunctions.FILE_STATE, new FileStateFunction( @@ -83,20 +103,51 @@ public class ContainingPackageLookupFunctionTest extends FoundationTestCase { skyFunctions.put( SkyFunctions.DIRECTORY_LISTING_STATE, new DirectoryListingStateFunction(externalFilesHelper)); + RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); + skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider)); + skyFunctions.put( + SkyFunctions.WORKSPACE_FILE, + new WorkspaceFileFunction( + ruleClassProvider, + analysisMock + .getPackageFactoryBuilderForTesting() + .setEnvironmentExtensions( + ImmutableList.of( + new PackageFactory.EmptyEnvironmentExtension())) + .build(ruleClassProvider, scratch.getFileSystem()), + directories)); + skyFunctions.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()); skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); - RecordingDifferencer differencer = new RecordingDifferencer(); + skyFunctions.put( + SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction()); + ImmutableMap repositoryHandlers = + ImmutableMap.of( + LocalRepositoryRule.NAME, (RepositoryFunction) new LocalRepositoryFunction()); + skyFunctions.put( + SkyFunctions.REPOSITORY_DIRECTORY, + new RepositoryDelegatorFunction(repositoryHandlers, null, new AtomicBoolean(true))); + skyFunctions.put(SkyFunctions.REPOSITORY, new RepositoryLoaderFunction()); + + differencer = new RecordingDifferencer(); evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer); driver = new SequentialBuildDriver(evaluator); PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID()); PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); PrecomputedValue.BLACKLISTED_PACKAGE_PREFIXES_FILE.set(differencer, PathFragment.EMPTY_FRAGMENT); + PrecomputedValue.BLAZE_DIRECTORIES.set(differencer, directories); + RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set( + differencer, ImmutableMap.of()); } private ContainingPackageLookupValue lookupContainingPackage(String packageName) throws InterruptedException { - SkyKey key = - ContainingPackageLookupValue.key(PackageIdentifier.createInMainRepo(packageName)); + return lookupContainingPackage(PackageIdentifier.createInMainRepo(packageName)); + } + + private ContainingPackageLookupValue lookupContainingPackage(PackageIdentifier packageIdentifier) + throws InterruptedException { + SkyKey key = ContainingPackageLookupValue.key(packageIdentifier); return driver .evaluate( ImmutableList.of(key), @@ -131,6 +182,36 @@ public class ContainingPackageLookupFunctionTest extends FoundationTestCase { assertThat(value.getContainingPackageRoot()).isEqualTo(rootDirectory); } + @Test + public void testContainingPackageIsExternalRepositoryViaExternalRepository() throws Exception { + scratch.overwriteFile( + "WORKSPACE", + "local_repository(name='a', path='a')"); + scratch.file("a/WORKSPACE"); + scratch.file("a/BUILD"); + scratch.file("a/b/BUILD"); + ContainingPackageLookupValue value = + lookupContainingPackage( + PackageIdentifier.create(RepositoryName.create("@a"), PathFragment.create("b"))); + assertThat(value.hasContainingPackage()).isTrue(); + assertThat(value.getContainingPackageName()) + .isEqualTo(PackageIdentifier.create(RepositoryName.create("@a"), PathFragment.create("b"))); + } + + @Test + public void testContainingPackageIsExternalRepositoryViaLocalPath() throws Exception { + scratch.overwriteFile( + "WORKSPACE", + "local_repository(name='a', path='a')"); + scratch.file("a/WORKSPACE"); + scratch.file("a/BUILD"); + scratch.file("a/b/BUILD"); + ContainingPackageLookupValue value = lookupContainingPackage("a/b"); + assertThat(value.hasContainingPackage()).isTrue(); + assertThat(value.getContainingPackageName()) + .isEqualTo(PackageIdentifier.create(RepositoryName.create("@a"), PathFragment.create("b"))); + } + @Test public void testEqualsAndHashCodeContract() throws Exception { ContainingPackageLookupValue valueA1 = ContainingPackageLookupValue.NONE; diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java index a18a7d637c..e99f74ec71 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java @@ -384,7 +384,28 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { /* The error message for this case used to be wrong. */ @Test - public void testPackageBoundaryError_ExternalRepository() throws Exception { + public void testPackageBoundaryError_ExternalRepository_Boundary() throws Exception { + scratch.file("r/WORKSPACE"); + scratch.file("r/BUILD"); + scratch.overwriteFile( + "WORKSPACE", + new ImmutableList.Builder() + .addAll(analysisMock.getWorkspaceContents(mockToolsConfig)) + .add("local_repository(name='r', path='r')") + .build()); + scratch.file("BUILD", "cc_library(name = 'cclib',", " srcs = ['r/my_sub_lib.h'])"); + invalidatePackages( + /*alsoConfigs=*/ false); // Repository shuffling messes with toolchain labels. + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//:cclib"); + assertContainsEvent( + "/workspace/BUILD:2:10: Label '//:r/my_sub_lib.h' crosses boundary of " + + "subpackage '@r//'"); + } + + /* The error message for this case used to be wrong. */ + @Test + public void testPackageBoundaryError_ExternalRepository_EntirelyInside() throws Exception { scratch.file("/r/WORKSPACE"); scratch.file("/r/BUILD", "cc_library(name = 'cclib',", " srcs = ['sub/my_sub_lib.h'])"); scratch.file("/r/sub/BUILD", "cc_library(name = 'my_sub_lib', srcs = ['my_sub_lib.h'])"); -- cgit v1.2.3