dsmiley commented on code in PR #4612: URL: https://github.com/apache/solr/pull/4612#discussion_r3532815571
########## changelog/unreleased/dagp-dependency-analysis-PR4612.yml: ########## @@ -0,0 +1,7 @@ +title: Add dependency-analysis (build-health) reporting to the build, and apply its uncontroversial advice (unused-dependency removals and test-scope corrections) +type: changed Review Comment: Our documentation in 2 places define the meaning of the options for "type" -- and this is undoubtedly a "other" category change and not "changed". ########## changelog/unreleased/dagp-dependency-analysis-PR4612.yml: ########## @@ -0,0 +1,7 @@ +title: Add dependency-analysis (build-health) reporting to the build, and apply its uncontroversial advice (unused-dependency removals and test-scope corrections) +type: changed +authors: + - name: Serhiy Bzhezytskyy +links: +- name: PR#4612 + url: https://github.com/apache/solr/pull/4612 Review Comment: When we have JIRA issues, we favor the JIRA issue instead of PR. ########## solr/core/build.gradle: ########## @@ -83,31 +80,25 @@ dependencies { implementation libs.apache.lucene.sandbox implementation libs.apache.lucene.spatialextras implementation libs.apache.lucene.suggest - // Collections & lang utilities implementation libs.google.guava implementation libs.apache.commons.lang3 implementation libs.apache.commons.math3 implementation libs.commonsio.commonsio implementation libs.carrotsearch.hppc - - implementation(libs.benmanes.caffeine) {transitive = false} + api(libs.benmanes.caffeine) {transitive = false} Review Comment: a dubious one IMO ########## solr/core/build.gradle: ########## @@ -121,40 +112,37 @@ dependencies { testImplementation(libs.apache.curator.test, { exclude group: 'org.apache.zookeeper', module: 'zookeeper' }) - implementation(libs.apache.zookeeper.zookeeper, { + api(libs.apache.zookeeper.zookeeper, { exclude group: "org.apache.yetus", module: "audience-annotations" }) - implementation(libs.apache.zookeeper.jute) { + api(libs.apache.zookeeper.jute) { exclude group: 'org.apache.yetus', module: 'audience-annotations' } - testImplementation variantOf(libs.apache.zookeeper.zookeeper) {classifier 'tests'} // required for instantiating a Zookeeper server (for embedding ZK) runtimeOnly libs.xerial.snappy.java runtimeOnly libs.dropwizard.metrics.core - implementation(libs.jayway.jsonpath, { + api(libs.jayway.jsonpath, { Review Comment: woah?! why? ########## solr/core/build.gradle: ########## @@ -121,40 +112,37 @@ dependencies { testImplementation(libs.apache.curator.test, { exclude group: 'org.apache.zookeeper', module: 'zookeeper' }) - implementation(libs.apache.zookeeper.zookeeper, { + api(libs.apache.zookeeper.zookeeper, { exclude group: "org.apache.yetus", module: "audience-annotations" }) - implementation(libs.apache.zookeeper.jute) { + api(libs.apache.zookeeper.jute) { exclude group: 'org.apache.yetus', module: 'audience-annotations' } - testImplementation variantOf(libs.apache.zookeeper.zookeeper) {classifier 'tests'} // required for instantiating a Zookeeper server (for embedding ZK) runtimeOnly libs.xerial.snappy.java runtimeOnly libs.dropwizard.metrics.core - implementation(libs.jayway.jsonpath, { + api(libs.jayway.jsonpath, { exclude group: "net.minidev", module: "json-smart" }) // StatsComponents percentiles implementation libs.tdunning.tdigest - // OpenTelemetry api libs.opentelemetry.api implementation libs.opentelemetry.context - implementation(libs.opentelemetry.exporter.prometheus) { + api(libs.opentelemetry.exporter.prometheus) { Review Comment: ehh... not sure why a solr-core user requires this? ########## solr/solrj-jetty/build.gradle: ########## Review Comment: I was hoping the new mechanism would be able to remove these super-obvious testImplementation declarations across our modules. One or two might even have a snide remark comment. *of course* the tests depend on the module they are testing!! It was a deficiency of the previous system that we needed to state the obvious. ########## solr/core/build.gradle: ########## @@ -121,40 +112,37 @@ dependencies { testImplementation(libs.apache.curator.test, { exclude group: 'org.apache.zookeeper', module: 'zookeeper' }) - implementation(libs.apache.zookeeper.zookeeper, { + api(libs.apache.zookeeper.zookeeper, { exclude group: "org.apache.yetus", module: "audience-annotations" }) - implementation(libs.apache.zookeeper.jute) { + api(libs.apache.zookeeper.jute) { Review Comment: Oh? I thought this was only used for embedded ZK but maybe I'm wrong ########## gradle/validation/dependencies.gradle: ########## @@ -113,7 +113,8 @@ allprojects { // This may not be the case where this task is added as a dependency, like for "dependencies" task project.configurations.findAll { // Add any custom filtering on the configurations to be resolved - it.canBeResolved + it.canBeResolved && + !it.attributes.keySet().any {attr -> attr.name.startsWith("dagp.")} Review Comment: why? ########## gradle/validation/dependency-analyze.gradle: ########## Review Comment: not critical but there was some project lazy-consensus on embracing kotlin for the gradle build. When I recently created a new build file, I found the syntax helpful to avoid problems due to groovy's untyped nature. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
