Detect extension staleness due root module name/version change #22123
Conversation
github-actions
bot
added
team-ExternalDeps
fmeum
force-pushed
the
22121-module-name
branch
from
c72af29
to
4326d90
Compare
|
@bazel-io fork 7.2.0 |
|
Alternative idea: at https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java;drc=6f48f1c2b3bb73768b9ff15ce6698e21eddc503a;l=224 , hash the entire |
|
The whole value would be too much as it contains repo mappings, but hashing in the abridged modules is a great idea. |
fmeum
force-pushed
the
22121-module-name
branch
2 times, most recently
from
9eb6c5c
to
7383021
Compare
|
I implemented the new approach, PTAL. |
fmeum
force-pushed
the
22121-module-name
branch
from
7383021
to
8defb06
Compare
|
@jin The only failing test is for Skyfocus. It looks like it can result in certain Skyframe nodes missing after the build that are contained in the transitive closure of |
| try { | ||
| depGraphValue = (BazelDepGraphValue) evaluator.getExistingValue(BazelDepGraphValue.KEY); | ||
| } catch (InterruptedException e) { | ||
| // Not thrown in Bazel. |
There was a problem hiding this comment.
what does this mean? Is it thrown in Blaze?
There was a problem hiding this comment.
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.
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.
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.
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 the SkyframeExecutor, 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.
Nvm, found the cause now:
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java
Outdated
Show resolved
Hide resolved
fmeum
force-pushed
the
22121-module-name
branch
from
784a011
to
f94c485
Compare
| } | ||
| @Nullable LockFileModuleExtension.WithFactors newExtensionInfo, | ||
| BazelDepGraphValue depGraphValue) { | ||
| if (!depGraphValue.getExtensionUsagesTable().containsRow(moduleExtensionId)) { |
There was a problem hiding this comment.
couldn't you just read this out of moduleResolutionEvent as before (old line 216)? unless we're looking to stop using that event and switch to using depGraphValue anyway
There was a problem hiding this comment.
Yes, that's going to happen in the follow-up PR anyway (which will get even more values from Skyframe).
Wyverald
added
awaiting-PR-merge
github-actions
bot
removed
the
awaiting-PR-merge
Fixes bazelbuild#22121 Closes bazelbuild#22123. PiperOrigin-RevId: 630277746 Change-Id: I893a69b0048e933554f515684c6ab1fdae541c0b
Fixes #22121