-
Notifications
You must be signed in to change notification settings - Fork 516
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
Support bzlmod #870
Support bzlmod #870
Conversation
e2a0bbf
to
3c5615b
Compare
fyi: @alexeagle, @Wyverald |
28f9f64
to
68f4841
Compare
My understanding is that something was added after you wrote the yaml parser, which permits one module extension to see a workspace that was created by another, and so perhaps that means it's possible to fetch some tool (like yq) and then execute it from a module extension. Perhaps that means we could use an existing requirements.txt parser like from pip or poetry that's likely to handle all the edge cases correctly. I doubt it, but might be worth a bit of poking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @hrfuller can help with reviews as well, he has a good memory and wrote the pip_parse stuff.
env = unittest.begin(ctx) | ||
|
||
asserts.equals(env, [ | ||
("certifi", "certifi==2021.10.8 --hash=sha256:78884e7c1d4b00ce3cea67b44566851c4343c120abd683433ce934a68ea58872 --hash=sha256:d62a0163eb4c2344ac042ab2bdf75399a71a2d8c7d47eac2e2ee91b9d6339569"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you only need the name for the module extension implementation, and not the hashes? I thought you'd need all the data required to produce a requirements.bzl file, like the hashes for downloading the packages.
OH I remember - the pip_parse rule doesn't actually know how to download anything, it will still end up calling pip install
on individual packages which are required by the build...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is the same requirement info that gets passed to whl_library
.
Yeah originally I wanted to call |
That's not directly possible (you can't declare a repo in an extension and immediately read from that repo), but you can use the trick mentioned by Alex: have an extension declare a
Alternatively, this works too. A module extension can create files, but these files are not addressable after the extension impl function finishes. |
Thanks for the clarification. That's interesting that a module context can create temporary files. I think one extension is a better developer experience than two, so I'll look into that approach and see where it leads me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool use of a state machine, thanks for working on this.
Looks good. I've got a different proposal to consider and wondered what you might think.
The benefits of the json installation report has a few benefits:
The direct url info in particular will be helpful in future, because it would enable us to parse the format into |
This might be built into Bzlmod in the future, without you guys having to do anything. With the planned lockfiles feature, Bzlmod will store the results of module extension resolution in the lockfile; said result is actually just the names and definitions of the repos that the extension generated. |
That would be something different to what I mention above. That would be for locking the bazel rules dependency graph, not the dependency graph(s) of resolved python packages from external sources. There is no mechanism for bzlmod to do that presently because we aren’t capturing it presently. Hence my line of questioning. |
I don't think it's ever a good idea for Bazel to manage the lockfile when a language-idiomatic package manager manages the dependencies and constraints. Then you get a different result outside of Bazel and the language tooling gets confused. Anyhow this PR is just to make a bzlmod module extension for the existing repository rules, not to redesign them. |
I'm going to hold off on this for now and see if I can just execute a python script in the module extension to do the parsing (using the same library that pip_parse uses). |
@kormide Note that even though |
That's good to know. I had ruled that out because I thought that users would have to declare both extensions. I'm going to put up a solution with a single module extension first because I'm close to getting that working, and if it feels like there's too much duplication I can go back to the two extension approach. |
68f4841
to
5cd9328
Compare
5cd9328
to
279a6be
Compare
I repurposed this PR to include the full bzlmod solution. Commit 1: requirements starlark parser (added support for parsing pip options) I ended up going with the starlark parsing of requirements as an initial solution here. The alternative is to add an additional module extension that parses the requirements file and outputs something like JSON that starlark can read. In the end, (I think) we need to have the |
.bcr/template.metadata.json
Outdated
"maintainers": [ | ||
{ | ||
"name": "Richard Levasseur", | ||
"email": "rlevasseur@google.com", | ||
"github": "rickeylev" | ||
}, | ||
{ | ||
"name": "Greg Roodt", | ||
"email": "groodt@gmail.com", | ||
"github": "groodt" | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a template for an entry that will appear the Bazel Central Registry (example).
I wasn't sure who the main maintainers of rules_python are. @rickeylev @groodt are you okay with being listed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kormide you can also add me.
279a6be
to
a333d2b
Compare
12367fd
to
c91bb3c
Compare
def requirement(name): | ||
if _bzlmod: | ||
return "@@{repo}//:" + _clean_name(name) + "_{py_library_label}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This points calls to xyz_requirement(...)
to the aliases inside the pip_parse repo, so that we don't need to declare a bunch of use_repo
s in MODULE.bazel.
c91bb3c
to
1dfeacd
Compare
1dfeacd
to
8b7afd9
Compare
Just realized that I still need to support registering the python toolchain via bzlmod. The tests are failing on CI because the bzlmod example doesn't use a consistent python version on all platforms (I think it's using whatever the executor has installed), and it's outputting the python version to the vendored requirements file, causing a diff_test to fail. |
8b7afd9
to
881d5f8
Compare
Added a module extension for registering a python toolchain. |
|
||
python = use_extension("@rules_python//python:extensions.bzl", "python") | ||
|
||
python.toolchain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does every root module have to manually register a toolchain or is the host Python used if nothing else is specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's optional and will use the host if not specified. I added a comment indicating that in the release workspace_snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of this example, I had to use it to fix the python version because the generated requirements_lock.txt
file stamps the python version in a comment, and on CI the different platforms had different versions installed, so the diff_test was failing.
881d5f8
to
b06ca61
Compare
b06ca61
to
4535cc7
Compare
This PR needs another review since adding the full bzlmod solution, but it's not clear to me who I should ping for that. |
@alexeagle is probably the best person to do the final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! sorry it took me a while to get to this
"pypi__click", | ||
"pypi__colorama", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for later: it's gross how this repository rule depends on packages from pypi, it should be self-contained and require only the python interpreter like what we did in rules_js
4535cc7
to
bb9c5dc
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The requirements file is parsed in a python script that generates
requirements.bzl
and passes information from the file intowhl_library
called byinstall_deps
within the generated file. In order to declare the wheel repositories in a bzlmod extension, we need to be able to parse that same information in Starlark.What is the new behavior?
Can parse a requirements file in Starlark which will be uses in a pip_parse bzlmod extension. This in in a similar vein to how
rules_js
parses a pnpm lockfile in starklark for thenpm_translate_lock
extension.Does this PR introduce a breaking change?
Other information
This isn't currently used, but it's a nice reviewable chunk for the bzlmod work.