Skip to content

Commit

Permalink
Respect main repository mapping in platform_mappings
Browse files Browse the repository at this point in the history 
Fixes bazelbuild#17127

Closes bazelbuild#17133.

PiperOrigin-RevId: 563803704
Change-Id: I0f49fbfce624207b81a6ca4f7e564d9f3b525d54
  • Loading branch information
@fmeum
fmeum committed Sep 12, 2023
1 parent f465abd commit 7e655a7
Show file tree
Hide file tree
Showing 7 changed files with 151 additions and 59 deletions.
50 changes: 32 additions & 18 deletions src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.Label.RepoContext;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration;
import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code;
Expand Down Expand Up @@ -67,6 +69,14 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env)
PathFragment workspaceRelativeMappingPath =
platformMappingKey.getWorkspaceRelativeMappingPath();

RepositoryMappingValue mainRepositoryMappingValue =
(RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
if (mainRepositoryMappingValue == null) {
return null;
}
RepoContext mainRepoContext =
RepoContext.of(RepositoryName.MAIN, mainRepositoryMappingValue.getRepositoryMapping());

PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
if (pkgLocator == null) {
return null;
Expand Down Expand Up @@ -103,13 +113,14 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env)
throw new PlatformMappingException(e, SkyFunctionException.Transience.TRANSIENT);
}

return parse(lines).toPlatformMappingValue(optionsClasses);
return parse(lines, mainRepoContext).toPlatformMappingValue(optionsClasses);
}

if (!platformMappingKey.wasExplicitlySetByUser()) {
// If no flag was passed and the default mapping file does not exist treat this as if the
// mapping file was empty rather than an error.
return new PlatformMappingValue(ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of());
return new PlatformMappingValue(
ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of(), mainRepoContext.repoMapping());
}
throw new PlatformMappingException(
new MissingInputFileException(
Expand Down Expand Up @@ -139,7 +150,8 @@ static final class PlatformMappingException extends SkyFunctionException {
}

@VisibleForTesting
static Mappings parse(List<String> lines) throws PlatformMappingException {
static Mappings parse(List<String> lines, RepoContext mainRepoContext)
throws PlatformMappingException {
PeekingIterator<String> it =
Iterators.peekingIterator(
lines.stream()
Expand All @@ -148,7 +160,7 @@ static Mappings parse(List<String> lines) throws PlatformMappingException {
.iterator());

if (!it.hasNext()) {
return new Mappings(ImmutableMap.of(), ImmutableMap.of());
return new Mappings(ImmutableMap.of(), ImmutableMap.of(), mainRepoContext);
}

if (!it.peek().equalsIgnoreCase("platforms:") && !it.peek().equalsIgnoreCase("flags:")) {
Expand All @@ -160,28 +172,28 @@ static Mappings parse(List<String> lines) throws PlatformMappingException {

if (it.peek().equalsIgnoreCase("platforms:")) {
it.next();
platformsToFlags = readPlatformsToFlags(it);
platformsToFlags = readPlatformsToFlags(it, mainRepoContext);
}

if (it.hasNext()) {
String line = it.next();
if (!line.equalsIgnoreCase("flags:")) {
throw parsingException("Expected 'flags:' but got " + line);
}
flagsToPlatforms = readFlagsToPlatforms(it);
flagsToPlatforms = readFlagsToPlatforms(it, mainRepoContext);
}

if (it.hasNext()) {
throw parsingException("Expected end of file but got " + it.next());
}
return new Mappings(platformsToFlags, flagsToPlatforms);
return new Mappings(platformsToFlags, flagsToPlatforms, mainRepoContext);
}

private static ImmutableMap<Label, ImmutableSet<String>> readPlatformsToFlags(
PeekingIterator<String> it) throws PlatformMappingException {
PeekingIterator<String> it, RepoContext mainRepoContext) throws PlatformMappingException {
ImmutableMap.Builder<Label, ImmutableSet<String>> platformsToFlags = ImmutableMap.builder();
while (it.hasNext() && !it.peek().equalsIgnoreCase("flags:")) {
Label platform = readPlatform(it);
Label platform = readPlatform(it, mainRepoContext);
ImmutableSet<String> flags = readFlags(it);
platformsToFlags.put(platform, flags);
}
Expand All @@ -195,11 +207,11 @@ private static ImmutableMap<Label, ImmutableSet<String>> readPlatformsToFlags(
}

private static ImmutableMap<ImmutableSet<String>, Label> readFlagsToPlatforms(
PeekingIterator<String> it) throws PlatformMappingException {
PeekingIterator<String> it, RepoContext mainRepoContext) throws PlatformMappingException {
ImmutableMap.Builder<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.builder();
while (it.hasNext() && it.peek().startsWith("--")) {
ImmutableSet<String> flags = readFlags(it);
Label platform = readPlatform(it);
Label platform = readPlatform(it, mainRepoContext);
flagsToPlatforms.put(flags, platform);
}

Expand All @@ -210,17 +222,15 @@ private static ImmutableMap<ImmutableSet<String>, Label> readFlagsToPlatforms(
}
}

private static Label readPlatform(PeekingIterator<String> it) throws PlatformMappingException {
private static Label readPlatform(PeekingIterator<String> it, RepoContext mainRepoContext)
throws PlatformMappingException {
if (!it.hasNext()) {
throw parsingException("Expected platform label but got end of file");
}

String line = it.next();
try {
// It is ok for us to use an empty repository mapping in this instance because all platform
// labels used in the mapping file should be relative to the root repository. Repository
// mappings however only apply within a repository imported by the root repository.
return Label.parseAbsolute(line, /*repositoryMapping=*/ ImmutableMap.of());
return Label.parseWithRepoContext(line, mainRepoContext);
} catch (LabelSyntaxException e) {
throw parsingException("Expected platform label but got " + line, e);
}
Expand Down Expand Up @@ -260,17 +270,21 @@ private static PlatformMappingException parsingException(String message, Excepti
static final class Mappings {
final ImmutableMap<Label, ImmutableSet<String>> platformsToFlags;
final ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms;
final RepoContext mainRepoContext;

Mappings(
ImmutableMap<Label, ImmutableSet<String>> platformsToFlags,
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms) {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms,
RepoContext mainRepoContext) {
this.platformsToFlags = platformsToFlags;
this.flagsToPlatforms = flagsToPlatforms;
this.mainRepoContext = mainRepoContext;
}

PlatformMappingValue toPlatformMappingValue(
ImmutableSet<Class<? extends FragmentOptions>> optionsClasses) {
return new PlatformMappingValue(platformsToFlags, flagsToPlatforms, optionsClasses);
return new PlatformMappingValue(
platformsToFlags, flagsToPlatforms, optionsClasses, mainRepoContext.repoMapping());
}
}

Expand Down
13 changes: 10 additions & 3 deletions src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
Expand Down Expand Up @@ -141,6 +142,7 @@ public String toString() {
private final ImmutableSet<Class<? extends FragmentOptions>> optionsClasses;
private final LoadingCache<ImmutableSet<String>, OptionsParsingResult> parserCache;
private final LoadingCache<BuildConfigurationKey, BuildConfigurationKey> mappingCache;
private final RepositoryMapping mainRepositoryMapping;

/**
* Creates a new mapping value which will match on the given platforms (if a target platform is
Expand All @@ -151,18 +153,21 @@ public String toString() {
* @param flagsToPlatforms mapping from a set of command line style flags to a target platform
* that should be set if the flags match the mapped options
* @param optionsClasses default options classes that should be used for options parsing
* @param mainRepositoryMapping the main repo mapping used to parse label-valued options
*/
PlatformMappingValue(
ImmutableMap<Label, ImmutableSet<String>> platformsToFlags,
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms,
ImmutableSet<Class<? extends FragmentOptions>> optionsClasses) {
ImmutableSet<Class<? extends FragmentOptions>> optionsClasses,
RepositoryMapping mainRepositoryMapping) {
this.platformsToFlags = checkNotNull(platformsToFlags);
this.flagsToPlatforms = checkNotNull(flagsToPlatforms);
this.optionsClasses = checkNotNull(optionsClasses);
this.mainRepositoryMapping = checkNotNull(mainRepositoryMapping);
this.parserCache =
Caffeine.newBuilder()
.initialCapacity(platformsToFlags.size() + flagsToPlatforms.size())
.build(this::parse);
.build(flags -> parse(flags, this.mainRepositoryMapping));
this.mappingCache = Caffeine.newBuilder().weakKeys().build(this::computeMapping);
}

Expand Down Expand Up @@ -250,12 +255,14 @@ private OptionsParsingResult parseWithCache(ImmutableSet<String> args)
}
}

private OptionsParsingResult parse(Iterable<String> args) throws OptionsParsingException {
private OptionsParsingResult parse(Iterable<String> args, RepositoryMapping mainRepoMapping)
throws OptionsParsingException {
OptionsParser parser =
OptionsParser.builder()
.optionsClasses(optionsClasses)
// We need the ability to re-map internal options in the mappings file.
.ignoreInternalOptions(false)
.withConversionContext(mainRepoMapping)
.build();
parser.parse(ImmutableList.copyOf(args));
// TODO(schmitt): Parse starlark options as well.
Expand Down
8 changes: 6 additions & 2 deletions src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ private ImmutableList<String> getRepoMappingManifestForTarget(String label) thro

@Test
public void diamond() throws Exception {
rewriteWorkspace("workspace(name='aaa_ws')");
scratch.overwriteFile(
"MODULE.bazel",
"module(name='aaa',version='1.0')",
Expand Down Expand Up @@ -153,6 +152,9 @@ public void diamond() throws Exception {
entry.getValue());
}

// Called last as it triggers package invalidation, which requires a valid MODULE.bazel setup.
rewriteWorkspace("workspace(name='aaa_ws')");

assertThat(getRepoMappingManifestForTarget("//:aaa"))
.containsExactly(
",aaa,_main",
Expand All @@ -169,7 +171,6 @@ public void diamond() throws Exception {

@Test
public void runfilesFromToolchain() throws Exception {
rewriteWorkspace("workspace(name='main')");
scratch.overwriteFile("MODULE.bazel", "bazel_dep(name='tooled_rule',version='1.0')");
// tooled_rule offers a tooled_binary rule, which uses a toolchain backed by a binary from
// bare_rule. tooled_binary explicitly requests that runfiles from this binary are included in
Expand Down Expand Up @@ -221,6 +222,9 @@ public void runfilesFromToolchain() throws Exception {
"load('@tooled_rule//:defs.bzl', 'tooled_binary')",
"tooled_binary(name='tooled')");

// Called last as it triggers package invalidation, which requires a valid MODULE.bazel setup.
rewriteWorkspace("workspace(name='main')");

assertThat(getRepoMappingManifestForTarget("//:tooled"))
.containsExactly(
",main,_main",
Expand Down
49 changes: 46 additions & 3 deletions src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.Label.RepoContext;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -29,8 +33,10 @@
@RunWith(JUnit4.class)
public class PlatformMappingFunctionParserTest {

private static final Label PLATFORM1 = Label.parseAbsoluteUnchecked("//platforms:one");
private static final Label PLATFORM2 = Label.parseAbsoluteUnchecked("//platforms:two");
private static final Label PLATFORM1 = Label.parseCanonicalUnchecked("//platforms:one");
private static final Label PLATFORM2 = Label.parseCanonicalUnchecked("//platforms:two");
private static final Label EXTERNAL_PLATFORM =
Label.parseCanonicalUnchecked("@dep~1.0//platforms:two");

@Test
public void testParse() throws Exception {
Expand All @@ -57,6 +63,36 @@ public void testParse() throws Exception {
assertThat(mappings.flagsToPlatforms.get(ImmutableSet.of("--cpu=two"))).isEqualTo(PLATFORM2);
}

@Test
public void testParseWithRepoMapping() throws Exception {
PlatformMappingFunction.Mappings mappings =
parse(
RepositoryMapping.create(
ImmutableMap.of(
"foo", RepositoryName.MAIN, "dep", RepositoryName.create("dep~1.0")),
RepositoryName.MAIN),
"platforms:",
" @foo//platforms:one",
" --cpu=one",
" @dep//platforms:two",
" --cpu=two",
"flags:",
" --cpu=one",
" @foo//platforms:one",
" --cpu=two",
" @dep//platforms:two");

assertThat(mappings.platformsToFlags.keySet()).containsExactly(PLATFORM1, EXTERNAL_PLATFORM);
assertThat(mappings.platformsToFlags.get(PLATFORM1)).containsExactly("--cpu=one");
assertThat(mappings.platformsToFlags.get(EXTERNAL_PLATFORM)).containsExactly("--cpu=two");

assertThat(mappings.flagsToPlatforms.keySet())
.containsExactly(ImmutableSet.of("--cpu=one"), ImmutableSet.of("--cpu=two"));
assertThat(mappings.flagsToPlatforms.get(ImmutableSet.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.get(ImmutableSet.of("--cpu=two")))
.isEqualTo(EXTERNAL_PLATFORM);
}

@Test
public void testParseComment() throws Exception {
PlatformMappingFunction.Mappings mappings =
Expand Down Expand Up @@ -393,6 +429,13 @@ public void testParseFlagsDuplicateFlags() throws Exception {

private static PlatformMappingFunction.Mappings parse(String... lines)
throws PlatformMappingFunction.PlatformMappingException {
return PlatformMappingFunction.parse(ImmutableList.copyOf(lines));
return parse(RepositoryMapping.ALWAYS_FALLBACK, lines);
}

private static PlatformMappingFunction.Mappings parse(
RepositoryMapping mainRepoMapping, String... lines)
throws PlatformMappingFunction.PlatformMappingException {
return PlatformMappingFunction.parse(
ImmutableList.copyOf(lines), RepoContext.of(RepositoryName.MAIN, mainRepoMapping));
}
}
8 changes: 7 additions & 1 deletion src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void testMappingFileIsRead() throws Exception {
@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",
Expand All @@ -145,6 +145,12 @@ public void testMappingFileIsRead_fromAlternatePackagePath() throws Exception {

@Test
public void handlesNoWorkspaceFile() throws Exception {
// --package_path is not relevant for Bazel and difficult to get to work correctly with
// WORKSPACE suffixes in tests.
if (analysisMock.isThisBazel()) {
return;
}

scratch.setWorkingDir("/other/package/path");
scratch.file(
"my_mapping_file",
Expand Down
Loading

0 comments on commit 7e655a7

Please sign in to comment.