Respect main repository mapping in platform_mappings

[fmeum]

Fixes #17127

[fmeum]

I'm rather clueless about the remaining test failures as these tests don't use non-trivial repository mappings:

Unrecoverable error while evaluating node 'PACKAGE:external' (requested by nodes 'Key{repoName=@, rootModuleShouldSeeWorkspaceRepos=true}')
java.lang.RuntimeException: 
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:657)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:382)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.IllegalStateException: The Package for external had a different set of targets (<targetsInPkg> - <targetsInNewlyLoadedPkg> = [], <targetsInNewlyLoadedPkg> - <targetsInPkg> = [//external:android_gmaven_r8, //external:com_google_protobuf, //external:local_config_xcode, //external:rules_java]) when loaded normally during execution of the current test than it did when loaded via BazelPackageLoader (done automatically by the BazelPackageLoadingListenerForTesting hook). This either means: (i) Skyframe package loading semantics have diverged from BazelPackageLoader semantics (ii) The test in question is doing something that confuses BazelPackageLoadingListenerForTesting.
	at com.google.devtools.build.lib.testutil.BazelPackageLoadingListenerForTesting.sanityCheckBazelPackageLoader(BazelPackageLoadingListenerForTesting.java:104)
	at com.google.devtools.build.lib.testutil.BazelPackageLoadingListenerForTesting.onLoadingCompleteAndSuccessful(BazelPackageLoadingListenerForTesting.java:65)
	at com.google.devtools.build.lib.packages.PackageFactory.afterDoneLoadingPackage(PackageFactory.java:583)
	at com.google.devtools.build.lib.skyframe.PackageFunction.getExternalPackage(PackageFunction.java:356)
	at com.google.devtools.build.lib.skyframe.PackageFunction.compute(PackageFunction.java:395)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:575)
	... 4 more

[Wyverald]

last time I tried to fix this, I ran into some circular dependency problems in Bazel's test setup code. Don't remember exactly what was in the cycle, but I gave up fairly quickly and thought nobody would be using platform mappings anyway. Guess I was wrong :P

[fmeum]

I do rely on them for macOS cross-compilation. I will take another stab at fixing these failures.

[fmeum]

@katre Do you happen to have an idea what to do about the test failures?

[katre]

Not offhand, but I'll try and take a look and see if I can figure anything out.

[brentleyjones]

Friendly bump on this.

[katre]

Okay, I rebased this to the current master and looked at the failures.

The core problem is that the expected error (in the case of StarlarkRuleContextTest.testLoadBlockRepositoryRedefinition, the error for redefining a repository) is being swallowed by a different error, and either the message is dropped or the type is changed.

I don't know why this PR caused a change in error handling: possibly things are being evaluated earlier? The fix is probably to handle the errors properly, but I don't know how.

@Wyverald, any ideas?

[brentleyjones]

Ping 🙏

[Wyverald]

I'll try to carve some time next week to look at this again. AFAIK what was hard about it last time (see above) is still hard today (see test failures...).

[fmeum]

I got this down to a single type of failure:

1) testMappingFileIsRead_fromAlternatePackagePath(com.google.devtools.build.lib.skyframe.PlatformMappingFunctionTest)
java.lang.AssertionError: ERROR /DEFAULT.WORKSPACE:11:17: fetching local_repository rule //external:rules_java_builtin: java.io.IOException: The repository's path is "/workspace/rules_java" (absolute: "/workspace/rules_java") but it does not exist or is not a directory.
	at org.junit.Assert.fail(Assert.java:89)
	at com.google.devtools.build.lib.testutil.FoundationTestCase$1.handle(FoundationTestCase.java:61)
	at com.google.devtools.build.lib.events.Reporter.handle(Reporter.java:127)
	at com.google.devtools.build.lib.events.Event.reportTo(Event.java:71)
	at com.google.devtools.build.skyframe.ParallelEvaluatorContext$NestedSetEventReceiver.accept(ParallelEvaluatorContext.java:210)
	at com.google.devtools.build.skyframe.ParallelEvaluatorContext$NestedSetEventReceiver.accept(ParallelEvaluatorContext.java:200)
	at com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor.visitRaw(NestedSetVisitor.java:77)
	at com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor.visit(NestedSetVisitor.java:56)
	at com.google.devtools.build.skyframe.SkyFunctionEnvironment.reportEventsAndGetEventsToStore(SkyFunctionEnvironment.java:317)
	at com.google.devtools.build.skyframe.SkyFunctionEnvironment.commitAndGetParents(SkyFunctionEnvironment.java:882)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:582)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:399)
	at java.base/java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1407)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

@meteorcloudy This looks like something you may know how to solve.

[meteorcloudy]

This is because for the new workspace, it's missing setup from https://cs.opensource.google/bazel/bazel/+/refs/heads/master:src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java;l=405-408

I tired to reuse the WORKSPACE file, it seems working:

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
index 7de265990e..d9ff4cbfb5 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
@@ -127,7 +127,7 @@ public final class PlatformMappingFunctionTest extends BuildViewTestCase {
   @Test
   public void testMappingFileIsRead_fromAlternatePackagePath() throws Exception {
     scratch.setWorkingDir("/other/package/path");
-    scratch.file("WORKSPACE");
+    scratch.copyFile(rootDirectory.getRelative("WORKSPACE").getPathString(), "WORKSPACE");
     setPackageOptions("--package_path=/other/package/path");
     scratch.file(
         "my_mapping_file",

[fmeum]

@meteorcloudy Thanks, that fixed the test.

I could of course add that line to

bazel/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java

Line 150 in 36d0275

public void handlesNoWorkspaceFile() throws Exception {

but the whole point of that test seems to be that there is no WORKSPACE file. I am not sure how this can work, but I also don't know anything about --package_path. Can this test be dropped?

[meteorcloudy]

I believe --package_path is never meant for Bazel, but is used internally. /cc @justinhorvitz @lberki

Could adding a --override_repository=rules_java_builtin=/workspace/rules_java_workspace work?

[fmeum]

Uh, sorry. I don't know anything off the bat. My best idea would be useLoadingOptions("--override_repository=rules_java_builtin=<some scratch file system path>"), but I assume you tried that already?

I did, that fails with Unrecognized option: ....

How come this becomes a problem with this pull request, but is not one at HEAD? I have only given the change the most cursory of glances, but it looks like it doesn't add anything new about Java, but then how come that test works now, but this pull request makes it require a @rules_java_builtin repository?

This happens because PlatformMappingFunction now depends on RepositoryMappingFunction to get the main repository mapping, which in turn triggers the evaluation of the WORKSPACE file.

[lberki]

Oh, I see. That's unfortunate. I don't have any better idea than disabling the test :(

If I really wanted to, I'd poke at SkyframeExecutor to see if it's possible to inject a mock WORKSPACE value, but then it'll get further away from production code... so I vote for disabling the test and adding a comment that tells why that is.

[fmeum]

@Wyverald With @lberki's okay, this should be ready to review and merge.

[Wyverald]

quick question -- this change shouldn't be necessary, right? everything in the platform mapping file should be anchored to the main repo? (it looks like you're passing around a mainRepoContext that always has RepositoryName.MAIN, instead of a mainRepoMapping; I think the latter would be less cognitive burden.)

[fmeum]

I found it confusing that RepoContext is not a valid conversionContext, but RepositoryMapping is - it doesn't matter that much though and RepoContext does have redundant information, so I reverted this part.

[fmeum]

@Wyverald Friendly ping

[Wyverald]

thanks for fixing this! and sorry for the long delay.

I'm still not entirely sure how you worked around the nasty dependency cycles in the tests, but for now I'll just be happy that you did and move along :)

[fmeum]

I'm still not entirely sure how you worked around the nasty dependency cycles in the tests, but for now I'll just be happy that you did and move along :)

Glad that you aren't sure since I have no idea :-D I suspect that something changed on master between my two attempts. This may blow up when we try to cherry-pick it.

[brentleyjones]

@bazel-io flag

[iancha1992]

@bazel-io fork 6.4.0

[iancha1992]

@fmeum @brentleyjones @Wyverald @lberki @katre @meteorcloudy We tried to cherry-pick this change to release-6.4.0. However, there were some conflicts. Can you please take a look?

1. src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java
   We currently have in release-6.4.0 branch,

static Mappings parse(List<String> lines) throws PlatformMappingException {

However, we expected the below before the cherry-picking

static Mappings parse(Environment env, List<String> lines) throws PlatformMappingException {

2. src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java

if (platformsToFlags == null) {
  return null;
}

is missing, but should be present already in the release-6.4.0 branch before cherry-picking

3. src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java

private static class SkyframeTargetLoader implements StarlarkOptionsParser.BuildSettingLoader

is missing, but should be present already in the release-6.4.0 branch before cherry-picking

4. src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java
   In the current release-6.4.0 branch, we have below

private OptionsParsingResult parse(Iterable<String> args) throws OptionsParsingException {

But we expected below before the cherry-picking

private OptionsParsingResult parse(NativeAndStarlarkFlags args) throws OptionsParsingException {

5. src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

public void actionRerunsOnRepoMappingChange_workspaceName() throws Exception {

should already be in the release-6.4.0 branch.

6. src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionParserTest.java
   We currently have below in the release-6.4.0 branch

return PlatformMappingFunction.parse(ImmutableList.copyOf(lines));

But, expected below before cherry-picking

return PlatformMappingFunction.parse(/* env= */ null, ImmutableList.copyOf(lines));

7. src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java

public void starlarkFlagMapping() throws Exception {

is currently missing in the release-6.4.0 branch

8. src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java
   We currently have below in the release-6.4.0 branch

private static final Label PLATFORM1 = Label.parseAbsoluteUnchecked("//platforms:one");
private static final Label PLATFORM2 = Label.parseAbsoluteUnchecked("//platforms:two");

But we expected below before cherry-picking

private static final Label PLATFORM1 = Label.parseCanonicalUnchecked("//platforms:one");
  private static final Label PLATFORM2 = Label.parseCanonicalUnchecked("//platforms:two");

cc: @bazelbuild/triage

[fmeum]

@iancha1992 I cherry-picked this manually in #19495.