Skip to content

Commit

Permalink
Collections of flags should be lists, not sets.
Browse files Browse the repository at this point in the history 
Since flag names and values can be separated by both spaces or equals signs, and because some flags can be repeated, using a set for flags can incorrectly change `--flag a --flag b` into `--flag a b`.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 604758501
Change-Id: Ib1b38d28a2341017fc944cfda85bcf4eb0e4bfb7
  • Loading branch information
@katre @copybara-github
katre authored and copybara-github committed Feb 6, 2024
1 parent cbbb0a2 commit f7dec90
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 59 deletions.
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/skyframe/config/NativeAndStarlarkFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe.config;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
Expand All @@ -34,7 +35,7 @@ public abstract class NativeAndStarlarkFlags {
/** Builder for new {@link NativeAndStarlarkFlags} instances. */
@AutoValue.Builder
public abstract static class Builder {
public abstract Builder nativeFlags(ImmutableSet<String> nativeFlags);
public abstract Builder nativeFlags(ImmutableList<String> nativeFlags);

public abstract Builder starlarkFlags(ImmutableMap<String, Object> starlarkFlags);

Expand All @@ -49,12 +50,12 @@ public abstract Builder optionsClasses(
/** Returns a new {@link Builder}. */
public static Builder builder() {
return new AutoValue_NativeAndStarlarkFlags.Builder()
.nativeFlags(ImmutableSet.of())
.nativeFlags(ImmutableList.of())
.starlarkFlags(ImmutableMap.of())
.optionsClasses(ImmutableSet.of());
}

public abstract ImmutableSet<String> nativeFlags();
public abstract ImmutableList<String> nativeFlags();

public abstract ImmutableMap<String, Object> starlarkFlags();

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/config/ParsedFlagsFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throws InterruptedException, ParsedFlagsFunctionException {
ParsedFlagsValue.Key key = (ParsedFlagsValue.Key) skyKey.argument();

ImmutableSet.Builder<String> nativeFlags = ImmutableSet.builder();
ImmutableList.Builder<String> nativeFlags = ImmutableList.builder();
ImmutableList.Builder<String> starlarkFlags = ImmutableList.builder();
for (String flagSetting : key.rawFlags()) {
if (STARLARK_SKIPPED_PREFIXES.stream().noneMatch(flagSetting::startsWith)) {
Expand Down
13 changes: 6 additions & 7 deletions src/main/java/com/google/devtools/build/lib/skyframe/config/ParsedFlagsValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.google.devtools.build.lib.skyframe.config;

import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label.PackageContext;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.skyframe.SkyFunctions;
Expand All @@ -23,7 +23,6 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Collection;
import java.util.Objects;

/** Stores the {@link NativeAndStarlarkFlags} that are the result of {@link ParsedFlagsFunction}. */
Expand All @@ -41,19 +40,19 @@ public static final class Key implements SkyKey {
*/
@AutoCodec.Instantiator
@VisibleForSerialization
public static Key create(Collection<String> rawFlags, PackageContext packageContext) {
public static Key create(ImmutableList<String> rawFlags, PackageContext packageContext) {
return interner.intern(new Key(rawFlags, packageContext));
}

private final ImmutableSet<String> rawFlags;
private final ImmutableList<String> rawFlags;
private final PackageContext packageContext;

private Key(Collection<String> rawFlags, PackageContext packageContext) {
this.rawFlags = ImmutableSet.copyOf(rawFlags);
private Key(ImmutableList<String> rawFlags, PackageContext packageContext) {
this.rawFlags = rawFlags;
this.packageContext = packageContext;
}

public ImmutableSet<String> rawFlags() {
public ImmutableList<String> rawFlags() {
return rawFlags;
}

Expand Down
22 changes: 11 additions & 11 deletions src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ static Mappings parse(Environment env, List<String> lines, RepoContext mainRepoC
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
// Flags -> platform mapping doesn't support Starlark flags. If the need arises we could upgrade
// this to NativeAndStarlarkFlags like above.
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.of();
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms = ImmutableMap.of();

if (it.peek().equalsIgnoreCase("platforms:")) {
it.next();
Expand Down Expand Up @@ -205,7 +205,7 @@ static Mappings parse(Environment env, List<String> lines, RepoContext mainRepoC
*/
@Nullable
private static NativeAndStarlarkFlags parseStarlarkFlags(
ImmutableSet<String> rawFlags, Environment env, RepoContext mainRepoContext)
ImmutableList<String> rawFlags, Environment env, RepoContext mainRepoContext)
throws PlatformMappingParsingException, InterruptedException {
PackageContext rootPackage = mainRepoContext.rootPackage();
ParsedFlagsValue.Key parsedFlagsKey = ParsedFlagsValue.Key.create(rawFlags, rootPackage);
Expand All @@ -232,7 +232,7 @@ private static ImmutableMap<Label, NativeAndStarlarkFlags> readPlatformsToFlags(
boolean needSkyframeDeps = false;
while (it.hasNext() && !it.peek().equalsIgnoreCase("flags:")) {
Label platform = readPlatform(it, mainRepoContext);
ImmutableSet<String> flags = readFlags(it);
ImmutableList<String> flags = readFlags(it);
NativeAndStarlarkFlags parsedFlags = parseStarlarkFlags(flags, env, mainRepoContext);
if (parsedFlags == null) {
needSkyframeDeps = true;
Expand All @@ -253,12 +253,12 @@ private static ImmutableMap<Label, NativeAndStarlarkFlags> readPlatformsToFlags(
}
}

private static ImmutableMap<ImmutableSet<String>, Label> readFlagsToPlatforms(
private static ImmutableMap<ImmutableList<String>, Label> readFlagsToPlatforms(
PeekingIterator<String> it, RepoContext mainRepoContext)
throws PlatformMappingParsingException {
ImmutableMap.Builder<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.builder();
ImmutableMap.Builder<ImmutableList<String>, Label> flagsToPlatforms = ImmutableMap.builder();
while (it.hasNext() && it.peek().startsWith("--")) {
ImmutableSet<String> flags = readFlags(it);
ImmutableList<String> flags = readFlags(it);
Label platform = readPlatform(it, mainRepoContext);
flagsToPlatforms.put(flags, platform);
}
Expand All @@ -284,14 +284,14 @@ private static Label readPlatform(PeekingIterator<String> it, RepoContext mainRe
}
}

private static ImmutableSet<String> readFlags(PeekingIterator<String> it)
private static ImmutableList<String> readFlags(PeekingIterator<String> it)
throws PlatformMappingParsingException {
ImmutableSet.Builder<String> flags = ImmutableSet.builder();
ImmutableList.Builder<String> flags = ImmutableList.builder();
// Note: Short form flags are not supported.
while (it.hasNext() && it.peek().startsWith("--")) {
flags.add(it.next());
}
ImmutableSet<String> parsedFlags = flags.build();
ImmutableList<String> parsedFlags = flags.build();
if (parsedFlags.isEmpty()) {
throw parsingException(
it.hasNext()
Expand All @@ -315,12 +315,12 @@ private static PlatformMappingParsingException parsingException(String message,
@VisibleForTesting
static final class Mappings {
final ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags;
final ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms;
final ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms;
final RepoContext mainRepoContext;

Mappings(
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags,
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms,
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms,
RepoContext mainRepoContext) {
this.platformsToFlags = platformsToFlags;
this.flagsToPlatforms = flagsToPlatforms;
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public SkyKeyInterner<Key> getSkyKeyInterner() {
}

private final ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags;
private final ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms;
private final ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms;
private final ImmutableSet<Class<? extends FragmentOptions>> optionsClasses;
private final LoadingCache<NativeAndStarlarkFlags, OptionsParsingResult> parserCache;
private final LoadingCache<BuildOptions, BuildOptions> mappingCache;
Expand All @@ -166,7 +166,7 @@ public SkyKeyInterner<Key> getSkyKeyInterner() {
*/
PlatformMappingValue(
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags,
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms,
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms,
ImmutableSet<Class<? extends FragmentOptions>> optionsClasses,
RepositoryMapping mainRepositoryMapping) {
this.platformsToFlags = checkNotNull(platformsToFlags);
Expand Down Expand Up @@ -237,7 +237,7 @@ private BuildOptions computeMapping(BuildOptions originalOptions) throws Options
originalOptions.applyParsingResult(parseWithCache(platformsToFlags.get(targetPlatform)));
} else {
boolean mappingFound = false;
for (Map.Entry<ImmutableSet<String>, Label> flagsToPlatform : flagsToPlatforms.entrySet()) {
for (Map.Entry<ImmutableList<String>, Label> flagsToPlatform : flagsToPlatforms.entrySet()) {
if (originalOptions.matches(
parseWithCache(
NativeAndStarlarkFlags.builder()
Expand Down
33 changes: 16 additions & 17 deletions ...java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
Expand Down Expand Up @@ -72,9 +71,9 @@ public void testParse() throws Exception {
assertThat(mappings.platformsToFlags.get(PLATFORM2).nativeFlags()).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(PLATFORM2);
.containsExactly(ImmutableList.of("--cpu=one"), ImmutableList.of("--cpu=two"));
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=two"))).isEqualTo(PLATFORM2);
}

@Test
Expand Down Expand Up @@ -102,9 +101,9 @@ public void testParseWithRepoMapping() throws Exception {
.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")))
.containsExactly(ImmutableList.of("--cpu=one"), ImmutableList.of("--cpu=two"));
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=two")))
.isEqualTo(EXTERNAL_PLATFORM);
}

Expand Down Expand Up @@ -132,9 +131,9 @@ public void testParseComment() throws Exception {
assertThat(mappings.platformsToFlags.get(PLATFORM2).nativeFlags()).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(PLATFORM2);
.containsExactly(ImmutableList.of("--cpu=one"), ImmutableList.of("--cpu=two"));
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=two"))).isEqualTo(PLATFORM2);
}

@Test
Expand Down Expand Up @@ -162,9 +161,9 @@ public void testParseWhitespace() throws Exception {
assertThat(mappings.platformsToFlags.get(PLATFORM2).nativeFlags()).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(PLATFORM2);
.containsExactly(ImmutableList.of("--cpu=one"), ImmutableList.of("--cpu=two"));
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=two"))).isEqualTo(PLATFORM2);
}

@Test
Expand Down Expand Up @@ -196,9 +195,9 @@ public void testParseMultipleFlagsInFlags() throws Exception {

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

Expand All @@ -225,8 +224,8 @@ public void testParseOnlyFlags() throws Exception {
" //platforms:one" // Force line break
);

assertThat(mappings.flagsToPlatforms.keySet()).containsExactly(ImmutableSet.of("--cpu=one"));
assertThat(mappings.flagsToPlatforms.get(ImmutableSet.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.flagsToPlatforms.keySet()).containsExactly(ImmutableList.of("--cpu=one"));
assertThat(mappings.flagsToPlatforms.get(ImmutableList.of("--cpu=one"))).isEqualTo(PLATFORM1);
assertThat(mappings.platformsToFlags).isEmpty();
}

Expand Down
34 changes: 17 additions & 17 deletions src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public final class PlatformMappingValueTest {
@Test
public void testMapNoMappings() throws OptionsParsingException {
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.of();
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms = ImmutableMap.of();
PlatformMappingValue mappingValue =
new PlatformMappingValue(
platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS, REPO_MAPPING);
Expand All @@ -69,7 +69,7 @@ public void testMapNoMappings() throws OptionsParsingException {

@Test
public void testMapPlatformToFlags() throws Exception {
ImmutableSet<String> nativeFlags = ImmutableSet.of("--cpu=one", "--compilation_mode=dbg");
ImmutableList<String> nativeFlags = ImmutableList.of("--cpu=one", "--compilation_mode=dbg");
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags =
ImmutableMap.of(
PLATFORM1,
Expand All @@ -79,7 +79,7 @@ public void testMapPlatformToFlags() throws Exception {
.repoMapping(REPO_MAPPING)
.build());

ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.of();
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms = ImmutableMap.of();
PlatformMappingValue mappingValue =
new PlatformMappingValue(
platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS, REPO_MAPPING);
Expand All @@ -94,8 +94,8 @@ public void testMapPlatformToFlags() throws Exception {

@Test
public void testMapFlagsToPlatform() throws Exception {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableSet.of("--cpu=one", "--compilation_mode=dbg"), PLATFORM1);
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableList.of("--cpu=one", "--compilation_mode=dbg"), PLATFORM1);

ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
PlatformMappingValue mappingValue =
Expand All @@ -113,10 +113,10 @@ public void testMapFlagsToPlatform() throws Exception {

@Test
public void testMapFlagsToPlatformPriority() throws Exception {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(
ImmutableSet.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1,
ImmutableSet.of("--cpu=foo"), PLATFORM2);
ImmutableList.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1,
ImmutableList.of("--cpu=foo"), PLATFORM2);

ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
PlatformMappingValue mappingValue =
Expand All @@ -133,8 +133,8 @@ public void testMapFlagsToPlatformPriority() throws Exception {

@Test
public void testMapFlagsToPlatformNoneMatching() throws Exception {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableSet.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1);
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableList.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1);

ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
PlatformMappingValue mappingValue =
Expand All @@ -152,8 +152,8 @@ public void testMapFlagsToPlatformNoneMatching() throws Exception {

@Test
public void testMapNoPlatformOptions() throws Exception {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1);
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1);

ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.of();
PlatformMappingValue mappingValue =
Expand All @@ -167,7 +167,7 @@ public void testMapNoPlatformOptions() throws Exception {

@Test
public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception {
ImmutableSet<String> nativeFlags = ImmutableSet.of("--cpu=one", "--compilation_mode=dbg");
ImmutableList<String> nativeFlags = ImmutableList.of("--cpu=one", "--compilation_mode=dbg");
ImmutableMap<Label, NativeAndStarlarkFlags> platformsToFlags =
ImmutableMap.of(
PLATFORM1,
Expand All @@ -176,8 +176,8 @@ public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception {
.optionsClasses(BUILD_CONFIG_PLATFORM_OPTIONS)
.repoMapping(REPO_MAPPING)
.build());
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1);
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1);

BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone();
modifiedOptions.get(CoreOptions.class).cpu = "one";
Expand All @@ -197,8 +197,8 @@ public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception {

@Test
public void testMapNoMappingIfPlatformIsSetAndNoPlatformMapping() throws Exception {
ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1);
ImmutableMap<ImmutableList<String>, Label> flagsToPlatforms =
ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1);

BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone();
modifiedOptions.get(CoreOptions.class).cpu = "one";
Expand Down

0 comments on commit f7dec90

Please sign in to comment.