Bazel: Add lfs_archives function - #22483
Conversation
This pulls out some generic stuff from the Swift static runtime into a generic location. Internal clean-up PR will follow once this is in.
| if remote: | ||
| infos = probe(remote) | ||
| for src, info in zip(remote, infos): | ||
| sha256, _, url = info.partition(" ") | ||
| repository_ctx.report_progress("downloading remote %s" % src.basename) | ||
| repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable) | ||
| if extract: | ||
| for src in srcs: | ||
| repository_ctx.report_progress("extracting %s" % src.basename) | ||
| repository_ctx.extract(src.basename, stripPrefix = stripPrefix) | ||
| repository_ctx.delete(src.basename) | ||
| if remote: | ||
| infos = probe(remote) | ||
| for src, info in zip(remote, infos): | ||
| sha256, _, url = info.partition(" ") | ||
| repository_ctx.report_progress("downloading remote %s" % src.basename) | ||
| repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable) | ||
| if extract: | ||
| for src in srcs: | ||
| repository_ctx.report_progress("extracting %s" % src.basename) | ||
| repository_ctx.extract(src.basename, stripPrefix = stripPrefix) | ||
| repository_ctx.delete(src.basename) |
There was a problem hiding this comment.
Copilot changed the indentation here. The old indentation indeed looks somewhat fishy.
There was a problem hiding this comment.
oh, indeed, seems like a latent bug being fixed 👍
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Duplicate archive basenames can collide in the shared staging location and cause incorrect extraction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
misc/bazel/lfs.bzl — Different source labels can have the same basename, but lfs_smudge stages every input at… |
What changed in this PR
Adds reusable support for overlaying multiple Git LFS archives in one Bazel repository.
Changes:
- Fixes multi-source download/extraction sequencing.
- Adds the public
lfs_archivesrepository rule.
| File | Description |
|---|---|
misc/bazel/lfs.bzl |
Adds ordered multi-archive LFS extraction support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _download_and_extract_lfs_archives(repository_ctx): | ||
| lfs_smudge( | ||
| repository_ctx, | ||
| [repository_ctx.path(src) for src in repository_ctx.attr.srcs], |
redsun82
left a comment
redsun82
left a comment
There was a problem hiding this comment.
thanks a lot, only a minor nit on my side
| if remote: | ||
| infos = probe(remote) | ||
| for src, info in zip(remote, infos): | ||
| sha256, _, url = info.partition(" ") | ||
| repository_ctx.report_progress("downloading remote %s" % src.basename) | ||
| repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable) | ||
| if extract: | ||
| for src in srcs: | ||
| repository_ctx.report_progress("extracting %s" % src.basename) | ||
| repository_ctx.extract(src.basename, stripPrefix = stripPrefix) | ||
| repository_ctx.delete(src.basename) | ||
| if remote: | ||
| infos = probe(remote) | ||
| for src, info in zip(remote, infos): | ||
| sha256, _, url = info.partition(" ") | ||
| repository_ctx.report_progress("downloading remote %s" % src.basename) | ||
| repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable) | ||
| if extract: | ||
| for src in srcs: | ||
| repository_ctx.report_progress("extracting %s" % src.basename) | ||
| repository_ctx.extract(src.basename, stripPrefix = stripPrefix) | ||
| repository_ctx.delete(src.basename) |
There was a problem hiding this comment.
oh, indeed, seems like a latent bug being fixed 👍
| "build_file_content": attr.string(doc = "The content for the BUILD file for this repository. " + | ||
| "Either build_file or build_file_content can be specified, but not both."), | ||
| "srcs": attr.label_list(doc = "Local paths to the LFS archives to extract in order.", mandatory = True), | ||
| "strip_prefix": attr.string(default = "", doc = "A directory prefix to strip from the extracted files."), |
There was a problem hiding this comment.
we might want to have a per src strip prefix, but until we need that we can just keep this
There was a problem hiding this comment.
Then I'll keep this for now.
| doc = "Overlay the contents from on-demand LFS archives. The corresponding paths should be added to be ignored " + | ||
| "in `.lfsconfig`.", | ||
| implementation = _download_and_extract_lfs_archives, | ||
| attrs = { |
There was a problem hiding this comment.
we should extract the common attributes to a _lfs_archive_attrs dictionary to avoid repeating, only leaving src/srcs out
attrs = {
"src": ...,
**_lfs_archive_attrs,
},or
attrs = {
"src": ...,
} | _lfs_archive_attrs,(don't remember whether bazel supports the former, but it definitely supports the latter)
This pulls out some generic stuff from the Swift static runtime into a generic location.
Internal clean-up PR will follow once this is in.