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

Vendor registry files #22566

Closed
wants to merge 14 commits into from

Conversation

meteorcloudy
Copy link
Member

@meteorcloudy meteorcloudy commented May 28, 2024

  • Vendor registry files needed for Bazel module resolution to achieve
    offline build with vendor mode.
  • Also refactored bazel_vendor_test to avoid vendoring dependencies of
    bazel_tools, which speeds up the test significantly.

Fixes #22554

@meteorcloudy meteorcloudy marked this pull request as draft May 28, 2024 13:48
@meteorcloudy meteorcloudy removed the request for review from Wyverald May 28, 2024 13:48
@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 May 28, 2024
@meteorcloudy meteorcloudy force-pushed the vendor_registry branch 3 times, most recently from 0747210 to 9448d68 Compare June 3, 2024 15:16
@meteorcloudy meteorcloudy marked this pull request as ready for review June 3, 2024 15:21
@meteorcloudy meteorcloudy requested a review from lberki as a code owner June 3, 2024 15:21
@meteorcloudy meteorcloudy requested review from Wyverald and fmeum and removed request for lberki June 3, 2024 15:21
vendorDirectory.createDirectoryAndParents();
}

for (RepositoryName repo : reposToVendor) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could probably be parallelized across all repos.

Do we need to worry about atomicity, e.g. if users interrupt the vendor command? Maybe we should atomically move the marker files into their final location at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I left a TODO for parallelizing.

But I think copying one marker file after vendoring one repo source is fine? If the vendoring is interrupted, the user probably doesn't want to re-vendor what's already been vendored?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the copying of the marker file was atomic then I would agree, but what if the marker file is only written partially and then happens to become identical to an up-to-date one even if the contents aren't?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see! Updated the code to copy the marker file to a tmp file first and then rename it.

Copy link
Member

Choose a reason for hiding this comment

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

there's technically still potential for races unless we actually have a proper locking scheme (eg. multiple Bazels can be working in the same workspace). but we can leave that for future PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

then external/A/../B will point to ~/workspace/vendor/B

This only happens when the symlink is resolved somehow, right? Do people actually use this pattern to locate another repo? I would consider this is a hack?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have used something like this in the past to point --repo_env=JAVA_HOME to the embedded JDK for r_j_e's coursier and I could see some repo rules doing this, but I do agree it's a hack :-) I just wanted to point out that we should have such breakages on our radar when we make this optimization (I think we still should).

Copy link
Member Author

@meteorcloudy meteorcloudy Jun 6, 2024

Choose a reason for hiding this comment

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

Yeah moving + leaving a symlink sounds good to me.

I ended up not doing this. It could cause problem in this situation:

  1. vendor a repo foo: now external/foo -> vendor_dir/foo
  2. Remove --vendor_dir
  3. Think vendor mode is off, and I can change vendor_dir/foo freely (e.g. delete it)
  4. Build breaks because external/foo still points to the modified source

Copy link
Member

Choose a reason for hiding this comment

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

did you actually run into this scenario? I'd expect step 2 to cause RepositoryDelegatorFunction to re-run and realize that --vendor_dir is now off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, then it will check the marker file, consider it's up-to-date and won't do anything. I guess we can remove the marker file under external/ to make sure the repo is re-refetched when vendor mode is off. I can explore in a next PR.

vendorDirectory.createDirectoryAndParents();
}

for (RepositoryName repo : reposToVendor) {
Copy link
Member

Choose a reason for hiding this comment

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

there's technically still potential for races unless we actually have a proper locking scheme (eg. multiple Bazels can be working in the same workspace). but we can leave that for future PRs.

vendorDirectory.createDirectoryAndParents();
}

for (RepositoryName repo : reposToVendor) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah moving + leaving a symlink sounds good to me. In fact I think that was my expectation in the beginning, before we settled on copying (can't remember why now).

- Vendor registry files needed for Bazel module resolution to achieve
offline build with vendor mode.
- Also refactored bazel_vendor_test to avoid vendoring dependencies of
  bazel_tools, which speeds up the test significantly.

Fixes bazelbuild#22554
@copybara-service copybara-service bot closed this in 96b90ba Jun 7, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 7, 2024
@malt3
Copy link
Contributor

malt3 commented Jun 7, 2024

Hello,
If I understand this change correctly, it would also be super useful for building Bazel itself in an air gapped environment (like the nix sandbox).
Is there any chance that this PR can be backported to the 7.x stream?
It would greatly simplify the packaging of Bazel (and potentially other projects using Bazel) in nixpkgs.

@meteorcloudy
Copy link
Member Author

@malt3 Yes, we plan to back port the full vendor feature into 7.3, as 7.2 will probably be released today.

meteorcloudy added a commit to meteorcloudy/bazel that referenced this pull request Jun 18, 2024
- Vendor registry files needed for Bazel module resolution to achieve
offline build with vendor mode.
- Also refactored bazel_vendor_test to avoid vendoring dependencies of
  bazel_tools, which speeds up the test significantly.

Fixes bazelbuild#22554

Closes bazelbuild#22566.

PiperOrigin-RevId: 641246903
Change-Id: I01a78612ad7ca454df2c70dc5dcc38ec2fbfb71c
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
Development

Successfully merging this pull request may close these issues.

Make sure offline build can be achieved with vendor mode and the new lockfile format
4 participants