search

pybind / pybind11_protobuf

Pybind11 bindings for Google's Protocol Buffers
Other
54 stars 33 forks source link

Build and run tests for CMake builds #162

Closed StefanBruens closed 1 month ago

StefanBruens commented 2 months ago

[ This MR contains everything from #161, as building is to broken otherwise. MR #161 should be merged first ]

Add the missing definitions for building and running the test when using the CMake build.

rwgk commented 2 months ago

Add the missing definitions for building and running the test when using the CMake build.

Wow, that's awesome, thanks a lot!

Source-of-truth for pybind11_protobuf is the Google codebase, which uses a completely different source code versioning system. I have some hope that we can get automatic import of PRs working early next week. We'll try that on your PRs.

StefanBruens commented 2 months ago

Note, the automatic test run is currently missing in the workflow file, something like the following needs to be added (untested):

      - name: CMake Ctest run
        shell: bash
        run: |
          ctest --output-on-failure --test-dir ./build/
rwgk commented 2 months ago

Note, the automatic test run is currently missing in the workflow file

Oh, I was wondering, thanks!

Could you add that to this PR? (Did you already and it needs more work?)

rwgk commented 2 months ago

Hi @mkruskal-google, is there a chance that you could 1. review the cmake changes here, 2. maybe help troubleshooting the errors in the ubuntu-build / cmake job (in case @StefanBruens hasn't knocked them out already before you get a chance to look)?

StefanBruens commented 2 months ago

Due to some very questionable decisions on the protobuf side, is is likely impossible to enable the tests with a protobuf checkout from github:

https://github.com/protocolbuffers/protobuf/pull/15671

The CMake build does not generate the required python protocols, and thus anything from "google.protobuf" is not usable (e.g. desriptor_pb2.py is missing).

The only way I can see to make the tests work is to only use the system provided protobuf (i.e. protobuf and python3-protobuf). Of course this disallows usage of ENABLE_PYPROTO_API, though that is disabled by default anyway.

StefanBruens commented 2 months ago

@rwgk My current suggestion would be to add another independent build, i.e. have "ubuntu-build / cmake" and "ubuntu-build / cmake-with-system-deps". The latter would use absl/protobuf/pybind11 from the Ubuntu base system (hopefully, that is recent enough), while the other build would keep using the stuff fetched from Github, but only builds the tests without running them.

StefanBruens commented 2 months ago

@rwgk My current suggestion would be to add another independent build, i.e. have "ubuntu-build / cmake" and "ubuntu-build / cmake-with-system-deps". The latter would use absl/protobuf/pybind11 from the Ubuntu base system (hopefully, that is recent enough), while the other build would keep using the stuff fetched from Github, but only builds the tests without running them.

Unfortunately, using system dependencies is not an option on Ubuntu/Debian, as the protobuf-devel package does not ship any CMake files (only fixed in Debian Experimental):

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1027876

So, on Ubuntu pybind11_protobuf is more or less unusable:

I have tested the CMake build and tests on openSUSE Tumbleweed, using the system packages, and all works fine ...

rwgk commented 2 months ago

Ugh ... sorry to see you going through such a series of frustrations.

Do you want to remove the cmake-system-dependencies job again? — Then we can merge this PR (on Monday) and take it from there.

StefanBruens commented 2 months ago

Ugh ... sorry to see you going through such a series of frustrations.

Do you want to remove the cmake-system-dependencies job again? — Then we can merge this PR (on Monday) and take it from there.

Yeah, thats unfortunately the only working solution at the moment. Even Ubuntu 2024.04 does not have a good protobuf package.

rwgk commented 2 months ago

@StefanBruens I merged #165 and then updated this PR accordingly (using git merge main, resolving conflicts; I'm intentionally avoiding rebases and force pushes).

Test pass.

Could you please review my commits? I'll import this PR into the internal repo after your review.

StefanBruens commented 1 month ago

@StefanBruens I merged #165 and then updated this PR accordingly (using git merge main, resolving conflicts; I'm intentionally avoiding rebases and force pushes).

Rebasing is totally fine when using it for merge requests, these are typically not considered public branches. More or less all projects I am aware of (Linux kernel, KDE, many others) use rebases. On the other hand, doing these reverts creates broken intermediate states, i.e. harming e.g. git bisect.

Could you please review my commits? I'll import this PR into the internal repo after your review.

Look ok so far.

rwgk commented 1 month ago

I created the internal change, which is exported to #166.

I'll look for a reviewer internally tomorrow (my timezone).

On the other hand, doing these reverts creates broken intermediate states, i.e. harming e.g. git bisect.

The (my) main reason for avoiding rebases is that all references to commits to that point are invalidated.

In this repo, reverts in a PR don't matter for sure, because we have to go through the internal code base anyway (which exports as one commit).

More typical for github: On pybind11 master we always "Squash and merge". — I.e. with simple merges the developments under a PR are easy to follow (for a reviewer, or backtracking), but master receives only one commit, therefore bisecting later on master will not be affected by whatever happened under the PR.

rwgk commented 1 month ago

166 was merged. The only differences compared to this PR are:

# FIXME What is the difference to the "extension_test"?` comment 
# add_py_test(extension_disallow_unknown_fields)

Thanks to @laramiel for catching this!

StefanBruens commented 1 month ago

I created the internal change, which is exported to #166.

I'll look for a reviewer internally tomorrow (my timezone).

On the other hand, doing these reverts creates broken intermediate states, i.e. harming e.g. git bisect.

The (my) main reason for avoiding rebases is that all references to commits to that point are invalidated.

For this, there must be an external reference in the first place. Github at least is fairly capable to keep track of rebases and keep references intact. The most typical references are done later, i.e. in other MRs/commits, but these reference the commit which had been merged, but then the commit SHA is fixed. And by squashing the commits, all references to them were invalidated anyway.

In this repo, reverts in a PR don't matter for sure, because we have to go through the internal code base anyway (which exports as one commit).

More typical for github: On pybind11 master we always "Squash and merge". — I.e. with simple merges the developments under a PR are easy to follow (for a reviewer, or backtracking), but master receives only one commit, therefore bisecting later on master will not be affected by whatever happened under the PR.

Honestly, the "squash everything" approach is quite bad. By doing so, almost all of the commit messages were lost. Anyone looking for the reasoning of a change (i.e. via git annotate) will have a hard time to figure it out. E.g. there is no longer any mention of the change from SHARED to STATIC libraries, an no mention of the added CMake build options for system dependencies.

The git history should be the source of truth, without any dependencies where the git is hosted and how. Development history should not depend on any "external" sources, like github issues, or something internal to google. If it is not part of the commit message, consider it lost.