Skip to content
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

Fix edge cases in lockfile handling #24754

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 19, 2024

  • 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

* 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 github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Dec 19, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2024

@bazel-io fork 8.0.1

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2024

@bazel-io fork 7.5.0

@fmeum fmeum changed the title Fix edge cases in BazelLockFileModule Fix edge cases in lockfile handling Dec 19, 2024
@fmeum fmeum force-pushed the 24723-preparations branch from 0bd9c35 to 97bcb7a Compare December 19, 2024 12:18
@fmeum fmeum force-pushed the 24723-preparations branch from 97bcb7a to 9c03c55 Compare December 19, 2024 13:53
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2024

@meteorcloudy Tests are passing now.

@@ -80,7 +80,7 @@ public static ModuleExtensionId create(
}

public boolean isInnate() {
return extensionName().contains("%");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants