dependency-analysis-gradle-plugin icon indicating copy to clipboard operation
dependency-analysis-gradle-plugin copied to clipboard

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

Open ColtonIdle opened this issue 3 years ago • 12 comments

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

ColtonIdle avatar Dec 02 '20 00:12 ColtonIdle

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

autonomousapps avatar Dec 02 '20 16:12 autonomousapps

@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.

autonomousapps avatar Dec 04 '20 04:12 autonomousapps

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

ColtonIdle avatar Dec 04 '20 05:12 ColtonIdle

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

autonomousapps avatar Dec 04 '20 08:12 autonomousapps

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

autonomousapps avatar Dec 04 '20 23:12 autonomousapps

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.

autonomousapps avatar Dec 04 '20 23:12 autonomousapps

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.

ColtonIdle avatar Dec 05 '20 03:12 ColtonIdle

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 [email protected] 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 avatar Dec 05 '20 06:12 autonomousapps

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.

autonomousapps avatar Dec 11 '20 05:12 autonomousapps

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.

jjohannes avatar Feb 14 '23 16:02 jjohannes

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.

autonomousapps avatar Feb 14 '23 18:02 autonomousapps

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

jjohannes avatar Mar 24 '23 15:03 jjohannes