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

3 files changed, 123 insertions, 10 deletions

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<ImmutableSet<PackageIdentifier>> deletedPackages;
   private MemoizingEvaluator evaluator;
   private SequentialBuildDriver driver;
+  private RecordingDifferencer differencer;
 
   @Before
   public final void setUp() throws Exception  {
+    AnalysisMock analysisMock = AnalysisMock.get();
+
     AtomicReference<PathPackageLocator> pkgLocator =
         new AtomicReference<>(new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)));
     deletedPackages = new AtomicReference<>(ImmutableSet.<PackageIdentifier>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<SkyFunctionName, SkyFunction> 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.<EnvironmentExtension>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<String, RepositoryFunction> 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.<RepositoryName, PathFragment>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
         .<ContainingPackageLookupValue>evaluate(
             ImmutableList.of(key),
@@ -132,6 +183,36 @@ public class ContainingPackageLookupFunctionTest extends FoundationTestCase {
   }
 
   @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;
     ContainingPackageLookupValue valueA2 = 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<String>()
+            .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'])");