Closed Alison-Soldado closed 4 years ago
google-oss-bot
commented
4 years ago I found a few problems with this issue:
jhansche
commented
4 years ago This issue also impacts firebase-crashlytics, as there are some dependencies that pull in com.google.dagger:dagger:2.27. So if an app is using a specific version of Dagger, Firebase is causing those versions to conflict (and may force an upgrade compared to the version the app wanted to use, which could create a compatibility issue):
+--- com.google.firebase:firebase-crashlytics:17.0.0
| +--- com.google.android.datatransport:transport-api:2.2.0
| +--- com.google.android.datatransport:transport-backend-cct:2.2.1
| | +--- androidx.annotation:annotation:1.1.0
| | +--- com.google.android.datatransport:transport-api:2.2.0
| | +--- com.google.android.datatransport:transport-runtime:2.2.1 <---
| | | +--- androidx.annotation:annotation:1.1.0
| | | +--- com.google.android.datatransport:transport-api:2.2.0
| | | \--- com.google.dagger:dagger:2.27 <---
| | | \--- javax.inject:javax.inject:1Given how widely Firebase is used, it seems to me like this datatransport:transport-runtime library should be using a repackaged/shadowed version of Dagger runtime, in order to avoid conflicts with the host application.
ashwinraghav
commented
4 years ago This issue also impacts
firebase-crashlytics, as there are some dependencies that pull incom.google.dagger:dagger:2.27. So if an app is using a specific version of Dagger, Firebase is causing those versions to conflict (and may force an upgrade compared to the version the app wanted to use, which could create a compatibility issue):+--- com.google.firebase:firebase-crashlytics:17.0.0 | +--- com.google.android.datatransport:transport-api:2.2.0 | +--- com.google.android.datatransport:transport-backend-cct:2.2.1 | | +--- androidx.annotation:annotation:1.1.0 | | +--- com.google.android.datatransport:transport-api:2.2.0 | | +--- com.google.android.datatransport:transport-runtime:2.2.1 <--- | | | +--- androidx.annotation:annotation:1.1.0 | | | +--- com.google.android.datatransport:transport-api:2.2.0 | | | \--- com.google.dagger:dagger:2.27 <--- | | | \--- javax.inject:javax.inject:1Given how widely Firebase is used, it seems to me like this
datatransport:transport-runtimelibrary should be using a repackaged/shadowed version of Dagger runtime, in order to avoid conflicts with the host application.
Unfortunately, the best we can help with on this front is to point you to ways to work around this situation.
See: https://github.com/firebase/firebase-android-sdk/issues/1643#issuecomment-642963147
ashwinraghav
commented
4 years ago [REQUIRED] Step 2: Describe your environment
- Android Studio version: 4.0
- Firebase Component: In-App In-App Messaging
- Component version: 19.0.7
[REQUIRED] Step 3: Describe the problem
Steps to reproduce:
After added the dependency in-app messaging cannot find symbol DaggerAppComponent
Relevant Code:
build.gradle(:app)
implementation 'androidx.multidex:multidex:2.0.1' implementation 'androidx.appcompat:appcompat:1.1.0' implementation 'androidx.legacy:legacy-support-v4:1.0.0' implementation 'androidx.constraintlayout:constraintlayout:1.1.3' implementation 'com.google.android.material:material:1.1.0' implementation 'androidx.recyclerview:recyclerview:1.1.0' implementation 'androidx.cardview:cardview:1.0.0' implementation 'androidx.gridlayout:gridlayout:1.0.0' implementation 'com.facebook.stetho:stetho:1.5.0' implementation 'com.facebook.stetho:stetho-okhttp3:1.5.0' implementation 'com.facebook.stetho:stetho-js-rhino:1.5.0' implementation 'com.jakewharton.timber:timber:4.5.1' implementation 'org.apache.commons:commons-lang3:3.6' implementation 'com.github.bumptech.glide:glide:4.0.0' implementation "com.github.barteksc:android-pdf-viewer:2.8.2" implementation 'com.github.bumptech.glide:okhttp3-integration:1.4.0@aar' testImplementation 'junit:junit:4.12' testImplementation "org.mockito:mockito-core:1.10.19" testImplementation 'com.squareup.retrofit2:retrofit-mock:2.3.0' androidTestImplementation('androidx.test.espresso:espresso-core:3.1.0', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation('androidx.test.espresso:espresso-intents:3.1.0', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation('androidx.test.espresso:espresso-web:3.1.0', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation('androidx.test.espresso:espresso-contrib:3.1.0', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' exclude group: 'com.android.support' }) androidTestImplementation('androidx.test.ext:junit:1.1.1', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation('androidx.test:rules:1.1.1', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation('tools.fastlane:screengrab:1.0.0', { exclude group: 'com.android.support', module: 'support-annotations' exclude group: 'com.google.code.findbugs' }) androidTestImplementation 'com.squareup.okhttp3:mockwebserver:4.2.1' implementation 'com.squareup.retrofit2:retrofit:2.6.2' implementation 'com.squareup.retrofit2:converter-gson:2.6.1' implementation 'com.squareup.retrofit2:converter-scalars:2.3.0' implementation 'com.squareup.okhttp3:logging-interceptor:4.2.1' implementation 'net.zetetic:android-database-sqlcipher:3.5.6@aar' implementation "com.google.android.gms:play-services-tasks:17.1.0" implementation 'uk.co.chrisjenx:calligraphy:2.2.0' implementation "com.google.dagger:dagger-android:2.17" implementation "com.google.dagger:dagger:2.17" implementation "com.google.dagger:dagger-android-support:2.17" annotationProcessor "com.google.dagger:dagger-android-processor:2.17" implementation "com.google.firebase:firebase-core:17.4.3" implementation "com.google.android.gms:play-services-analytics:17.0.0" implementation "com.google.android.gms:play-services-tagmanager:17.0.0" implementation "com.google.firebase:firebase-perf:19.0.7" implementation "com.google.firebase:firebase-messaging:20.2.0" implementation "com.google.firebase:firebase-inappmessaging:19.0.7" implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.3.72" implementation('com.crashlytics.sdk.android:crashlytics:2.10.1@aar') { transitive = true }build.gradle
classpath 'com.android.tools.build:gradle:4.0.0' classpath 'io.fabric.tools:gradle:1.31.2' classpath 'com.github.gfx.ribbonizer:ribbonizer-plugin:2.1.0' classpath 'org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:2.6.1' classpath 'com.google.gms:google-services:4.3.3' classpath 'com.google.firebase:perf-plugin:1.3.1' classpath 'com.github.timfreiheit:ResourcePlaceholdersPlugin:0.2' classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:1.3.72"
I can't tell what the 'conflict' you are seeing is. If you are referring to the same problem that @jhansche is referring to, the suggested workarounds are covered here https://github.com/firebase/firebase-android-sdk/issues/1677#issuecomment-645583794.
Please reopen if that's not what you are referring to
jhansche
commented
4 years ago Unfortunately, the best we can help with on this front is to point you to ways to work around this situation.
See: #1643 (comment)
I don't think this is the right solution. See the linked Dagger comment thread, where Dagger's stance is made more clear (multiple times), that third party libraries should be using a shaded version of Dagger, so that their choices don't collide with anyone else's: https://github.com/google/dagger/pull/950#issuecomment-360270413 https://github.com/google/dagger/pull/950#issuecomment-439026362
jhansche
commented
4 years ago I can't tell what the 'conflict' you are seeing is.
If I'm understanding correctly, the issue @Alison-Soldado is describing is the same as what I described:
implementation "com.google.dagger:dagger-android:2.17"
implementation "com.google.dagger:dagger:2.17"
implementation "com.google.dagger:dagger-android-support:2.17"
annotationProcessor "com.google.dagger:dagger-android-processor:2.17"
...
implementation "com.google.firebase:firebase-inappmessaging:19.0.7"^ Their app depends on Dagger v2.17, but Firebase's transitive dependencies are causing an unexpected forced version bump during Gradle's dependency version conflict resolution, such that their 2.17 dependency automatically gets bumped to 2.27 out from under them and with no warning. That creates a compatibility issue, similar to those mentioned in google/dagger#950.
It's also different from #1643 which is about Dagger1 vs Dagger2, and the main problem there is because of the @Inject annotations, especially when you want to inject the same class/method/constructor with 2 different dagger graphs from dagger1 and dagger2. Whereas in this case, it's a conflict of different versions of the same Dagger2 artifact.
Especially when the transitive dependency only forces one of the dependencies to get updated, while the others all remain on the older 2.17 version.
ashwinraghav
commented
4 years ago Unfortunately, the best we can help with on this front is to point you to ways to work around this situation.
See: #1643 (comment)
I don't think this is the right solution. See the linked Dagger comment thread, where Dagger's stance is made more clear (multiple times), that third party libraries should be using a shaded version of Dagger, so that their choices don't collide with anyone else's: https://github.com/google/dagger/pull/950#issuecomment-360270413 https://github.com/google/dagger/pull/950#issuecomment-439026362
@jhansche Shading is the solution I have suggested as well, except I recommend that apps do it while importing the library and it's dependencies.
Libraries shipping with shaded dagger and vendoring a copy will optimize for consumers that are on the older versions of dagger. It would lead to customers who are on newer versions having to take on unnecessary footprint. That's the reason we are leaving the shadowing up to customers.
Can you show what the challenges are migrating your code up to the new version? That would would help us prioritize this a little better.
jhansche
commented
4 years ago Can you show what the challenges are migrating your code up to the new version?
I think it's less about our own use of it, and more the fact that this unexpectedly influences the dependency graph of every app that uses it (and Firebase is very popular, so that includes a lot of apps). I agree that updating Dagger is usually painless, but an app can only do that if they know it is a requirement.
What's worse, is Gradle will not complain about this scenario, and the code will compile, but it might crash at runtime.
If every third party library operated like this, then multiple libraries may be using competing versions, and given that there are binary incompatibility issues that have come up in multiple versions of Dagger, that could mean that an app having 2 different libraries that each want a different version of Dagger (whether or not the app is able to update their version to match, which I do agree is normally a simple task) could be put in a situation that's impossible to resolve.
Libraries shipping with shaded dagger and vendoring a copy will optimize for consumers that are on the older versions of dagger. It would lead to customers who are on newer versions having to take on unnecessary footprint.
Actually, I would argue that it's optimizing for the case of all apps and libraries that reference this datatransport library to never have to worry about this problem again. That, in my opinion, far outweighs any arguments of "older vs newer."
The Dagger runtime is relatively small (~33KB jar, 53KB uncompressed), but the runtime artifact is only guaranteed to work if the generated code was generated with the same version of the dagger compiler, as noted in the comments above from the Dagger team. When a library like this influences the dependency graph, it's only going to coerce the runtime artifact to match the library's runtime, but will not automatically bump the build-time compiler/spi/provider artifact. In the OP's case, that means their app will have code generated with the 2.17 Dagger compiler, while Firebase forces a bump of Dagger runtime to 2.27, and those are not compatible.
Shading is the solution I have suggested as well, except I recommend that apps do it while importing the library and it's dependencies.
But the Dagger team (@ronshapiro) recommends the opposite 😞
ashwinraghav
commented
4 years ago Can you show what the challenges are migrating your code up to the new version?
I think it's less about our own use of it, and more the fact that this unexpectedly influences the dependency graph of every app that uses it (and Firebase is very popular, so that includes a lot of apps). I agree that updating Dagger is usually painless, but an app can only do that if they know it is a requirement.
Fair!
What's worse, is Gradle will not complain about this scenario, and the code will compile, but it might crash at runtime.
If every third party library operated like this, then multiple libraries may be using competing versions, and given that there are binary incompatibility issues that have come up in multiple versions of Dagger, that could mean that an app having 2 different libraries that each want a different version of Dagger (whether or not the app is able to update their version to match, which I do agree is normally a simple task) could be put in a situation that's impossible to resolve.
Thanks for raising. I did not realize that this would fail only at runtime. Is there a crash you can share ?
Libraries shipping with shaded dagger and vendoring a copy will optimize for consumers that are on the older versions of dagger. It would lead to customers who are on newer versions having to take on unnecessary footprint.
Actually, I would argue that it's optimizing for the case of all apps and libraries that reference this datatransport library to never have to worry about this problem again. That, in my opinion, far outweighs any arguments of "older vs newer."
I'll share why this is a problem. The data transport libraries are used by some of the most widely adopted Firebase libraries: Firebase Messaging and Firebase Crashlytics. Other libs that use it include In App Messaging and a variety of other non Firebase libraries. Each of these would have to ship with their own copies of the shaded runtime, and the costs add up quickly particularly on lower end Android devices.
Alternatively they could share a release cadence that allows them to share a copy. That's sub optimal for maintainability. Not to mention, it would force you to upgrade all or nothing, a world we have lived in previously.
Shading is the solution I have suggested as well, except I recommend that apps do it while importing the library and it's dependencies.
But the Dagger team (@ronshapiro) recommends the opposite 😞
We have a couple of options.
We provide an out of the box configuration mechanism that allows you to shade this in the app. But, this is problematic if the failures are silent at compile time, like you described. We could possibly have the google-services plugin catch these incompatibilities in dagger at compile time.
We could stop using dagger in our libraries (a very real possibility).
Any others come to mind ?
jhansche
commented
4 years ago The data transport libraries are used by some of the most widely adopted Firebase libraries: Firebase Messaging and Firebase Crashlytics. Other libs that use it include In App Messaging and a variety of other non Firebase libraries. Each of these would have to ship with their own copies of the shaded runtime, and the costs add up quickly particularly on lower end Android devices.
My suggestion was actually related to datatransport having its Dagger version shaded. It does not look like Firebase directly requires anything from that Dagger dependency, so shading it inside datatransport would hide it not only from apps, but even hides it from Firebase itself. And then all Firebase SDKs that use datatransport would automatically resolve to a single version (per Gradle's resolution rules), and there's no need to negotiate the shaded-dagger version in Firebase.
Thanks for raising. I did not realize that this would fail only at runtime. Is there a crash you can share ?
I don't currently have one, but the scenario would occur the same way as the Dagger issue linked above, starting at:
https://github.com/google/dagger/pull/950#issuecomment-353223029
In that particular case, the MembersInjector class was removed as of Dagger v2.15, so any apps using a previous version, would have their code generated using the pre-2.15 compiler, and a transitive dependency would cause a bump to post-2.15. That bump means the MembersInjector class is no longer present in runtime, but the app's code was compiled with it present on the classpath. So no errors at compile time, but the app's Dagger code would crash at runtime because the class cannot be found.
And a similar issue would happen if a library used an older version (so their generated code was generated with the older compiler), and the app is the one that requests an updated version. Same thing, just reversed, so the crash occurs in the library code, not the app code.
ashwinraghav
commented
4 years ago My suggestion was actually related to datatransport having its Dagger version shaded. It does not look like Firebase directly requires anything from that Dagger dependency, so shading it inside datatransport would hide it not only from apps, but even hides it from Firebase itself. And then all Firebase SDKs that use datatransport would automatically resolve to a single version (per Gradle's resolution rules), and there's no need to negotiate the shaded-dagger version in Firebase.
There are libraries other than data transport that use dagger too.
google-oss-bot
commented
4 years ago Hey @Alison-Soldado. We need more information to resolve this issue but there hasn't been an update in 5 weekdays. I'm marking the issue as stale and if there are no new updates in the next 5 days I will close it automatically.
If you have more information that will help us get to the bottom of this, just add a comment!
ashwinraghav
commented
4 years ago Pls ignore @google-oss-bot. No more information needed at this point.
cw-sanjeev
commented
4 years ago implementation 'com.google.firebase:firebase-messaging:20.1.0 implementation 'com.google.dagger:dagger:2.11' annotationProcessor 'com.google.dagger:dagger-compiler:2.11'
Causes an runtime exception static method membership injector cannot be found Any update on this
ashwinraghav
commented
4 years ago Hey Sanjeev, We are actively working on this, but recently hit a snag with tooling with getting our own instrumentation tests to run because of the way gradle plumbs artifacts. @vkryachko has WIP.
vkryachko
commented
4 years ago Yep, I have the PR up here: #1821 . But I got kind of stuck trying to get instrumentation tests to pass. I will try to make more progress on it in the coming days
vkryachko
commented
4 years ago The issue is fixed in the latest release of fiam and fiamd
acrespo
commented
4 years ago Just letting you guys (and other people reading this) know, this may be fixed for Firebase In-App Messaging but issue still persists in latest Firebase Crashlytics. When upgrading to Firebase Crashlytics SDK, following this (VERY USEFUL) guide https://firebase.google.com/docs/crashlytics/upgrade-sdk?platform=android, I had to upgrade to Dagger 2.27 (which was a bit unexpected).
vkryachko
commented
4 years ago @acrespo sorry about that, the next release of crashlytics will fix this, in the meantime you can manually include the following as a workaround:
implementation 'com.google.firebase:firebase-datatransport:17.0.8'Man, that was fast! Thanks! I managed to upgrade Dagger and everything went well but I'm glad to hear there was another option. Maybe it'll help someone else. Keep up the good work!
KalaiSelvanG
commented
4 years ago @acrespo sorry about that, the next release of crashlytics will fix this, in the meantime you can manually include the following as a workaround:
implementation 'com.google.firebase:firebase-datatransport:17.0.8'
This worked for me. I have excluded datatransport group and implemented com.google.firebase:firebase-datatransport:17.0.8 separtely and can able to run the application without runtime crash. Waiting for next release soon.
[REQUIRED] Step 2: Describe your environment
[REQUIRED] Step 3: Describe the problem
Steps to reproduce:
After added the dependency in-app messaging cannot find symbol DaggerAppComponent
Relevant Code:
build.gradle(:app)
build.gradle