diff options
author | Greg Estren <gregce@google.com> | 2017-01-17 23:47:24 +0000 |
---|---|---|
committer | Vladimir Moskva <vladmos@google.com> | 2017-01-18 11:01:49 +0000 |
commit | 037bbcf67e707098bf8733108847b5d317be3af2 (patch) | |
tree | 484522c21ae525542927c773f78d3d2deba4aa95 /src/main/java/com/google/devtools/build/lib | |
parent | 9e723af08c94520fff7bcbb26d5b3a2d562bcd88 (diff) |
Change TargetContext.findDirectPrerequisite from config1 == config2 to config1.equals(c2).
This fixes an obscure bug between dynamic configurations, host configuration caching, and Skyframe skyKey interning that makes Bazel crash under certain sequences of builds. See changes for details.
--
PiperOrigin-RevId: 144766296
MOS_MIGRATED_REVID=144766296
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java | 4 | ||||
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java | 10 |
2 files changed, 13 insertions, 1 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java index fc263efbab..f943e6ec76 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis; +import com.google.common.base.Objects; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -104,7 +105,8 @@ public class TargetContext { */ TransitiveInfoCollection maybeFindDirectPrerequisite(Label label, BuildConfiguration config) { for (ConfiguredTarget prerequisite : directPrerequisites) { - if (prerequisite.getLabel().equals(label) && (prerequisite.getConfiguration() == config)) { + if (prerequisite.getLabel().equals(label) + && (Objects.equal(prerequisite.getConfiguration(), config))) { return prerequisite; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index a905a9f68a..c84cf2726a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -528,6 +528,16 @@ public final class SkyframeBuildView { if (hostConfig != null) { return hostConfig; } + // TODO(bazel-team): investigate getting the trimmed config from Skyframe instead of cloning. + // This is the only place we instantiate BuildConfigurations outside of Skyframe, This can + // produce surprising effects, such as requesting a configuration that's in the Skyframe cache + // but still produces a unique instance because we don't check that cache. It'd be nice to + // guarantee that *all* instantiations happen through Skyframe. That could, for example, + // guarantee that config1.equals(config2) implies config1 == config2, which is nice for + // verifying we don't accidentally create extra configurations. But unfortunately, + // hostConfigurationCache was specifically created because Skyframe is too slow for this use + // case. So further optimization is necessary to make that viable (proto_library in particular + // contributes to much of the difference). BuildConfiguration trimmedConfig = topLevelHostConfiguration.clone(fragmentClasses, ruleClassProvider); hostConfigurationCache.put(fragmentClasses, trimmedConfig); |