Skip to content

Conversation

DownerCase
Copy link
Contributor

@DownerCase DownerCase commented Apr 10, 2025

Description

Updates protobuf from 3.11.4 to 5.29.4.
Originally it would have been to 4.25.x but:
a. That major version just went out of maintenance support.
b. There is a regression in that version with msvc and /permissive- which impacts eCAL.
c. Jumping to 5.x didn't seem to need any additional work...
d. 5.x gives a year of maintenance support

Unfortunately, updating protobuf has some knock on effects.

  1. Protobuf now depends on the abseil library collection.
  2. So abseil needed to be added as a submodule, wired in, and gets vendored out into the install tree along with protobuf
  3. Abseil had some target names that conflicted with those in eCAL so they had to be changed to avoid collisions.
  4. Abseil has strict ABI rules and requires everything that uses it to be compiled with the same C++ version as it.
  5. This means I had to add a mechanism to detect and enforce an appropriate C++ version for the project via CMAKE_CXX_STANDARD
  • The majority of the time this will be set to C++17 due to the use of FTXUI or QT6
  • Moving the project to strictly require C++17 is not something currently needed, but will be for protobuf 6.x
  1. Some executables/plugins were requiring C++17 when not needed so they have been bumped back to C++14
  • Without this they try and use protobuf with std::string_view APIs but as protobuf was compiled with C++14 they would be missing and cause link failures.
  1. The update to protobuf removes the protobuf-protos target so it is removed from the innosetup script
  • (The files it use to include are now part of protobuf-headers)
  1. There was a typo in the C# CMake for determining the protobuf version I corrected.
  2. To catch integration problems (like UPB needing to be disabled) I added a simple test to the Windows workflow
  • Windows specifically as the others use system packages for protobuf.

See also:

WARNING I have not lent any attention to the Python or C# (beyond fixing an obvious bug) binding situation (but no CI job has started failing). If there's something that needs attention point me at it and I'll have a go 😄

Related issues

Related to #1187 (I'll let the team decide if this closes it or not)
Superceeds #1818
Plus a bunch of other open issues mainly relating to the Python situation.

@DownerCase DownerCase marked this pull request as ready for review April 10, 2025 18:43
@KerstinKeller KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Apr 10, 2025
@KerstinKeller
Copy link
Contributor

It's a big one. What I find a bit strange - though nice: it's only build system changes. So apparently nothing of the API parts which we used have been changed. Which is great, and unexpected.

I am a bit confused though, especially regarding ftxui, since you downgraded the c++ standard there (mon_cli for example) - that it builds, because my understanding was that C++17 was a requirement there.
Also it was downgraded in some protobuf samples, I really though that there was a reason that I have upgraded it recently.

Also, Protobuf is no longer a dependency of eCAL core.
We use it for the apps, and then of course the service and pubsub protobuf extensions.
Having a change like this, I wonder if we should go into the direction that users using eCAL&Protobuf should build the protobuf extension themselves, because then they have full control of the protobuf version, their C++ standard, ... But I know that not all eCAL devs will share that point of view.

but I will take a proper look tomorrow.

@DownerCase
Copy link
Contributor Author

It's a big one. What I find a bit strange - though nice: it's only build system changes. So apparently nothing of the API parts which we used have been changed. Which is great, and unexpected.

I am a bit confused though, especially regarding ftxui, since you downgraded the c++ standard there (mon_cli for example) - that it builds, because my understanding was that C++17 was a requirement there. Also it was downgraded in some protobuf samples, I really though that there was a reason that I have upgraded it recently.

Also, Protobuf is no longer a dependency of eCAL core. We use it for the apps, and then of course the service and pubsub protobuf extensions. Having a change like this, I wonder if we should go into the direction that users using eCAL&Protobuf should build the protobuf extension themselves, because then they have full control of the protobuf version, their C++ standard, ... But I know that not all eCAL devs will share that point of view.

but I will take a proper look tomorrow.

Thanks Kerstin.

For protobuf the major version changes on the C++ API are nominal and scheduled yearly*, so going from 3.x -> 5.x hasn't required eCAL to change anything. Going to 6.x will require changes because they move signatures from const std::string& to std::string_view (expect another patch set after this one addressing that).

Technically ecal_mon_tui uses FTXUI so it implicitly gains a C++17 requirement, I was just doing a grep for cxx_std_17 and making sure nothing was using both that and protobuf. Also, perftool could get away with using C++17 but turns out it doesn't need it 🤷
The protobuf samples absolutely do need to be bumped back.

I can't speak for anyone except me, but protobuf is pretty much the only serializer I have used with eCAL, I consider it a "core" feature.

Splitting off "ecal-protobuf" could work as its fairly simple, and wouldn't be too hard for people to integrate into their builds, one way or another.

That said if nothing is done, this change will, with great certainty, bite someone if the binaries continue to be supplied in the current methods. Personally I'm against including the SDK in the windows installer (just leave it as the app installer) and would direct people to Conan/vcpkg/others. I appreciate that's considerable extra effort, but am willing to help. On the Linux side Debian-based distros are stuck on the pre-abseil protobuf versions so not an imminent concern yet. I've taken up maintainership of eCAL for the AUR (where protobuf is updated) and am working through all the issues; but here there aren't exactly many users.


* Some extra reading on Protobuf versions if you care:

https://protobuf.dev/support/version-support/
https://protobuf.dev/support/migration/
https://protobuf.dev/support/migration/#cpp-22

@Peguen Peguen added cherry-pick-to-support/v6.0 Cherry pick these changes to support/v6.0 and removed cherry-pick-to-NONE Don't cherry-pick these changes labels Apr 22, 2025
@DownerCase DownerCase force-pushed the update-protobuf branch 2 times, most recently from f241845 to 4c9eea8 Compare June 7, 2025 11:10
@DownerCase DownerCase changed the title [build] Update protobuf to 5.29.4 [build] Update protobuf to 5.29.5 Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-support/v6.0 Cherry pick these changes to support/v6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants