Skip to content
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

protobuf 5.26.1 #215

Merged
merged 93 commits into from
Sep 19, 2024
Merged

protobuf 5.26.1 #215

merged 93 commits into from
Sep 19, 2024

Conversation

h-vetinari
Copy link
Member

This is going to be a bigger operation I fear - just opening this PR as a status update for now. Protobuf removed the usual setup.py approach, and we now either need to deal with the PyPI sources (harder to patch) or build with bazel.

Also, the new way to build python from source seems to just rebuild a lot of stuff we already have in libprotobuf (e.g. utf8_range), and more importantly, also upb, which is also being vendored by grpc, causing potential clashes there. I had tried to avoid this in protocolbuffers/protobuf#12927 but to no avail (c.f. also #170).

The C++ backend is also removed, so I guess it's not even possible anymore to build atop of libprotobuf.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

knedlsepp added a commit to knedlsepp/nixpkgs that referenced this pull request May 19, 2024
In order to enable bumping the default protobuf version from protobuf_24
to protobuf_25, we address the build failure of pythonPackages.protobuf
against that version.
Protobuf's python package is moving away from cpp backend in favor of a
µpb backend.
(See: https://github.com/protocolbuffers/protobuf/tree/main/upb)
The work on that seems to have lead to the introduction of a broken test
in "minimal_test.py":
protocolbuffers/protobuf@501ecec
I suspect that this is not an issue on the nixpkgs packaging end but
rather that this file is uncovered code upstream. I don't know enough
about Bazel to be sure, but it looks like that file is not part of their
protobuf/python/BUILD.bazel file.
(I wanted to prove that in
protocolbuffers/protobuf#16888 but couldn't
trigger upstream's CI)
So for now let's just skip that file.
Note that protobuf_26.tests.pythonProtobuf is still broken. This is due
to the fact that upstream removed support for building the library
directly from the GitHub repo. (See:
protocolbuffers/protobuf#15708)
Conda packaging is also currently struggling with this:
conda-forge/protobuf-feedstock#215

Related: NixOS#264902
@h-vetinari h-vetinari changed the title WIP: protobuf 5.26.1 WIP: protobuf 5.27.2 Jun 28, 2024
@xhochy xhochy requested review from ocefpaf, xhochy, xylar and a team as code owners September 19, 2024 09:05
@xhochy
Copy link
Member

xhochy commented Sep 19, 2024

I want to do 2-3 more cleanups of possibly unnecessary things in the Windows build, but this is already ready for review, and I would like to merge it on my own if I get a positive review.

@xhochy xhochy changed the title WIP: protobuf 5.26.1 protobuf 5.26.1 Sep 19, 2024
@h-vetinari
Copy link
Member Author

Spectacular work! ❤️🙏

I was wondering how we can check that the use_fast_cpp_protos bits are working. According to the link check, both libprotobuf and libabseil are unused... Also we might want to double-check (not sure yet how) if the upb-symbols are actually made invisible by protocolbuffers/protobuf#17207, because otherwise we'll get clashes with the upb in grpc.

@xhochy
Copy link
Member

xhochy commented Sep 19, 2024

According to the link check, both libprotobuf and libabseil are unused...

Yes, they are statically linked, and there is no system libs option. I would probably be able to build one, but I don't have the patience for this atm. We would have to copy what Tensorflow does over here.

@xhochy
Copy link
Member

xhochy commented Sep 19, 2024

From my side, this is ready for a merge. @h-vetinari feel free to add what you want on top. There is at least one unit test that checks for use_fast_cpp_protos. I'm not sure though whether unit tests are an easy feast here.

@h-vetinari
Copy link
Member Author

OK, I'm fine to merge this for now. We'll have to keep iterating anyway going forward. I'd also tend towards not migrating 5.26, as that seems to have already been fully dropped upstream (i.e. 5.25, 5.27 & 5.28 receive patch releases, but not 5.26).

My preference would be to build 5.27.5 against the new abseil, bootstrap a recent grpc on top (1.66?), and then try for a migration. We can also discuss this in conda-forge/conda-forge-pinning-feedstock#4075

@xhochy
Copy link
Member

xhochy commented Sep 19, 2024

Then, I'll merge this and spent some time on migrating the patches for each release until we hit the newest. If we at one point still need an older release, we then have them.

@xhochy xhochy merged commit e35ebe1 into conda-forge:main Sep 19, 2024
32 checks passed
@h-vetinari
Copy link
Member Author

I don't think we need the older patch releases for 5.27.x; I think we could go directly for 5.27.5 from here, but if you wanna build out the whole line, go for it. At least the libprotobuf bits should be there already.

Yes, they are statically linked, and there is no system libs option.

We might want to remove the run-deps here then for now

@h-vetinari h-vetinari deleted the bump branch September 19, 2024 12:17
@h-vetinari
Copy link
Member Author

oh well. 🙃

@xhochy
Copy link
Member

xhochy commented Sep 19, 2024

We can remove them in a follow-up PR. Packages with smaller dependencies will be preferred.

@jjerphan
Copy link
Member

Thank you, @xhochy. 🙏

@h-vetinari
Copy link
Member Author

h-vetinari commented Sep 19, 2024

We can remove them in a follow-up PR. Packages with smaller dependencies will be preferred.

I know, it's no big deal.

@h-vetinari
Copy link
Member Author

Though we might want to still have a run-constraint? I guess we'd open ourselves up to some possibly weird interactions if we let the versions of protobuf and protoc/libprotobuf diverge in any given build/environment...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants