autonomousapps / dependency-analysis-gradle-plugin

Gradle plugin for JVM projects written in Java, Kotlin, Groovy, or Scala; and Android projects written in Java or Kotlin. Provides advice for managing dependencies and other applied plugins
Apache License 2.0
1.82k stars 121 forks source link

Dependency model should include classifier, and this must be printed in the advice #342

Open ColtonIdle opened 4 years ago

ColtonIdle commented 4 years ago

Plugin version 0.68.0

Gradle version 6.7

Android Gradle Plugin (AGP) version 4.1.1

Describe the bug Added a dependency of implementation "net.danlew:android.joda:2.10.7.2" and then running this plugin:

Transitively used dependencies that should be declared directly as indicated:
- implementation("joda-time:joda-time:2.10.7")

After adding it, my project doesn't build due to this issue:

Duplicate class org.joda.time.Chronology found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateMidnight found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateMidnight$Property found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateTime found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateTime$Property found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateTimeComparator found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateTimeConstants found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)
Duplicate class org.joda.time.DateTimeField found in modules jetified-joda-time-2.10-no-tzdb.7-no-tzdb (joda-time:joda-time:2.10.7) and jetified-joda-time-2.10.7 (joda-time:joda-time:2.10.7)

Not sure if this is really an issue, but I didn't know what was going on so I filed, just to be sure as it could help someone else out in the future

autonomousapps commented 4 years ago

That's really interesting. I wonder what unusual think the danlew library is doing. Will take a look!

autonomousapps commented 4 years ago

@ColtonIdle could you give me some more detail on reproducing this? If I follow the readme alone, and just initialize the library, that is not sufficient to trigger this issue. I assume you have to call actual methods on classes that are normally provided by the core joda-time library. So, provide some sample method calls? I have never used either of these libraries myself.

ColtonIdle commented 4 years ago

@autonomousapps The only thing I really call is in my custom App class onCreate() JodaTimeAndroid.init(this);

autonomousapps commented 4 years ago

But then you must use the library somehow? Just initing it isn't very useful. Which date-related methods are you calling?

autonomousapps commented 4 years ago

Got it. I can just add something like DateTime() and that's sufficient to trigger the issue.

autonomousapps commented 4 years ago

Found the issue. My plugin takes no account whatsoever of classifiers, and in this case it matters. If instead of

implementation("joda-time:joda-time:2.10.7")

you did

implementation("joda-time:joda-time:2.10.7:no-tzdb")

then there would be no issue building your app. Btw, I discovered this by looking at the POM file for the danlew library:

    <dependency>
      <groupId>joda-time</groupId>
      <artifactId>joda-time</artifactId>
      <version>2.10.7</version>
      <classifier>no-tzdb</classifier>
      <scope>compile</scope>
    </dependency>

You will note the classifier.

However, even when I fix that, the plugin will still advise you to add that transitive dependency, which in this case it really shouldn't. The best way to handle that at this time is to use a "dependency bundle". Tl;dr add this to your root build.gradle

dependencyAnalysis {
    dependencies {
        bundle('joda-time') {
            includeDependency('net.danlew:android.joda')
            includeDependency('joda-time:joda-time')
        }
    }
}

You can read more about that feature here.

ColtonIdle commented 4 years ago

Interesting. I'm fine with just disabling the check on this specific dependency. I figured it's something wonky that the joda-time libs have to do, but I just wanted to bring it up. =) Thanks for the quick response.

autonomousapps commented 3 years ago

It is in fact a fairly common pattern, but it's not well modeled by Gradle. So my dependency bundle feature is kind of a workaround. 

On December 4, 2020, GitHub notifications@github.com wrote:

Interesting. I'm fine with just disabling the check on this specific

dependency. I figured it's something wonky that the joda-time libs have to do, but I just wanted to bring it up. =) Thanks for the quick response.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/autonomousapps/dependency-analysis-android-gradle-

plugin/issues/342#issuecomment-739119095>, or unsubscribe https://github.com/notifications/unsubscribe- auth/ABJG5PNIYLVDSETRHNK7ES3STGV4PANCNFSM4UJXS4HA.

autonomousapps commented 3 years ago

Annoyingly, Gradle insists it would be impossible to expose the classifier of a dependency via some public API. I need to investigate more deeply, including looking at gradle-internal APIs.

jjohannes commented 1 year ago

We can do a little bit here after #854 is integrated.

If you ONLY have ONE dependency with a classifier to a component that you define yourself, we could preserve that for the advices. I started on this here: https://github.com/jjohannes/dependency-analysis-android-gradle-plugin/commit/a8a713aba243caad21cac55989b2891525a93bf5

However, that's not the original case, where the classifier is defined in external metadata.

We might be able to go a bit further, as the classifier has a fixed position in the name of the Jar file. And if you know the component name and version, you could extract it from the file name... if we think it's worth going down that path.

autonomousapps commented 1 year ago

if we think it's worth going down that path.

There are a few other cases where this comes up. If it's not too hard/weird of an implementation, I'd definitely be open to extending the model to include classifier information.

jjohannes commented 1 year ago

Newish issue related to this on the Gradle side: https://github.com/gradle/gradle/issues/23318