Repository navigation
Conversation
The `config` extension read `artifacts_manifest.json` by Label, so with `--experimental_remote_repo_contents_cache` every consumer still materialized the whole `@rules_python` repo, the gap left after bazel-contrib#4294. The manifest is now `artifacts_manifest.bzl` with an `ARTIFACTS_MANIFEST_JSON` string that the extension loads. The release generator writes that .bzl, and the archive script bundles it in place of the empty one in git.
605daa3 to
b5b9262
Compare
|
AI disclosure: this change was written with an AI coding agent and human-reviewed by me before sending. I'll respond to review comments directly; no agent is monitoring or replying on this PR. |
|
Verified with Bazel 9.1.1, Before ( After (this PR): no |
|
PTAL |
| http_file = mocks.repo() | ||
| toolchain_repo = mocks.repo() | ||
| tools_toolchains_repo = mocks.repo() | ||
| mctx = mocks.mctx( |
There was a problem hiding this comment.
We should add a validation or a comment here to ensure that we don't do something similar in the future. Could we in theory add an integration test to test a regression like this?
There was a problem hiding this comment.
With the mocks, it might not be so bad to do that. All the the mctx.read() calls should go through the mock interface, so if that kept track of the paths it saw, you could then compute if the read would have caused a repo to be materialized.
For an integration test, not sure, since it requires the remote repo cache. It sounded like @sputt had a cleaner repro strategy, so maybe they have ideas / know
|
|
||
| It's a .bzl file so the `config` extension can load it rather than read it by | ||
| Label, which would make Bazel's remote repository contents cache materialize | ||
| the entire repository. |
There was a problem hiding this comment.
add a reference to the pending PR in bazel to optimize this case. Given that, we can track down when we can remove this hack
| the entire repository. | ||
| """ | ||
|
|
||
| ARTIFACTS_MANIFEST_JSON = "" |
There was a problem hiding this comment.
An embedded JSON string is a bit weird, so add a comment about why we do that: to keep the code paths for loading a custom prebuilt tools manifest the same as when using the builtin manifest.
| manifest_path = self.rules_python_dir / "python/private/artifacts_manifest.bzl" | ||
| manifest_path.write_text( | ||
| json.dumps(manifest, indent=2) + "\n", | ||
| f'ARTIFACTS_MANIFEST_JSON = """{json.dumps(manifest)}"""\n', |
There was a problem hiding this comment.
pretty print the JSON string. Use indent 1. The parsing and storage overhead are minimal, so lets take the win in human inspection of it.
| . \ | ||
| ":!MODULE.bazel" \ | ||
| ":!python/private/artifacts_manifest.json" | gzip > "$ARCHIVE" | ||
| ":!python/private/artifacts_manifest.bzl" | gzip > "$ARCHIVE" |
There was a problem hiding this comment.
nit: since we're here, move the | gzip > "$ARCHIVE" to its own line
| @@ -0,0 +1,4 @@ | |||
| (toolchains) Evaluating the {obj}`config` module extension no longer | |||
There was a problem hiding this comment.
I'm pretty sure the artifacts.json code path didn't land yet, so this news entry isn't necessary
The
configextension readsartifacts_manifest.jsonwithmctx.read(Label(...)), so with--experimental_remote_repo_contents_cacheevery consumer still materializes the whole@rules_pythonrepo. This is the remaining gap after #4294.Following #4294, the manifest becomes
python/private/artifacts_manifest.bzl, whoseARTIFACTS_MANIFEST_JSONstring the extension loads. It stays empty in git, so source checkouts build the tools from source. The release generator now writes this.bzlfile, and the archive script bundles it in place of the empty one.configure_tool_toolchainsno longer needsmodule_ctx, and tests pass their manifest JSON directly.