Closed sihutch closed 1 month ago
I think you're correct in expecting such cases to be handled by org.openrewrite.gradle.ChangeDependency
; I'd consider that something not yet implemented. You're welcome to help explore a fix, perhaps starting with a unit test on a draft PR similar to this test.
Thanks for getting back to me @timtebeek. I have created a PR with the failing test.
https://github.com/openrewrite/rewrite/pull/3665#issuecomment-1794948186
I'll share this oldie, but goodie reference. I envision a state that any of the Gradle dependency recipes work correctly over JVM test suites. As you can tell from the comment and what you've probably gathered from the recipes' behaviors, this is yet another location for dependencies to be declared and handled correctly.
This particular aspect will be made a little bit easier when the Trait
feature lands for Gradle dependencies, but in fair warning fixing this issue will be a challenging one for a beginner. If you're still up for it, we can absolutely assist you in your journey.
Thanks @shanman190. If someone could outline a high-level solution, along with any areas of potential difficulty, or point me to some specific areas to familiarize myself with, then I'd be happy to take a look and see if I can make progress.
So for this one, I'd start by debugging the existing recipes to see if the dependencies
block is being picked up as a DependencyHandlerSpec
. If it is then there might be something else going wrong with the recipe.
If that isn't the case, then I'd reuse something that I formulated for the MigrateGradleEnterpriseToDevelocity
recipe in so far as detecting where we are at in the buildscript. (See https://github.com/openrewrite/rewrite/blob/main/rewrite-gradle%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fgradle%2Fplugins%2FMigrateGradleEnterpriseToDevelocity.java#L204-L220). From there, it's then making sure to accept the new locations into the existing update or change code paths.
The Groovy parser a lot of the time will leave off the type information and rely upon its dynamic dispatch, so that's what makes the initial variable hard to tie down.
Thanks @shanman190. I will take a look
@shanman190 The issue was that the dependencies block is not being picked up as a DependencyHandlerSpec
.
I have updated my PR. This is not a complete solution, but was sufficient to make my failing test pass and to illustrate the problem.
For a more complete solution, I have a few questions
jvm-test-suite
plugin is applied. Is it acceptable to add this to the RewriteGradleProject
as I have done in my commit, or is there a better way to handle plugins?suite
with any name and then delegating that closure? For example, the test
method below could be any method name.interface SuitesSpec {
void test (@DelegatesTo(strategy=Closure.DELEGATE_ONLY, value=SuiteTestSpec) Closure cl)
}
Hi @sihutch,
That's unfortunate to hear that the method was not being type attributed as a DependencyHandlerSpec
, but neither is it terribly surprising either.
As far as more stable remediation, what I'd probably now pivot to is code based upon a new PR of mine found here which introduces the Gradle Trait
API for identifying dependencies.
I think the trait should receive one small update near the beginning of the block to confirm that the current method invocation is contained within a parent method invocation named dependencies
. With each of Gradle's usages, they have always placed dependencies within a block named dependencies
.
So envisioning something about like this:
Update the GradleTraitMatcher#test method
if (!isWithin("dependencies")) {
return null;
}
Within ChangeDependency make updates to no longer use the old style dependency test and instead use the GradleDependency.Matcher
trait matcher.
GradleDependency.Matcher gradleDependencyMatcher = new GradleDependency.Matcher();
if (!matcher.get(getCursor()).isPresent()) {
// This method invocation is not an add dependency invocation
return m;
}
@shanman190 - Cool. That's a much better solution
@shanman190 I have picked this issue up again as this PR https://github.com/openrewrite/rewrite/pull/4354 seems to have stalled, and we need this functionality.
I have implemented the trait approach from above, and it works; however, I am getting a single failing test in 'ChangeDependencyTest`
This test is failing. I have debugged a bit, and it is because the nameToConfiguration
map in the Gradle
project is empty. This test differs from the others as the dependencies are within the build script. Where should I look to try to solve this?
@sihutch, alright, the issue with that test is that we are grabbing the configurations from the project, but the classpath
configuration is associated to the buildscript itself. Like in other places, we should probably just use one off handling for this configuration in particular (old style MethodMatcher
on DependencyHandlerSpec
). In the future, we may look into how to get this configuration into the GradleProject
marker as well and can remove the special handling in the future.
Closing as fixed by https://github.com/openrewrite/rewrite/pull/4376
I am using
How are you running OpenRewrite?
I am running OpenRewrite using the Gradle Plugin, and my project is a single module project.
My build.gradle has the same dependency defined in the main dependencies and also with a jvm-test-suites section as shown below
When I run either of the two recipes shown below only the
javax.servlet:javax.servlet-api:3.1.0
in the main dependencies section is changed.org.openrewrite.java.dependencies.ChangeDependency
org.openrewrite.gradle.ChangeDependency
I have created an example project here
I was expecting the dependencies in the jvm-test-suite block to also be changed. Can you please confirm whether this is a bug or something that is not implemented ? If that later, is there another recipe that you can recommend that can handle the jvm-test-suite case ?
Thanks
Si