Import X-Ray Agent Plugin project #52
Conversation
|
Travis is failing because it's still testing the old, maven-based project. When Disco and 2.7.0 of the SDK are in maven central, I'll switch travis over to the new project. |
| dependencies { | ||
| implementation("com.amazonaws:aws-xray-recorder-sdk-aws-sdk") | ||
| implementation("software.amazon.disco:disco-java-agent-api") | ||
| implementation(project(":aws-xray-agent")) |
There was a problem hiding this comment.
Would put project dependencies above non-project
| testImplementation("org.powermock:powermock-module-junit4:2.0.7") | ||
| } | ||
|
|
||
| description = "AWS X-Ray Agent AWS SDK V1 auto-instrumentation library" |
There was a problem hiding this comment.
Would put description at top, just below plugins block
build.gradle.kts
Outdated
| } | ||
| named<ShadowJar>("shadowJar") { | ||
| // Suppress the "-all" suffix on the jar name, simply replace the default built jar instead | ||
| archiveClassifier.set(null as String?) |
There was a problem hiding this comment.
| archiveClassifier.set(null as String?) | |
| archiveClassifier.set("") |
build.gradle.kts
Outdated
| // Suppress the "-all" suffix on the jar name, simply replace the default built jar instead | ||
| archiveClassifier.set(null as String?) | ||
|
|
||
| dependsOn(named<Jar>("jar")) |
There was a problem hiding this comment.
Do we need to even run jar task? For reference, here's my pattern https://github.com/anuraaga/aws-opentelemetry-java-instrumentation/blob/master/otelagent/build.gradle.kts#L62
There was a problem hiding this comment.
I like your way better :) will change.
| @@ -0,0 +1,7 @@ | |||
| rootProject.name = "com.amazonaws.xray.agent" | |||
|
|
|||
| include("aws-xray-agent") | |||
There was a problem hiding this comment.
We should avoid having projects with subprojects, it's easy to mess up Gradle's caching if files in the subproject change. Maybe just have aws-xray-agent-plugin at the top level
There was a problem hiding this comment.
I was copying the Disco project structure, e.g. https://github.com/awslabs/disco/tree/master/disco-java-agent-sql
But I suppose that's more applicable since they have several library-plugin subprojects. I think our repo could be flat without issue.
build.gradle.kts
Outdated
| // Otherwise, just publish the standard JAR | ||
| plugins.withId("java") { | ||
| if (plugins.hasPlugin("com.github.johnrengelman.shadow")) { | ||
| artifact(tasks.named<Jar>("jar").get()) |
There was a problem hiding this comment.
Is this the shaded artifact?
There was a problem hiding this comment.
Yes, although I think this actually might have to be wrapped in afterExecute for the hasPlugin to work. I'll add that.
There was a problem hiding this comment.
Was there something you noticed awry?
There was a problem hiding this comment.
Ah sorry should have been explicit - it's referring to "jar" task, not "shadowJar" task
There was a problem hiding this comment.
Instead of afterEvaluate, you can just use plugins.WithId("com.github.johnrengelman.shadow")
There was a problem hiding this comment.
Yeah I thought about this but I couldn't figure out a way to get gradle to behave differently if a plugin wasn't present. E.g. something like:
plugins.withId("shadow") {
artifact()
}
plugins.withoutId("shadow") {
from(components["java"])
}The afterEvaluate + hasPlugin approach worked, but let me know if there's a preferable withId way instead.
build.gradle.kts
Outdated
| // Otherwise, just publish the standard JAR | ||
| plugins.withId("java") { | ||
| afterEvaluate { | ||
| if (plugins.hasPlugin("com.github.johnrengelman.shadow")) { |
There was a problem hiding this comment.
Ah I think since we replace the jar (disable default task, use the empty classifier), actually components["java"] picks up the shadow jar. Appears to be working here
https://github.com/anuraaga/aws-opentelemetry-java-instrumentation/blob/master/build.gradle.kts#L111
Feels a bit hacky admittedly, but it makes sure the doc / source jars are published too which I think is requirement of maven central. With this approach we need to make sure to add those
There was a problem hiding this comment.
Oh nice that worked for me too! Added the simplification.
Description of changes:
gradle clean buildThis leaves the repo in an awkward, duplicated state, and I plan on removing all the legacy Maven subprojects in the near future. I'll also update travis later, the Gradle project isn't very testable by CI until Disco is released to maven central. Just didn't want to make this PR more bloated than it already is.
Might not be in the incredibly near future, but I plan on adding license, checkstyle, null checker, etc to keep parity with the SDK standards.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.