java: updated protobuf-java dep version, moved local build to docker #121
Conversation
java/Dockerfile
Outdated
| RUN curl -OL https://dlcdn.apache.org/maven/maven-3/$MAVEN_VERSION/binaries/apache-maven-$MAVEN_VERSION-bin.zip \ | ||
| && unzip -o apache-maven-$MAVEN_VERSION-bin.zip -d /usr/local \ | ||
| && rm -f apache-maven-$MAVEN_VERSION-bin.zip | ||
| ENV PATH=$PATH:/usr/local/apache-maven-$MAVEN_VERSION/bin |
There was a problem hiding this comment.
Can you use the official Maven docker images instead? https://hub.docker.com/_/maven
There was a problem hiding this comment.
Or via apt-get as described above? https://packages.debian.org/bullseye/maven
There was a problem hiding this comment.
The official Maven docker image works!
| RUN curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v21.12/$PROTOC_ZIP \ | ||
| && unzip -o $PROTOC_ZIP -d /usr/local bin/protoc \ | ||
| && unzip -o $PROTOC_ZIP -d /usr/local 'include/*' \ | ||
| && rm -f $PROTOC_ZIP |
There was a problem hiding this comment.
I'm wondering if we can install protoc via apt-get as well? Aka https://packages.debian.org/bullseye/protobuf-compiler
Might be more concise
There was a problem hiding this comment.
Trying to install with apt-get seems to introduce a bunch of additional dependencies that (IMO) would just be bloat here. Given that, I'd prefer to keep things simple and continue using the existing installation pattern
There was a problem hiding this comment.
Does that bloat really matter? These are just temporarily installed dependencies, not something that will be included in a published image.
There was a problem hiding this comment.
@bdferris-v2 I'm running into a couple issues here.
- it looks like the default
mavendocker image uses Ubuntu. On Ubuntu's package repo, I'm only able to find a significantly older version ofprotobuf-compiler(3.12.4). It looks like this also causes a diff in the codegen, so I would prefer to use a more up-to-date version of the compiler. Not sure if someone can push up a more recent version of that to Ubuntu? - I tried to switch over to
maven'salpineimage, but I'm running into a number of issues likeapknot being able to find theprotobuf-compilerpackage for some reasonapk add protobuf-compiler. I'm also fighting with getting Alpine docker images to play nicely with M1 Apple Silicon hardware :(
There was a problem hiding this comment.
From what I can tell, the least platform dependent way (and most fine-grained as far as maintaining a consistent protoc version) is to install still seems to be installing from source rather than using a package manager. What do you think about keeping it that way?
There was a problem hiding this comment.
I guess the old protobuf-compiler images on Unbuntu jammy are probably reason enough to avoid that path and install the binary directly. LGTM then.
| # copy the file from the named container running as a daemon onto the host machine | ||
| docker cp gtfs-java-container:/lib/src/main/java/com/google/transit/realtime/GtfsRealtime.java $JAVA_DIR/src/main/java/com/google/transit/realtime/GtfsRealtime.java | ||
| # delete the named container | ||
| docker rm -f gtfs-java-container |
There was a problem hiding this comment.
I wonder if it's worth abstracting out a generic "updated_generated_code_from_docker.sh" helper script that contains the largely duplicated bits of this script and https://github.com/MobilityData/gtfs-realtime-bindings/blob/master/golang/update_generated_code.sh
There was a problem hiding this comment.
@bdferris-v2 are you ok with me creating this as a separate issue that we can resolve later? I'd prefer to get these new Java bindings merged + deployed first. That will allow a subsequent PR to be simpler and more targeted to improving the code sharing
There was a problem hiding this comment.
Sure, please open a bug.
…MobilityData#123) Fixed deprecated method in Java Gradle dependency - replaced compile with implementation. Closes MobilityData#122.
jameslinjl
force-pushed
the
james/java/update-deps-and-move-local-build-to-docker
branch
from
1b2296e
to
13d8d73
Compare
| @@ -64,7 +64,7 @@ | |||
| <dependency> | |||
| <groupId>com.google.protobuf</groupId> | |||
| <artifactId>protobuf-java</artifactId> | |||
| <version>3.16.1</version> | |||
| <version>3.21.12</version> | |||
There was a problem hiding this comment.
Do you want to try to keep this in sync with the version of protoc used?
There was a problem hiding this comment.
@bdferris-v2 yes! this is in-sync, right? they should all be [3.]21.12
There was a problem hiding this comment.
Ah, I misunderstood the protobuf versioning scheme.
protobuf-javafrom3.16.1to3.21.12to resolve some runtime issues