Build and run tests for CMake builds #162

StefanBruens · 2024-06-15T01:16:53Z

[ 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.

In case `PYBIND11_PROTOBUF_ENABLE_PYPROTO_API` is not defined, the py_proto_api_ member of the GlobalState singleton is never changed from its default nullptr value. Any code protected by a `GlobalState::instance()->py_proto_api()` check can thus also be made dependent on the `PYPROTO_API` define. This allows to remove the dependency on the proto_api.h header file. As the call to check_unknown_fields::CheckRecursively is also protected by the `py_proto_api()` it can be stubbed out. See pybind#127.

The PYBIND11_PROTOBUF_ENABLE_PYPROTO_API define was never set in CMake builds, add a corresponding option. As the check_unknown_fields code is only called dependent on the define remove it from the build if not used.

As pybind11 does not guarantee stable ABI for its types (e.g. pybind11::handle), these types have "hidden" visibility at least on Linux. This visibility is propagated to any method which has any of the pybind11 types in its parameters, which is the case for many of the public methods in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED pybind11_native_proto_caster library will create linker errors to to undefined symbols for any non-trivial case. The other approach would be to move everything which uses a pybind11 type to the headers, and only leave pybind11 agnostic methods in the shared library. While this is trivially possible for e.g. PyBytesAsStringView, for the majority of the methods (e.g. anything depending on the GlobalState object) a significant refactoring would be required. See pybind#160.

The current implementation reimplements CMake's native handling of system packages, i.e. "FIND_PACKAGE_ARGS" in `FetchContent_Declare`. Add options to force usage of system provided abseil-cpp/protobuf/pybind11, to avoid having different versions here and in dependent packages. Use the correct version for protobuf, v23.3 corresponds to 4.23.3. The output for FetchContent is a little bit less verbose now, but in case debugging is required it is much more useful to use the "-DFETCHCONTENT_QUIET=False" option.

Building and running tests was never implemented for CMake, create the same test cases as for the Bazel build. Use the standard "BUILD_TESTING" option (implicitly added by CTest).

rwgk · 2024-06-15T04:49:44Z

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 · 2024-06-15T14:13:42Z

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 · 2024-06-15T14:18:55Z

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 · 2024-06-15T15:31:47Z

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 · 2024-06-15T16:45:20Z

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

protocolbuffers/protobuf#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 · 2024-06-15T16:51:51Z

@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.

rwgk · 2024-06-15T17:03:05Z

.github/workflows/ubuntu-build.yml

+      - name: CTest Run
+        shell: bash
+        run: |
+          ctest --test-dir ./build --output-on-failure


Could you please do something like this here?

echo 'DISABLED ctest --test-dir ./build --output-on-failure'

Just enough that GHA succeed. I think it's best to merge and do follow-on work separately.

(Aiming for merging on Monday.)

StefanBruens · 2024-06-15T21:12:18Z

@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:

You can not build with the system provided protobuf
You can not use the system provided python-protobuf, as that has a different version than the fetched protobuf
You can not use python-protobuf from the fetched sources, as that is not built

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

rwgk · 2024-06-16T04:49:59Z

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 · 2024-06-16T13:18:33Z

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 · 2024-06-21T21:03:52Z

@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 · 2024-06-26T16:18:48Z

@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.

Add the missing definitions for building the test when using the CMake build. Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs. PiperOrigin-RevId: 647572543

rwgk · 2024-06-28T07:18:25Z

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.

* Add the missing definitions for building the tests when using the CMake build. Actually running the tests is left for another PR (see #162 for difficulties encountered): * #162 (comment) Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs. PiperOrigin-RevId: 647572543

* Add the missing definitions for building the tests when using the CMake build. Actually running the tests is left for another PR (see #162 for difficulties encountered): * #162 (comment) Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs. PiperOrigin-RevId: 647802705

rwgk · 2024-06-29T05:43:17Z

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

pybind11_protobuf/tests/BUILD: extension_disallow_unknown_fields_test was removed (ideally that would have been done already in PR Strip out code that depends on PyProto_API when exporting from Google codebase to GitHub. #165).
Correspondingly this comment in PR Build and run tests for CMake builds #162 was NOT included in Import https://github.com/pybind/pybind11_protobuf/pull/162: #166:

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

Thanks to @laramiel for catching this!

StefanBruens · 2024-06-29T12:17:06Z

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.

StefanBruens added 4 commits June 15, 2024 02:54

Add ENABLE_PYPROTO_API option to CMake build, clean up compilation

18b906f

The PYBIND11_PROTOBUF_ENABLE_PYPROTO_API define was never set in CMake builds, add a corresponding option. As the check_unknown_fields code is only called dependent on the define remove it from the build if not used.

StefanBruens force-pushed the cmake_add_tests branch 2 times, most recently from ad10fd1 to 1a48382 Compare June 15, 2024 01:47

[CMake] Build and run the test cases

d7d37cd

Building and running tests was never implemented for CMake, create the same test cases as for the Bazel build. Use the standard "BUILD_TESTING" option (implicitly added by CTest).

StefanBruens force-pushed the cmake_add_tests branch from 1a48382 to d7d37cd Compare June 15, 2024 01:51

StefanBruens force-pushed the cmake_add_tests branch 2 times, most recently from f744f92 to d64b104 Compare June 15, 2024 15:19

rwgk reviewed Jun 15, 2024

View reviewed changes

StefanBruens force-pushed the cmake_add_tests branch from d64b104 to 42519bf Compare June 15, 2024 20:02

StefanBruens force-pushed the cmake_add_tests branch from 42519bf to d7d37cd Compare June 16, 2024 15:48

Ralf W. Grosse-Kunstleve added 2 commits June 21, 2024 13:44

Revert some changes after pybind#165 was merged.

2f79409

Merge branch 'main' into cmake_add_tests

1b2b00d

copybara-service bot mentioned this pull request Jun 28, 2024

Import https://github.com/pybind/pybind11_protobuf/pull/162: #166

Merged

rwgk closed this Jun 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build and run tests for CMake builds #162

Build and run tests for CMake builds #162

StefanBruens commented Jun 15, 2024

rwgk commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

rwgk commented Jun 15, 2024

rwgk commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

rwgk Jun 15, 2024

StefanBruens commented Jun 15, 2024

rwgk commented Jun 16, 2024

StefanBruens commented Jun 16, 2024

rwgk commented Jun 21, 2024

StefanBruens commented Jun 26, 2024

rwgk commented Jun 28, 2024

rwgk commented Jun 29, 2024

StefanBruens commented Jun 29, 2024

Build and run tests for CMake builds #162

Build and run tests for CMake builds #162

Conversation

StefanBruens commented Jun 15, 2024

rwgk commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

rwgk commented Jun 15, 2024

rwgk commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

StefanBruens commented Jun 15, 2024

rwgk Jun 15, 2024

Choose a reason for hiding this comment

StefanBruens commented Jun 15, 2024

rwgk commented Jun 16, 2024

StefanBruens commented Jun 16, 2024

rwgk commented Jun 21, 2024

StefanBruens commented Jun 26, 2024

rwgk commented Jun 28, 2024

rwgk commented Jun 29, 2024

StefanBruens commented Jun 29, 2024