-
Notifications
You must be signed in to change notification settings - Fork 843
SOLR-18302: DAGP inter-module project() api promotions (part 2/3, draft) #4613
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
Closed
serhiy-bzhezytskyy
wants to merge
7
commits into
apache:main
from
serhiy-bzhezytskyy:dagp-pr2-project-api
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e933003
SOLR-18296: add dependency-analysis (DAGP) reporting + apply safe advice
serhiy-bzhezytskyy 03a76c4
SOLR-18296: add changelog entry for dependency-analysis PR (#4612)
serhiy-bzhezytskyy 77e8ffa
SOLR-18296: restore kafka-clients test jar as testRuntimeOnly (fix cr…
serhiy-bzhezytskyy fa39a80
SOLR-18302: address review — revert api promotions, drop change-comme…
serhiy-bzhezytskyy d0c3fde
SOLR-18302: drop redundant self/duplicate test project deps in solrj-…
serhiy-bzhezytskyy 6306c81
SOLR-18302: address review — kotlin build script, drop self-test deps…
serhiy-bzhezytskyy fa4f0f0
SOLR-18296: promote genuinely-public inter-module deps to api (part 2/3)
serhiy-bzhezytskyy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
|
|
||
| title: > | ||
| Add dependency-analysis (build-health) reporting to the Gradle build in report-not-fail mode, | ||
| and apply its safe advice (unused-dependency removals and test-scope corrections) | ||
| type: other | ||
| authors: | ||
| - name: Serhiy Bzhezytskyy | ||
| links: | ||
| - name: SOLR-18302 | ||
| url: https://issues.apache.org/jira/browse/SOLR-18302 |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import com.autonomousapps.DependencyAnalysisExtension | ||
|
|
||
| configure<DependencyAnalysisExtension> { | ||
| issues { | ||
| project(":solr:ui") { | ||
| onUnusedDependencies { | ||
| severity("ignore") | ||
| } | ||
| onUsedTransitiveDependencies { | ||
| severity("ignore") | ||
| } | ||
| onIncorrectConfiguration { | ||
| severity("ignore") | ||
| } | ||
| } | ||
|
|
||
| project(":solr:solrj-jetty") { | ||
| onUnusedDependencies { | ||
| exclude(":solr:solrj") | ||
| } | ||
| } | ||
|
|
||
| project(":solr:modules:jwt-auth") { | ||
| onDuplicateClassWarnings { | ||
| severity("ignore") | ||
| } | ||
| } | ||
|
|
||
| all { | ||
| // Report advice as warnings rather than failing the build: several categories | ||
| // (api-vs-implementation promotions, some test-scope changes) need case-by-case | ||
| // human judgment. Run `gradlew buildHealth` to see the current advice. | ||
| onAny { | ||
| severity("warn") | ||
| } | ||
| onUnusedDependencies { | ||
| exclude( | ||
| "org.jspecify:jspecify", | ||
| "com.google.code.findbugs:jsr305", | ||
| "com.google.errorprone:error_prone_annotations", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| tasks.named("check").configure { | ||
| dependsOn(tasks.named("buildHealth")) | ||
| } |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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's confusing to me why solrj is listed here at all (whatever configuration scope). solr-core has an "api" config dependency on solrj... thus this means solrJ is exposed at equivalent scope to whoever consumed solr-core. The subproject here is depending on solr-core... so then why also mention solrj at all?
What I'm talking about is probably tangential to this PR because you didn't change the scopes I'm talking about. Note that many of our dependency declarations are redundant due to cutterslade not understanding the nuance of gradle's "api". I'm hoping we can remove these redundant declarations and maybe the dagp thing you are adding could help us identify redundancies?
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.
Good question, and it's actually the opposite of redundant here — cross-dc-manager uses solrj directly (CloudSolrClient, UpdateRequest, SolrRequest, etc. in ~4 files), so declaring it is the right call rather than leaning on the transitive
apifrom core. The Gradle guidance is "declare what you use directly," so you don't break when core's internals shift.And yeah — DAGP is exactly the tool for what you're after. It flags both directions: unused declarations you can drop, and transitives you use directly but haven't declared. For this one it agrees with keeping solrj (doesn't flag it for removal). So it should help find the genuinely-redundant cutterslade leftovers without touching the legit direct declarations.
Happy to do a pass on the redundant ones as a follow-up if useful.
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 agree there's a judgement call to make a dependency "api" instead of "implementation"; we're not doing it willy-nilly or simply because some tool found one public API signature using the dependency. If we are diligent about this (I think we are), then why not rely on this judgement on the consuming end to not be redundant? Otherwise, what's the point of "api" at all if we don't take advantage of it.
Case in point: There's no way in hell that SolrJ will stop being a dependency of Solr-core. Any consumer of solr-core will consume solrJ. Even if for some other example where it'a "api" today but we change our mind... well so what? Consumer will find out (won't compile) and add the dependency explicitly then.
Uh oh!
There was an error while loading. Please reload this page.
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.
Strong agree — this is a human call, not the tool's. dagp wouldn't flag it for removal (it'd keep solrj — cross-dc uses it directly). I can do a manual pass and put up a PR — WDYT?
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.
Thanks. I think I'd prefer that you iterate on the part1 PR; no need for additional ones IMO.
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.
Sounds good — will iterate on #4612.