-
Notifications
You must be signed in to change notification settings - Fork 1.7k
WIP: AVRO-4223 Gradle plugin for generating Java code #3614
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
base: main
Are you sure you want to change the base?
Conversation
Fix Intellij not finding generated sources Add generating BigDecimal property
Add dynamic output directory
|
Avro gradle plugin Protocol support has been added in this release |
|
@raphaelauv did you test the latest release https://plugins.gradle.org/plugin/eu.eventloopsoftware.avro-gradle-plugin? No need to add |
lang/java/gradle-plugin/README.md
Outdated
|
|
||
| ```kotlin | ||
| avro { | ||
| sourceZipFiles = listOf("/home/user/.gradle/caches/modules-2/files-2.1/eu.eventloopsoftware.group-id/artifact-id/1.0.0/92ac3d0533de9dd79ac35373c892ebaa01763d4d/jar_with_schemas-1.0.0.jar") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, we probably want to add some pre-configured configuration, called avroSources or something that way people can add their dependencies as normal, like
dependencies {
avroSources("eu.eventloopsoftware.group-id:artifact-id:1.0.0") // ...Certain plugins ship their own dependencies block (like in Gradle's test suites plugin) so perhaps something like
avro {
sourceZipFiles {
dependencies {
avroSources("eu.eventloopsoftware.group-id:artifact-id:1.0.0") // ...
could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is possible to add the compile- & runtime dependencies while generating code? Then we wouldn't need any configuration at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@opwvhk I'm not quite following? I think the dependencies here was just to allow people to add avro sources that are zipped in jars deployed to a standard maven repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, if we at least made sourceZipFiles a ConfigurableFileCollection, then users could configure either via files, or a their own custom configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is yet another marked improvement. Thank you!
I have a few open comments, and we'll want to fix the build, but otherwise I'm happy with the result.
| <executable>./gradlew</executable> | ||
| <arguments> | ||
| <argument>assemble</argument> | ||
| <argument>-i</argument> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass the Maven log level here? I doubt it, but it would be convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which Maven log level? The Maven log level can be passed when this Maven command is run, which in turn runs this Gradle command: ./gradlew assemble -i. Passing a Maven argument here would be on the wrong level, one too deep.
...va/gradle-plugin/src/main/kotlin/eu/eventloopsoftware/avro/gradle/plugin/AvroGradlePlugin.kt
Outdated
Show resolved
Hide resolved
|
I have created https://issues.apache.org/jira/browse/INFRA-27616 for the requirement from Gradle to prove the ownership of avro.apache.org DNS domain. |
| project, | ||
| extension, | ||
| includesProtocol, | ||
| extension.testSourceDirectory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| extension.testSourceDirectory, | |
| extension.sourceDirectory, |
| private fun instantiateAdditionalVelocityTools(velocityToolsClassesNames: List<String>): List<Any> { | ||
| return velocityToolsClassesNames.map { velocityToolClassName -> | ||
| try { | ||
| Class.forName(velocityToolClassName).getDeclaredConstructor().newInstance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this uses the current class' loader ?
loadLogicalTypesFactories() and doCompile() use the thread's class loader
|
|
||
| `0.0.8` | ||
|
|
||
| Add `sourceZipFiles` property to add zip files with schemas in them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence seems unfinished
| val testSourceDirectory: Property<String> = objects.property(String::class.java).convention("src/test/avro") | ||
|
|
||
| /** | ||
| * @parameter property="outputDirectory" default-value="${project.layout.buildDirectory}/generated-test-sources/avro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Javadoc seems like a copy/paste from Maven
|
|
||
| private fun compileSchemas(schemaFileTree: ConfigurableFileCollection, outputDirectory: File) { | ||
| val sourceFileForModificationDetection: File? = | ||
| schemaFileTree.asFileTree.files.filter { file: File -> file.lastModified() > 0 }.maxBy { it.lastModified() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| schemaFileTree.asFileTree.files.filter { file: File -> file.lastModified() > 0 }.maxBy { it.lastModified() } | |
| schemaFileTree.asFileTree.files.filter { file: File -> file.lastModified() > 0 }.maxByOrNull { it.lastModified() } |
to avoid NoSuchElementException if there are no files to filter
| try { | ||
| Class.forName(velocityToolClassName).getDeclaredConstructor().newInstance() | ||
| } catch (e: Exception) { | ||
| throw RuntimeException(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw RuntimeException(e) | |
| throw org.gradle.api.GradleException(e) |
| compileSchemaTask.schemaFiles.from( | ||
| project.fileTree(sourceDirectory).apply { | ||
| setIncludes(includes) | ||
| setExcludes(extension.excludes.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addSchemaFiles() is also used for tests, so this should use setTestExcludes(extension.testExcludes.get()) in that cases
| compileSchemaTask.protocolFiles.from( | ||
| project.fileTree(sourceDirectory).apply { | ||
| setIncludes(includesProtocol) | ||
| setExcludes(extension.excludes.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - testExcludes
| </execution> | ||
| <execution> | ||
| <id>run-gradle-task-publish</id> | ||
| <phase>deploy</phase> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use deploy to deploy -SNAPSHOTs for the Maven artefacts.
AFAIK Gradle plugins repo does not allow -SNAPSHOTs
| */ | ||
|
|
||
| plugins { | ||
| kotlin("jvm") version "2.2.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 2.3.0 ?
Below it uses org.jetbrains.kotlin:kotlin-gradle-plugin-api:2.3.0
What is the purpose of the change
Gradle plugin to generate Java code from Avro files
Verifying this change
This change added tests and can be verified as follows:
cd to
avro/lang/java/gradle-plugin./gradlew testDocumentation
Release
0.0.2is released and fully works with AVSC files: https://central.sonatype.com/artifact/eu.eventloopsoftware.avro-gradle-plugin/eu.eventloopsoftware.avro-gradle-plugin.gradle.plugin/versions - read docs (above) how to use0.0.5https://plugins.gradle.org/plugin/eu.eventloopsoftware.avro-gradle-plugin0.0.8https://plugins.gradle.org/plugin/eu.eventloopsoftware.avro-gradle-plugin0.1.0https://plugins.gradle.org/plugin/eu.eventloopsoftware.avro-gradle-plugin - this release adds Protocol support.Protocol support has been added in this release
Installation instructions: https://github.com/frevib/avro/blob/AVRO-4223-gradle-plugin/lang/java/gradle-plugin/README.md#version
An official release will be done in the coming month