-
Notifications
You must be signed in to change notification settings - Fork 9
Add pypi package providing buf.validate gencode #367
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
base: main
Are you sure you want to change the base?
Conversation
|
hey @lukasbindreiter, thanks for the PR! looks good to me from the outset.
Appreciate you moving these over and getting them to work; I don't care for the sed trick although I know that's just the way with python & protobuf :(. I put up an alternative approach of moving just the test dependencies to using generated SDKs in #368, thoughts? Not sure if having the index declared in pyproject.toml would cause issues for your downstream packaging (although I doubt it?).
Looks good to me so far.
What's the plan for versioning the package? |
Oh that's a great idea, didn't think of that but I like it a lot, the
Nope, that shouldn't be an issue I think.
Since the package anyway includes multiple plugin versions, the idea was to just mirror the tag from the protofiles, e.g. the latest version would be What's still missing though is the Github Action which checks if a new tagged version exists, and if so builds and pushes it to pypi. My idea for that is to eventually have the main protovalidate-python just depend on the pypi.org version of |
yep, seems reasonable. I think if we add the functionality here, we could bootstrap the package with a single workflow run & then dep on it locally, and remove the workspace member? (I'm not super familiar with uv workspaces yet, but assuming we'd want to drop that once we have the proper dep?) |
9eeab79 to
50b07d5
Compare
50b07d5 to
773b131
Compare
|
@stefanvanburen I've now pushed a first version to Test PyPI: https://test.pypi.org/project/bufbuild-protovalidate-protocolbuffers/ And now have bootstrapped Could you take a look at the workflow in I'm not sure what's required for the workflow to run now, @stefanvanburen could you trigger it somehow manually as repo maintainer? |
Not sure if there's a way for me to run actions until they land on main, I'm afraid. But it looks close enough that we can iterate on it separately using test-pypi until it's right.
makes sense, yeah I think that's probably the last bit we want to get working here (besides the test failures). From there I think I have enough where I can probably pull out the subdirectory into a separate repo & get it working fairly quickly. Ideally we'd publish all of the pre-existing protovalidate versions, but if that's not feasible for whatever reason we could also just publish them moving forward (since e.g. we'll only add this dep for presumably |
|
Lets revert df3d2b0; I don't think we need to format the generated code. |
Sure, I reverted it now, and added the paths to the
Got everything running just now.
@stefanvanburen Can you take another quick look at the This means for now the publishing process is manual still, but it does allow us to then test the publishing workflow against
My idea was to write a script that fetches all versions from |
yep! I think that's the last bit I'd like to see in this PR, and then I can pretty much move forward and get it integrated. Would be great if you could include that, otherwise I can try to get to it in the next couple days. Otherwise, agree with next steps in terms of getting it integrated as a dependency here. |
|
took me a while to get around to it, but I just now added the script: The way I query the with the following request data I think for a small script like that it's easier to just do that rather than including the LabelService grpc and messages as dependency here. But feel free to change that if another way of fetching the tags is preferred. Usage of the script: Afterwards which the I've limited the script to building a maximum of 2 packages for now at once, so we can test it over time, and also to avoid rate limiting hopefully. Tomorrow at |
|
The Here is the logs of that job: Here are the releases: |
|
@stefanvanburen quick ping for visibility, would appreciate a quick review if you have some time, so that we can get this over the finish line and hopefully merged soon, especially with the |
hey @lukasbindreiter, yep will definitely take a look as soon as I can :). Should be able to check this out by EOW or early next week. |
Implementation of the outcome of the discussion in #339
Some notes on what this does:
bufbuild-protovalidate-protocolbuffersas a subpackage (anduvworkspace member)protovalidate-pythonpackage now depends on it (see: https://docs.astral.sh/uv/concepts/projects/dependencies/#workspace-member)genfolder entirely, since I think it causes confusion.bufbuild-protovalidate-protocolbuffers
make generate-protovalidate-pypi-packagegenerates the gencode forbuf.build/bufbuild/protovalidategencode into this new packagewandbapproach to support multiple major protobuf runtime versions.makefills in the$(PROTOVALIDATE_VERSION)from the Makefile into thepyproject.tomlof the subpackagegencode for unit tests
make generate-protobuf-testsgenerates the required gencode for the unit teststest/gencel.exprit needs to update the import path totest.gen.cel.exprwhich I do a bit hacky withsedhere, since I think buf doesn't allow me to specify that in python unfortunately.With those changes for me:
make testoruv run pytestwork again as expected.@stefanvanburen what do you think of this package architecture? would appreciate an initial review before I continue further with the package build / publish step.