build: remove needless permitTestUnusedDeclared #4048
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,3 +255,28 @@ file as a URL under a <license> tag if there is no reference to a repository | |
| in Maven Central, or in the artifact downloaded by maven when the library | ||
| is added as a dependency (in IntelliJ IDEA the libraries can be found | ||
| in the project view under External Libraries at the bottom). | ||
|
|
||
| Gradle analyzeDependencies and analyzeTestDependencies | ||
| ----------------------------------------- | ||
|
|
||
| We use the "cutterslade.analyze" build plugin to help maintain good dependency hygiene. | ||
| It tries to identify when a dependency is needlessly declared, and then fail the build to complain. | ||
| Unfortunately, it also requires us to add declarations that Gradle would otherwise deem unnecessary, | ||
|
Contributor
Author
There was a problem hiding this comment. @risdenk I saw a notification about a comment that I can't find in GitHub. You said:
How does an "api dependency" remove a dependency? RE "api": An illustrative example would be There's a spectrum of developer prerogative from conservative to liberal, in terms of use of "api". I say "likely to use in conjunction with" but another person's definition might simply be the mere existence of a public signature that includes the target lib. I've increasingly preferred to use it conservatively. At work it's used liberally in a mono-repo and the extensive dependency web that doing so creates is out of control.
Contributor
Author
There was a problem hiding this comment. |
||
| since the plugin isn't aware of the semantics of "api" scope. | ||
|
|
||
| Since the plugin is imperfect, there are rare scenarios where we need to use one of its many special | ||
| gradle configurations, such as permitUnusedDeclared and permitTestUnusedDeclared. | ||
| Please avoid them by finding alternatives (if possible) -- get peer review. | ||
|
|
||
| For example, did you declare something as implementation (or testImplementation) but it's not needed at compile time? | ||
| Remove it. | ||
| If it's needed at runtime, then add as runtimeOnly (or testRuntimeOnly), *if* it otherwise | ||
| doesn't come transitively as such. | ||
| Let passing tests be your guide. | ||
| Try to be minimalist, omitting dependency declarations unless a failing build requires that you add them. | ||
|
|
||
| At each iteration of experimentation doing any dependency change in a build, write the gradle dependency locks. | ||
| *Then* run checks or otherwise evaluate the results. | ||
| A failure due to dependency locks is generally a failure to remember to write locks; it usually doesn't mean your build edits are flawed. | ||
|
|
||
| For more info: https://github.com/gradle-dependency-analyze/gradle-dependency-analyze | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,9 +38,9 @@ dependencies { | |
|
|
||
| // For 'tikaserver' backend | ||
| implementation libs.eclipse.jetty.client | ||
| implementation libs.eclipse.jetty.http | ||
| implementation libs.eclipse.jetty.util | ||
| implementation libs.eclipse.jetty.io | ||
|
Comment on lines
-41
to
+43
Contributor
Author
There was a problem hiding this comment. @janhoy were you trying to avoid simply using
Contributor
There was a problem hiding this comment. Strange. I must have added |
||
|
|
||
| testImplementation project(':solr:test-framework') | ||
| testImplementation libs.apache.lucene.testframework | ||
|
|
||
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.
@risdenk please review this; I'd love to have your input
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 looks great. I went down this rabbit hole for a bit too the other day. I didn't make progress. I went through and ripped out all the "permit" and ended up with test failures that I didn't have time to debug. I agree with what you are trying to do. We should not have permit* anywhere for the most part. There may be a few cases where its needed but it should be very rare.