Use relocated protobuf runtime #6355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review notes: The PR contains multiple commits:
|
|
Another note: Quarkus itself (or better: some extensions) also pull in protobuf in the exact same version that Quarkus requires via the |
dimas-b
reviewed
Mar 22, 2023
|
The "build tools integrations tests" failed for the "external Gradle build" - fix in the last new commit |
dimas-b
reviewed
Mar 22, 2023
dimas-b
previously approved these changes
Mar 22, 2023
snazy
added a commit
to snazy/nessie
that referenced
this pull request
Mar 23, 2023
The "build tools integrations test" failed for PR projectnessie#6355 for the "external Gradle build". Reason is that the relocated protobuf jar is published via the Shadow plugin. `PublishingHelperPlugin` used the recommended approach to call `ShadowExtension.component(MavenPublication)`, which is fine for Maven poms, but is not enough for Gradle module metadata (the `.module` files published along the `.pom` files). The published Gradle module metadata files reference the "original" jar (named `*-raw.jar` in the Nessie build) in the included `apiElements` and `runtimeElements` variants, but that's wrong for Nessie and inconsistent with the published pom, because we want to _replace_ the "original" jar with the one produced by `ShadowJar`. Relevant pieces that assemble the Gradle component to be published are taken from Gradle's `Java(Base)Plugin` and `ShadowExtension`. The issue can be inspected in the published `.module` file (or one generated via the `generateMetadataFileForMavenPublication`, e.g. from `:nessie-spark-extensions-3.1_2.12`) project. Before this change, the file `build/publications/maven/module.json` contains these variants (simplified for readability): ``` { ... "variants": [ { "name": "apiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "external", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT-raw.jar", ... } ] }, ... (similar for runtimeElements) ... (javadoc + sources follow) { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] } ] } ``` With this change it looks as follows, note that the the duplicated `java-runtime` usage is gone and a `java-api` for the the shadowed bundling is present. This way, "external" builds can properly select the required variant. ``` { ... "variants": [ { "name": "shadowApiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, ... (javadoc + sources follow) ] } ```
snazy
added a commit
to snazy/nessie
that referenced
this pull request
Mar 23, 2023
The "build tools integrations test" failed for PR projectnessie#6355 for the "external Gradle build". Reason is that the relocated protobuf jar is published via the Shadow plugin. `PublishingHelperPlugin` used the recommended approach to call `ShadowExtension.component(MavenPublication)`, which is fine for Maven poms, but is not enough for Gradle module metadata (the `.module` files published along the `.pom` files). The published Gradle module metadata files reference the "original" jar (named `*-raw.jar` in the Nessie build) in the included `apiElements` and `runtimeElements` variants, but that's wrong for Nessie and inconsistent with the published pom, because we want to _replace_ the "original" jar with the one produced by `ShadowJar`. Relevant pieces that assemble the Gradle component to be published are taken from Gradle's `Java(Base)Plugin` and `ShadowExtension`. The issue can be inspected in the published `.module` file (or one generated via the `generateMetadataFileForMavenPublication`, e.g. from `:nessie-spark-extensions-3.1_2.12`) project. Before this change, the file `build/publications/maven/module.json` contains these variants (simplified for readability): ``` { ... "variants": [ { "name": "apiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "external", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT-raw.jar", ... } ] }, ... (similar for runtimeElements) ... (javadoc + sources follow) { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] } ] } ``` With this change it looks as follows, note that the the duplicated `java-runtime` usage is gone and a `java-api` for the the shadowed bundling is present. This way, "external" builds can properly select the required variant. ``` { ... "variants": [ { "name": "shadowApiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, ... (javadoc + sources follow) ] } ```
snazy
force-pushed
the
pb-relocate
branch
2 times, most recently
from
20f1a26 to
edd0fbe
Compare
|
Ugh - turned out that the published Gradle module metadata produced via the recommended way is broken. Created #6361 to fix it. |
snazy
commented
Mar 23, 2023
| dependencies { include(dependency(libs.protobuf.java.get())) } | ||
| } | ||
|
|
||
| tasks.named("compileJava") { finalizedBy(shadowJar) } |
There was a problem hiding this comment.
Note: this and the following are sadly necessary :(
dimas-b
previously approved these changes
Mar 23, 2023
| @@ -185,6 +196,7 @@ class PublishingHelperPlugin : Plugin<Project> { | |||
| // enforcedPlatform() dependency - but Quarkus requires enforcedPlatform(), so we have to | |||
| // allow it. | |||
| tasks.withType<GenerateModuleMetadata>().configureEach { | |||
| val t = this | |||
snazy
added a commit
to snazy/nessie
that referenced
this pull request
Mar 23, 2023
The "build tools integrations test" failed for PR projectnessie#6355 for the "external Gradle build". Reason is that the relocated protobuf jar is published via the Shadow plugin. `PublishingHelperPlugin` used the recommended approach to call `ShadowExtension.component(MavenPublication)`, which is fine for Maven poms, but is not enough for Gradle module metadata (the `.module` files published along the `.pom` files). The published Gradle module metadata files reference the "original" jar (named `*-raw.jar` in the Nessie build) in the included `apiElements` and `runtimeElements` variants, but that's wrong for Nessie and inconsistent with the published pom, because we want to _replace_ the "original" jar with the one produced by `ShadowJar`. Relevant pieces that assemble the Gradle component to be published are taken from Gradle's `Java(Base)Plugin` and `ShadowExtension`. The issue can be inspected in the published `.module` file (or one generated via the `generateMetadataFileForMavenPublication`, e.g. from `:nessie-spark-extensions-3.1_2.12`) project. Before this change, the file `build/publications/maven/module.json` contains these variants (simplified for readability): ``` { ... "variants": [ { "name": "apiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "external", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT-raw.jar", ... } ] }, ... (similar for runtimeElements) ... (javadoc + sources follow) { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] } ] } ``` With this change it looks as follows, note that the the duplicated `java-runtime` usage is gone and a `java-api` for the the shadowed bundling is present. This way, "external" builds can properly select the required variant. ``` { ... "variants": [ { "name": "shadowApiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, ... (javadoc + sources follow) ] } ```
snazy
added a commit
that referenced
this pull request
Mar 23, 2023
The "build tools integrations test" failed for PR #6355 for the "external Gradle build". Reason is that the relocated protobuf jar is published via the Shadow plugin. `PublishingHelperPlugin` used the recommended approach to call `ShadowExtension.component(MavenPublication)`, which is fine for Maven poms, but is not enough for Gradle module metadata (the `.module` files published along the `.pom` files). The published Gradle module metadata files reference the "original" jar (named `*-raw.jar` in the Nessie build) in the included `apiElements` and `runtimeElements` variants, but that's wrong for Nessie and inconsistent with the published pom, because we want to _replace_ the "original" jar with the one produced by `ShadowJar`. Relevant pieces that assemble the Gradle component to be published are taken from Gradle's `Java(Base)Plugin` and `ShadowExtension`. The issue can be inspected in the published `.module` file (or one generated via the `generateMetadataFileForMavenPublication`, e.g. from `:nessie-spark-extensions-3.1_2.12`) project. Before this change, the file `build/publications/maven/module.json` contains these variants (simplified for readability): ``` { ... "variants": [ { "name": "apiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "external", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT-raw.jar", ... } ] }, ... (similar for runtimeElements) ... (javadoc + sources follow) { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] } ] } ``` With this change it looks as follows, note that the the duplicated `java-runtime` usage is gone and a `java-api` for the the shadowed bundling is present. This way, "external" builds can properly select the required variant. ``` { ... "variants": [ { "name": "shadowApiElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.jvm.version": 8, "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-api" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, { "name": "shadowRuntimeElements", "attributes": { "org.gradle.category": "library", "org.gradle.dependency.bundling": "shadowed", "org.gradle.libraryelements": "jar", "org.gradle.usage": "java-runtime" }, "files": [ { "name": "nessie-protobuf-relocated-0.52.4-SNAPSHOT.jar", ... } ] }, ... (javadoc + sources follow) ] } ```
Generated protobuf code must be run with the protobuf runtime using the exact same version as the generator used. This is currently a soft constraint, but it is likely that protobuf will enforce that in future protobuf versions. Using a different version of the protobuf runtime can (and does) lead to issues at runtime, see [this issue for an example](protocolbuffers/protobuf#11986). This change adds a new Nessie artifact `nessie-protobuf-relocated` which contains a relocated protobuf runtime that is used by all other Nessie artifacts. Also: * Move `*.proto` files into the directories corresponding to the Java package names. * Remove the `ProtobufHelperPlugin`, as the underlying issue (not exposing the generated java source folders in IDEs) has been fixed. * Do not apply checkstyle to Gradle projects that just generate protobuf code (the `ProtobufHelperPlugin` did that before). Side note: CEL-Java heavily depends on protobuf itself via its API and not just as a runtime/compile-only dependency. As long as no generated protobuf code is being used, it _should_ be okay to use a different protobuf version at runtime, but there is and will be no guarantee.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generated protobuf code must be run with the protobuf runtime using the exact same version as the generator used. This is currently a soft constraint, but it is likely that protobuf will enforce that in future protobuf versions. Using a different version of the protobuf runtime can (and does) lead to issues at runtime, see this issue for an example.
This change adds a new Nessie artifact
nessie-protobuf-relocatedwhich contains a relocated protobuf runtime that is used by all other Nessie artifacts.Also:
*.protofiles into the directories corresponding to the Java package names.ProtobufHelperPlugin, as the underlying issue (not exposing the generated java source folders in IDEs) has been fixed.ProtobufHelperPlugindid that before).Side note: CEL-Java heavily depends on protobuf itself via its API and not just as a runtime/compile-only dependency. As long as no generated protobuf code is being used, it should be okay to use a different protobuf version at runtime, but there is and will be no guarantee.