-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect extension staleness due root module name/version change #22123
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,14 +25,14 @@ | |
import com.google.devtools.build.lib.cmdline.LabelConstants; | ||
import com.google.devtools.build.lib.runtime.BlazeModule; | ||
import com.google.devtools.build.lib.runtime.CommandEnvironment; | ||
import com.google.devtools.build.lib.skyframe.SkyframeExecutor; | ||
import com.google.devtools.build.lib.util.AbruptExitException; | ||
import com.google.devtools.build.lib.vfs.FileSystemUtils; | ||
import com.google.devtools.build.lib.vfs.Path; | ||
import com.google.devtools.build.lib.vfs.Root; | ||
import com.google.devtools.build.lib.vfs.RootedPath; | ||
import com.google.devtools.build.skyframe.MemoizingEvaluator; | ||
import java.io.IOException; | ||
import java.util.Arrays; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
@@ -44,15 +44,15 @@ | |
*/ | ||
public class BazelLockFileModule extends BlazeModule { | ||
|
||
private MemoizingEvaluator evaluator; | ||
private SkyframeExecutor executor; | ||
private Path workspaceRoot; | ||
@Nullable private BazelModuleResolutionEvent moduleResolutionEvent; | ||
|
||
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); | ||
|
||
@Override | ||
public void beforeCommand(CommandEnvironment env) { | ||
evaluator = env.getSkyframeExecutor().getEvaluator(); | ||
executor = env.getSkyframeExecutor(); | ||
workspaceRoot = env.getWorkspace(); | ||
RepositoryOptions options = env.getOptions().getOptions(RepositoryOptions.class); | ||
if (options.lockfileMode.equals(LockfileMode.UPDATE)) { | ||
|
@@ -77,7 +77,8 @@ public void afterCommand() throws AbruptExitException { | |
// first if needed. | ||
Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos = | ||
new ConcurrentHashMap<>(); | ||
evaluator | ||
executor | ||
.getEvaluator() | ||
.getInMemoryGraph() | ||
.parallelForEach( | ||
entry -> { | ||
|
@@ -89,13 +90,23 @@ public void afterCommand() throws AbruptExitException { | |
} | ||
}); | ||
|
||
BazelDepGraphValue depGraphValue; | ||
try { | ||
depGraphValue = | ||
(BazelDepGraphValue) executor.getEvaluator().getExistingValue(BazelDepGraphValue.KEY); | ||
} catch (InterruptedException e) { | ||
// Not thrown in Bazel. | ||
throw new IllegalStateException(e); | ||
} | ||
|
||
BazelLockFileValue oldLockfile = moduleResolutionEvent.getOnDiskLockfileValue(); | ||
// Create an updated version of the lockfile, keeping only the extension results from the old | ||
// lockfile that are still up-to-date and adding the newly resolved extension results. | ||
BazelLockFileValue newLockfile = | ||
moduleResolutionEvent.getResolutionOnlyLockfileValue().toBuilder() | ||
.setModuleExtensions( | ||
combineModuleExtensions(oldLockfile.getModuleExtensions(), newExtensionInfos)) | ||
combineModuleExtensions( | ||
oldLockfile.getModuleExtensions(), newExtensionInfos, depGraphValue)) | ||
.build(); | ||
|
||
// Write the new value to the file, but only if needed. This is not just a performance | ||
|
@@ -119,7 +130,8 @@ public void afterCommand() throws AbruptExitException { | |
ModuleExtensionId, | ||
ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>> | ||
oldExtensionInfos, | ||
Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos) { | ||
Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos, | ||
BazelDepGraphValue depGraphValue) { | ||
Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>> | ||
updatedExtensionMap = new HashMap<>(); | ||
|
||
|
@@ -136,8 +148,8 @@ public void afterCommand() throws AbruptExitException { | |
if (shouldKeepExtension( | ||
moduleExtensionId, | ||
firstEntryFactors, | ||
firstEntryExtension.getUsagesDigest(), | ||
newExtensionInfos.get(moduleExtensionId))) { | ||
newExtensionInfos.get(moduleExtensionId), | ||
depGraphValue)) { | ||
updatedExtensionMap.put(moduleExtensionId, factorToLockedExtension); | ||
} | ||
} | ||
|
@@ -174,47 +186,46 @@ public void afterCommand() throws AbruptExitException { | |
} | ||
|
||
/** | ||
* Decide whether to keep this extension or not depending on all of: | ||
* Keep an extension if and only if: | ||
* | ||
* <ol> | ||
* <li>If its dependency on os & arch didn't change | ||
* <li>If its usages haven't changed | ||
* <li>it still has usages | ||
* <li>its dependency on os & arch didn't change | ||
* <li>it hasn't become reproducible | ||
* </ol> | ||
* | ||
* We do not check for changes in the usage hash of the extension or e.g. hashes of files accessed | ||
* during the evaluation. These values are checked in SingleExtensionEvalFunction. | ||
* | ||
* @param lockedExtensionKey object holding the old extension id and state of os and arch | ||
* @param oldUsagesDigest the digest of usages of this extension in the existing lockfile | ||
* @param newExtensionInfo the in-memory lockfile entry produced by the most recent up-to-date | ||
* evaluation of this extension (if any) | ||
* @param depGraphValue the dep graph value | ||
* @return True if this extension should still be in lockfile, false otherwise | ||
*/ | ||
private boolean shouldKeepExtension( | ||
ModuleExtensionId extensionId, | ||
ModuleExtensionId moduleExtensionId, | ||
ModuleExtensionEvalFactors lockedExtensionKey, | ||
byte[] oldUsagesDigest, | ||
@Nullable LockFileModuleExtension.WithFactors newExtensionInfo) { | ||
|
||
// If there is a new event for this extension, compare it with the existing ones | ||
if (newExtensionInfo != null) { | ||
boolean doNotLockExtension = !newExtensionInfo.moduleExtension().shouldLockExtension(); | ||
boolean dependencyOnOsChanged = | ||
lockedExtensionKey.getOs().isEmpty() | ||
!= newExtensionInfo.extensionFactors().getOs().isEmpty(); | ||
boolean dependencyOnArchChanged = | ||
lockedExtensionKey.getArch().isEmpty() | ||
!= newExtensionInfo.extensionFactors().getArch().isEmpty(); | ||
if (doNotLockExtension || dependencyOnOsChanged || dependencyOnArchChanged) { | ||
return false; | ||
} | ||
@Nullable LockFileModuleExtension.WithFactors newExtensionInfo, | ||
BazelDepGraphValue depGraphValue) { | ||
if (!depGraphValue.getExtensionUsagesTable().containsRow(moduleExtensionId)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. couldn't you just read this out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's going to happen in the follow-up PR anyway (which will get even more values from Skyframe). |
||
return false; | ||
} | ||
|
||
// If there is no new extension info, the properties of the existing extension entry can't have | ||
// changed. | ||
if (newExtensionInfo == null) { | ||
return true; | ||
} | ||
|
||
// Otherwise, compare the current usages of this extension with the ones in the lockfile. We | ||
// trim the usages to only the information that influences the evaluation of the extension so | ||
// that irrelevant changes (e.g. locations or imports) don't cause the extension to be removed. | ||
// Note: Extension results can still be stale for other reasons, e.g. because their transitive | ||
// bzl hash changed, but such changes will be detected in SingleExtensionEvalFunction. | ||
return Arrays.equals( | ||
ModuleExtensionUsage.hashForEvaluation( | ||
GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(), | ||
moduleResolutionEvent.getExtensionUsagesById().row(extensionId)), | ||
oldUsagesDigest); | ||
boolean doNotLockExtension = !newExtensionInfo.moduleExtension().shouldLockExtension(); | ||
boolean dependencyOnOsChanged = | ||
lockedExtensionKey.getOs().isEmpty() | ||
!= newExtensionInfo.extensionFactors().getOs().isEmpty(); | ||
boolean dependencyOnArchChanged = | ||
lockedExtensionKey.getArch().isEmpty() | ||
!= newExtensionInfo.extensionFactors().getArch().isEmpty(); | ||
return !doNotLockExtension && !dependencyOnOsChanged && !dependencyOnArchChanged; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean? Is it thrown in Blaze?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not thrown in Bazel because the in-memory graph implementation doesn't declare this checked exception. It's presumably thrown by Blaze's graph impl, but then the code in this PR doesn't touch any Blaze code.
Maybe I should just rethrow this unchecked? Handling it with an exit code when it can't happen also doesn't feel right. If it does indeed happen some day, we should learn about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could work around this by calling
evaluator.getInMemoryGraph().getIfPresent(...)
.But I think this value is now unnecessary -- you could just remove it since it's no longer used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed to learn of extensions that are no longer used anywhere. This was found by tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the graph maintained by the
MemoizingEvaluator
instance I was holding onto became empty due to Skyfocus. Instead, I'm now holding on to theSkyframeExecutor
, which seems to work. Looks like something in Skyfocus is switching out the evaluator after the build - I don't understand where or why this happening, but I won't complain if the tests pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, found the cause now:
bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
Line 4077 in 3beaaaf