Fix edge cases in lockfile handling #24754
Conversation
* Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe. * Honor `reproducible` per extension eval factor, not per extension.
github-actions
bot
added
team-ExternalDeps
|
@bazel-io fork 8.0.1 |
|
@bazel-io fork 7.5.0 |
fmeum
changed the title
BazelLockFileModule
fmeum
force-pushed
the
24723-preparations
branch
from
0bd9c35 to
97bcb7a
Compare
fmeum
force-pushed
the
24723-preparations
branch
from
97bcb7a to
9c03c55
Compare
|
@meteorcloudy Tests are passing now. |
| @@ -80,7 +80,7 @@ public static ModuleExtensionId create( | |||
| } | |||
|
|
|||
| public boolean isInnate() { | |||
| return extensionName().contains("%"); | |||
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
This is for the third point in the description: module extension IDs with an isolation key and those for repo rules would conflict after serialization into a string (for the lockfile), which results in a crash when parsing them back.
There was a problem hiding this comment.
this seems dangerous to me. Changing the separator to + here would mean that (IIUC) we can't have it in the .bzl file label, so essentially using any repo rule from a non-main repo would fail (because that would result in an extension name like @@foo++foo_ext+foo_repo//:bar.bzl+my_repo_rule). In fact I'm surprised all tests passed -- maybe we never use_repo_rule from non-main repos in our tests?
(and yes, I understand that this also means % was never allowed to show up in the .bzl file label, but this was also true of aspects etc.)
There was a problem hiding this comment.
Hmm, why exactly would this fail? The synthetic extension name as passed in by module file evaluation is "prettified" to just _repo_rules for the purpose of generating the extension unique prefix: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java;l=186?q=_repo_rules&ss=bazel%2Fbazel&start=41.
There was a problem hiding this comment.
Yes, the repos generated by an innate extension have the form of module+_repo_rulesN+repo_name. But the name of the extension corresponding to the repo name prefix module+_repo_rulesN is currently <bzl_file_label>%<rule_name> (code). This PR is changing that line to split on +, which should result in errors.
There was a problem hiding this comment.
I'm now using a space instead of a plus and am also only splitting on the last occurrence of the separator.
I don't think this was an issue in practice though as the bzl file label is the one specified in a module file and thus uses apparent rather than canonical repo names (I don't think we ever intentionally supported the latter and even prohibited them at some point).
There was a problem hiding this comment.
ah, I see! That's indeed why everything was actually okay.
Yeah, as long as the separator is not a valid Starlark identifier character and we split by the last occurrence only, we should be all good in all cases. This LGTM now, thanks!
meteorcloudy
added
awaiting-PR-merge
fmeum
force-pushed
the
24723-preparations
branch
from
3e57eb9 to
3c023e8
Compare
src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java
Show resolved
Hide resolved
github-actions
bot
removed
the
awaiting-PR-merge
* Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe. * Honor `reproducible` per extension eval factor, not per extension. * Fix encoding conflict between isolation key and `use_repo_rule`'s fake extension names This doesn't require a lockfile version bump as `use_repo_rule`'s fake extension (so far) isn't included in the lockfile. Work towards bazelbuild#24723 Closes bazelbuild#24754. PiperOrigin-RevId: 712623562 Change-Id: I61fd439539031a01ddec4488276ff2d0484849f2 (cherry picked from commit cfda178)
Currently `ModuleExtensionId#toString()` uses the default Java record repr, which isn't very user-friendly. There's really no reason not to use the `asTargetString()` method instead. This PR just renames `asTargetString()` to `toString()` and updates all call sites. Closes bazelbuild#24450. PiperOrigin-RevId: 700050362 Change-Id: I82238d2134e1642694f0b20235fcfe9307ceaa7d (cherry picked from commit d23421a) Fix edge cases in lockfile handling * Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe. * Honor `reproducible` per extension eval factor, not per extension. * Fix encoding conflict between isolation key and `use_repo_rule`'s fake extension names This doesn't require a lockfile version bump as `use_repo_rule`'s fake extension (so far) isn't included in the lockfile. Work towards bazelbuild#24723 Closes bazelbuild#24754. PiperOrigin-RevId: 712623562 Change-Id: I61fd439539031a01ddec4488276ff2d0484849f2 (cherry picked from commit cfda178)
… lockfile handling (#24845) ### Streamline `ModuleExtensionId#toString()` Currently `ModuleExtensionId#toString()` uses the default Java record repr, which isn't very user-friendly. There's really no reason not to use the `asTargetString()` method instead. This PR just renames `asTargetString()` to `toString()` and updates all call sites. Closes #24450. PiperOrigin-RevId: 700050362 Change-Id: I82238d2134e1642694f0b20235fcfe9307ceaa7d (cherry picked from commit d23421a) ### Fix edge cases in lockfile handling * Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe. * Honor `reproducible` per extension eval factor, not per extension. * Fix encoding conflict between isolation key and `use_repo_rule`'s fake extension names This doesn't require a lockfile version bump as `use_repo_rule`'s fake extension (so far) isn't included in the lockfile. Work towards #24723 Closes #24754. PiperOrigin-RevId: 712623562 Change-Id: I61fd439539031a01ddec4488276ff2d0484849f2 (cherry picked from commit cfda178) Fixes #24754 Co-authored-by: Xdng Yng <[email protected]>
--lockfile_modeisofforerrorbut the command doesn't forward options to Skyframe.reproducibleper extension eval factor, not per extension.use_repo_rule's fake extension namesThis doesn't require a lockfile version bump as
use_repo_rule's fake extension (so far) isn't included in the lockfile.Work towards #24723