Repository navigation
Conversation
It seems that the automatic bzlmod lockfile driver is doing only shallow merging, let's use a different schema that is much more friendly towards such workflows. Before this PR users would have to manually resolve git conflicts or regenerate the lockfile from scratch, with this it will work better as the bazel authors intended. Fixes bazel-contrib#4162 Related bazelbuild/bazel#31139
|
Looking at this, I think maybe it would be good to have the key of the fact as Lastly there was someone asking (#4278) if we could reduce the lock file even more, and I guess we could, but that would require signalling to the cache about what to retain in the |
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
partial review, but overall lgtm. agent will post some comments automatically shortly
| if not filename: | ||
| _, _, filename = url.rpartition("/") | ||
| for url, entry in known_facts.get(fact_key, {}).items(): | ||
| if type(entry) != "string": |
There was a problem hiding this comment.
why the type check? compatibility with an older lock file? That's fine, just put an explanatory comment
| distribution = distribution.rstrip("/") | ||
| root_url = root_url.rstrip("/") | ||
|
|
||
| # The schema is |
There was a problem hiding this comment.
These schema comments seem important for understanding. Can we keep them somewhere?
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
Nit on the PR description (since it becomes the merge commit message): could you move the AI note ("Implementation done by an opencode clanker...") to a PR comment per CONTRIBUTING.md, and add a sentence summarizing how v3 flattens the schema (per-package top-level keys + compact pipe-delimited strings)?
This is an agent-generated comment reviewed and approved by the user.
| raw_index_urls = {} | ||
| for pkg in requested_versions.keys(): | ||
| fact_key = pkg + "#" + index_url | ||
| if fact_key in known_facts: | ||
| raw_index_urls[pkg] = known_facts[fact_key] | ||
|
|
||
| result = _filter_packages( | ||
| dists = known_facts.get("index_urls", {}).get(index_url, {}), | ||
| dists = raw_index_urls, | ||
| requested_versions = requested_versions, | ||
| ) |
There was a problem hiding this comment.
Nit: since raw_index_urls already iterates over requested_versions.keys(), _filter_packages is a no-op copy here—returning None inside the loop when fact_key not in known_facts avoids the extra pass and len(result) != len(requested_versions) check.
This is an agent-generated comment reviewed and approved by the user.
| distribution = distribution.rstrip("/") | ||
| root_url = root_url.rstrip("/") | ||
|
|
||
| fact_key = root_url + " " + distribution |
There was a problem hiding this comment.
+1 to your comment about using <distribution>@<root_url> (distribution + "@" + root_url) instead of root_url + " " + distribution—it matches the package-first ordering of pkg + "#" + index_url (while keeping @ distinct from # so index-page and package-page keys can never collide), groups lockfile entries by package name, and avoids another fact_version bump later. (On stripping index_url prefixes from file URLs: since pypi.org/simple serves wheels from files.pythonhosted.org, prefix stripping wouldn't shrink standard PyPI entries anyway.) Worth switching to distribution + "@" + root_url before merging v3?
This is an agent-generated comment reviewed and approved by the user.
| if digest: | ||
| known_sources.setdefault("hashes_by_version", {}).setdefault(version, []).append(digest) |
There was a problem hiding this comment.
Skipping empty digest values here makes _get_from_facts diverge from parse_simpleapi_html (parse_simpleapi_html.bzl:124) and _filter_packages (pypi_cache.bzl:156,163), which both record [""] in hashes_by_version[version]. parse_requirements.bzl:693-703 iterates hashes_by_version[version] and looks up index_urls.whls.get(digest), so omitting "" causes lockfile hits for hashless SimpleAPI entries + hashless requirements.txt to miss the wheel and fall back to pip. Removing if digest: restores round-trip parity (and lets pypi_cache_tests.bzl:553 expect {"1.0.0": [""]}).
This is an agent-generated comment reviewed and approved by the user.
|
Moved the author's AI disclosure note from the PR description into this comment (and added a brief
This is an automatically generated agent comment. |
Improve `CHANGELOG.md` readability by mentioning `{obj}\`pip\`` and
`MODULE.bazel.lock`, wrapping lines at 80 columns, and formatting the
`bazel-contrib#4162` link in trailing parentheses.
|
Ignas: if the bot comments aren't helpful, then just comment/rant to them in response. I feed it back into the code review skill to incrementally improve it. (Especially around verbosity) My rule of thumb is: if the gut reaction is "wtf are you talking about?" then its done a bad job. Human comments are usually clear enough that we think e.g. "I understand what they mean, they're (maybe|certainly) (right|wrong)" |
Align `bazel_binaries` in `MODULE.bazel` and `bzlmod_lockfile_test` in `tests/integration/BUILD.bazel` with `9.3.1` in `tests/integration/bzlmod_lockfile/.bazelversion` (`lockFileVersion: 28`).
It seems that the automatic bzlmod lockfile driver is doing only shallow
merging, let's use a different schema that is much more friendly towards
such workflows. Before this PR users would have to manually resolve git
conflicts or regenerate the lockfile from scratch, with this it will
work better as the bazel authors intended.
Flatten
pipextension lockfile facts (v3) to top-level per-packagekeys (
<pkg>#<index_url>and<root_url> <distribution>) with compactpipe-delimited distribution metadata strings.
Fixes #4162
Related bazelbuild/bazel#31139