-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Gradle modernization #1642
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
Gradle modernization #1642
Conversation
0144d6e
to
aeda926
Compare
1ca551a
to
d80ee10
Compare
5fa97ac
to
3f03dbd
Compare
Added library conventions and language based configurations. - Added `doclets` to `buildSrc` for the `javadocs` - Added `conventions` to centralize configuration of gradle plugins - Added `projects` to handle project level conventions JAVA-5758 JAVA-5757
Added library conventions and language based configurations. - Updated to Gradle 8 (JAVA-5756) - Removed `util:taglets` as now in `buildSrc`. - Moved `util:spock` into `driver-core` as the annotation is now shared via `testArtifacts`. - Added optional support (JAVA-5757) JAVA-5756 JAVA-5758 JAVA-5757
Added library conventions and language based configurations. - Refactored `JsonPoweredTestHelper` to handle either local and jar sources - Refactored test cases to use new `JsonPoweredTestHelper.getTestDocuments` method - Refactored test cases to normalize usage of loading test data - Moved scala integration tests into a standardized `integerationTest` directory JAVA-5758
Testing builds between
|
Env & task | main | JAVA-5758 | Findings | |
---|---|---|---|---|
latest Sharded Cluster Auth SSL JDK8 Linux test-core | 4718 | 4776 | ⬆️ | All tests for a class are now reported as skipped rather than the first, when @IgnoreIf is set on the class. Thanks Nathan for the investigation 🔎 |
latest Sharded Cluster Auth SSL JDK8 Linux test-legacy | 1033 | 1033 | ||
latest Sharded Cluster Auth SSL JDK8 Linux test-reactive | 3218 | 3218 | ||
latest Sharded Cluster Auth SSL JDK8 Linux test-sync | 4454 | 4460 | ⬆️ | |
Load Balancer latest Auth SSL JDK21 Ubuntu | 1826 | 1826 | ||
Scala 2.13 JDK17 7.0 Replica Set Ubuntu | 944 | 944 | ||
Kotlin: JDK17 7.0 Replica Set Ubuntu | 1314 | 1227 | ⬇️ |
Now Fixed - Kotlin extensions had the wrong junit library |
JDK8 Linux Unit test bson and crypt | 3813 | 3813 |
Source Main: Evergreen main build
Source PR: Evergreen patch build
Issues found and fixed by this testing
- Scala language dependencies updated to
api
fromimplementation
to keep the poms the same. slf4j
dependencies updated tooptionalAPI
fromoptionalImplementation
to keep poms the same.- bson-kotlinx dependencies for datetime and json serialization updated to
optionalAPI
fromoptionalImplementation
to keep poms the same. - Fixed various mainfest attributes as they differed from previous versions
- Fixed driver-kotlin-extensions testing and updated kotlin junit dependency in libs.versions.toml to use junit5.
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.
LGTM!
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 didn't the integration test directory name change require a corresponding change in Gradle scripts?
@jyemin static tests will now pass - a side effect of moving integration tests. Also removed the unneeded extra source directory for kotlin integration tests from the kotlin project. |
Ah, so it was looking in both places, so they were found in either location. Thanks. |
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.
LGTM
Since the validation of the correctness of changes were largely manual, please re-validate before final merge. :)
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.
Looks great and I learned a lot from the PR. Thanks.
} | ||
|
||
repositories { | ||
gradlePluginPortal() |
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 thread is only for discussion. Did you see scenario where some plugin could only be found in gradlePluginPortal()
, not in mavenCentral()
? I asked for I struggled on whehter I need to use gradlePluginPortal()
in mongo-hibernate
project as well.
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.
Just following the official recommendations. I imagine any third party plugins would be on maven but any gradle specific ones might not be.
// Add multiple versions with Scala suffixes for each Scala-related dependency. | ||
scalaVersions!!.forEach { scalaVersion -> | ||
if (scalaVersion != defaultScalaVersion) { | ||
// Replace scala version suffix |
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.
super minor. scala version
-> Scala version
? Don't wanna make a fuss about it. Feel free to ignore.
pom.withXml { | ||
val pomXml: Node = asNode() |
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.
It looks like the validation isn’t currently being executed, becausepom.withXml {}
runs during the configuration phase (before the POM file is written), placing it inside a doLast
block causes it to be skipped.
One alternative is to remove pom.WithXml
block and parse the generated POM file directly using XmlParser
, as we did previously:
val pomXml: Node = XmlParser().parse(destination)
The rest of the logic should not be affected after the change.
pom.withXml { | ||
val pomXml: Node = asNode() | ||
val dependenciesNode = pomXml.getNode("dependencyManagement").getNode("dependencies") | ||
assert(dependenciesNode!!.children().isNotEmpty()) { |
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.
These assertions only take effect when the JVM is run with the -ea flag, which could lead to silent failures. We could replace assert()
with require()
to ensure the validation always runs.
Added buildSrc, library conventions and language based configurations. - Updated to Gradle 8 (JAVA-5756) - Added libs.versions.toml (JAVA-5291) - Removed `util:taglets` as now in `buildSrc`. - Moved `util:spock` into `driver-core` as the annotation is now shared via `testArtifacts`. - Added optional support (JAVA-5757) Migrated tests to work with conventions: - Refactored `JsonPoweredTestHelper` to handle either local and jar based resources - Added test artifacts dependencies convention for use with test artifact jars. - Refactored test cases to use new `JsonPoweredTestHelper.getTestDocuments` method - Refactored test cases to normalize usage of loading test data - Moved scala / kotlin integration tests into a standardized `integerationTest` directory Updated github action and put version in `gradle.properties`. #1642 #1654 JAVA-5291 JAVA-5756 JAVA-5757 JAVA-5758
This PR comprises of 3 separate commits for the purpose of aiding reviewability.
The commits themselves aren't designed to be standalone (they are not) but rather compose separate areas of work that updating the build has required.