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

Migrate to an incremental format for MODULE.bazel.lock #22154

Closed
wants to merge 22 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 26, 2024

The old lockfile fields and all code related to it as well as the workarounds for local path inclusions are removed.

The distribution archive lockfile needs to be checked in temporarily as the main MODULE.bazel.lock file does not contain the hashes for the registry files.

Fixes #19621
Fixes #20369

RELNOTES: The format for MODULE.bazel.lock is now less likely to result in merge conflicts and is updated incrementally, with only new files downloaded from registries and existing ones taken from the repository cache (if configured).

@fmeum fmeum marked this pull request as draft April 26, 2024 16:50
@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 Apr 26, 2024
@fmeum fmeum force-pushed the 20369-migrate branch 12 times, most recently from 1db45d6 to afb8daf Compare May 3, 2024 15:33
@fmeum fmeum marked this pull request as ready for review May 3, 2024 16:43
@fmeum fmeum requested a review from a team as a code owner May 3, 2024 16:43
@fmeum fmeum requested review from aranguyen and removed request for a team May 3, 2024 16:43
@github-actions github-actions bot added the team-Configurability Issues for Configurability team label May 3, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented May 3, 2024

@Wyverald This is stacked on #21901, but otherwise ready for a first review.

@fmeum fmeum removed the request for review from aranguyen May 3, 2024 16:43
@fmeum fmeum added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Configurability Issues for Configurability team labels May 3, 2024
@fmeum fmeum force-pushed the 20369-migrate branch 2 times, most recently from 14ecf1d to b720a41 Compare May 7, 2024 08:39
@fmeum
Copy link
Collaborator Author

fmeum commented May 7, 2024

I rebased and fixed the remaining tests. This got a bit larger as I reverted some commits instead of adapting their code and removing it in a follow-up PR, but I kept all their tests around that still apply.

As a follow-up, I could relatively easily add a new lockfile mode if that's our preferred way of allowing users to refetch mutable registry content.

@SalmaSamy I'm not entirely sure how this interacts with bazel vendor and airgapped builds. Do you have an integration test that models the expected behavior? Then I could make sure that the new format doesn't break it.

@fmeum fmeum force-pushed the 20369-migrate branch 3 times, most recently from 8ad8702 to b210862 Compare May 10, 2024 13:40
@fmeum fmeum requested a review from Wyverald May 10, 2024 13:41
@fmeum
Copy link
Collaborator Author

fmeum commented May 10, 2024

Forgot to update the test lockfile, hope this is green now.

@fmeum
Copy link
Collaborator Author

fmeum commented May 10, 2024

@Wyverald The --batch tests in starlark_repository_test appear to time out pretty reliably. Do you know whether --batch messes with repo cache or lockfile usage in any way? I'm not sure why there would be more BCR connections now, although perhaps they are just timed differently and that tickles the RBE setup in the wrong way.

@fmeum
Copy link
Collaborator Author

fmeum commented May 10, 2024

I noticed that this patch causes the verify_default_lockfile test to fail, which I wouldn't expect if the test repo cache is populated correctly (with the new lockfile format):

diff --git a/src/test/tools/bzlmod/BUILD b/src/test/tools/bzlmod/BUILD
index ccaa79a657..a0bbf6bc15 100644
--- a/src/test/tools/bzlmod/BUILD
+++ b/src/test/tools/bzlmod/BUILD
@@ -23,6 +23,5 @@ sh_test(
         "//src:bazel",
         "//src/test/shell/bazel:test-deps",
     ],
-    tags = ["requires-network"],
     deps = ["@bazel_tools//tools/bash/runfiles"],
 )

Do you know whether I need to do anything more to ensure all tests have access to a repo cache containing the BCR files? It's surprising that no existing test is failing.

Edit: Nvm, the repo cache is only used in CI, not locally.

Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

nice. I like this new approach and the comments make it much easier to understand too. Let's ship it!!

@Wyverald Wyverald 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 May 10, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 13, 2024
@Wyverald
Copy link
Member

@bazel-io fork 7.2.0

Wyverald pushed a commit that referenced this pull request May 13, 2024
The old lockfile fields and all code related to it as well as the workarounds for local path inclusions are removed.

The distribution archive lockfile needs to be checked in temporarily as the main `MODULE.bazel.lock` file does not contain the hashes for the registry files.

Fixes #19621
Fixes #20369

RELNOTES: The format for MODULE.bazel.lock is now less likely to result in merge conflicts and is updated incrementally, with only new files downloaded from registries and existing ones taken from the repository cache (if configured).

Closes #22154.

PiperOrigin-RevId: 633233519
Change-Id: Ie2c3042e4141a36e472b2c25cbd67be4aad096a1
@fmeum fmeum deleted the 20369-migrate branch May 13, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
2 participants