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

Update to Protobuf 4.29.3 #156

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tredpath
Copy link

@tredpath tredpath commented Feb 6, 2025

Update the protobuf library and protoc to 4.29.3 for #136. Updated sbt-protoc to 1.0.7 and compilerplugin to 0.11.17.

I updated the proto file in geojson-proto to use proto3 syntax. The changes were minimal, only the required flag had to be dropped since it isn't needed anymore.

All tests passed with the updates but I had to modify the build script locally to get the updated geojson-proto library to be used by the core library.
image

jackson-core, commons-compress, and zstd-jni can be updated without breaking anything internal to this library but I didn't include them as part of this pull request.

Update the protobuf library and protoc to 4.29.3.

Update the protobuf build plugins to their most recent versions.
Change the GeoJSON protobuf definition to use proto3 syntax
instead of proto2. The only change is the removal of the
required flag as it isn't available or needed in proto3.
As of December 2024, the ubuntu-latest GitHub runner no longer
contains SBT, it has to be installed manually.
@tredpath
Copy link
Author

tredpath commented Feb 7, 2025

I updated the pull request to install SBT in both the pull request and release actions. It looks like it was dropped from the ubuntu-latest runner as of December last year.

@RomanIakovlev
Copy link
Owner

Here's hoping that changing the geojson-proto version to 1.1.5-SNAPSHOT would resolve the tests failures in this PR.

@tredpath
Copy link
Author

I'm not sure how to best get the tests to run, maybe split it into two pull requests. The way build.sbt is setup the core project always pulls geojson-proto from the dependency manager so it will never make use of local changes. To test it I had to compile the project with changes to geojson-proto, update the core project to reference the compiled output and the protobuf-java library instead of the remote geojson-proto library, then compile it again.

@RomanIakovlev
Copy link
Owner

Yeah, I'm not even sure why the tests are failing now. The only thing that's changed for the core sub-project is the sbt plugin versions, the rest of the changes are only for the geojson-proto, which shouldn't impact the tests of the core.

I generally agree with the idea of trying to make this PR smaller and see if there's some way to make the tests pass while still upgrading the protobuf dependency in the geojson-proto sub-project.

I'm also not sure why we had to switch to proto3 syntax here, is proto2 not supported by protoc 4.x anymore?

@RomanIakovlev
Copy link
Owner

I've done some testing, and it seems we can fix the tests failing by keeping the proto2 syntax and required fields in place. In other words, just revert the commit tredpath/timeshape@1b4a124 from this PR, keeping everything else the same.

@tredpath could you please give it a try? For some reason I couldn't push to your branch myself.

@@ -44,7 +44,7 @@ lazy val core = (project in file("core"))
"com.novocode" % "junit-interface" % "0.11" % Test
exclude ("junit", "junit-dep"),
"org.slf4j" % "slf4j-api" % "1.7.30",
"net.iakovlev" % "geojson-proto" % "1.1.5"
"net.iakovlev" % "geojson-proto" % "1.1.6-SNAPSHOT"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 1.1.5, not 1.1.6-SNAPSHOT. My bad, I was resolving conflicts with master and typed the wrong value.

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.

2 participants