Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems with new Gradle Plugin #490

Closed
5 tasks done
rubengees opened this issue Apr 12, 2020 · 15 comments
Closed
5 tasks done

Problems with new Gradle Plugin #490

rubengees opened this issue Apr 12, 2020 · 15 comments
Assignees
Labels

Comments

@rubengees
Copy link
Contributor

About this issue

I am in the process of migrate my app to latest materialdrawer, iconics and aboutlibrares. The new Gradle Plugin works great, but produces some odd things which I wanted to report.

okhttp-brotli iconics LeakCanary
Screenshot_20200412-134318 Screenshot_20200412-134334 Screenshot_20200412-134359
  • okhttp-brotli seems to not resolve the name correctly (POM).
  • Iconics shows up twice (propably because of the internal library definition)? Happens for other libs with an internal definition also.
  • leakcanarys ObjectWatcher shows up many times (Not sure if these are multiple dependencies, but can't see because of the truncated title).

Usage

LibsBuilder()
    .withAutoDetect(false)
    .withShowLoadingProgress(false)
    .withAboutVersionShown(false)
    .withAboutIconShown(false)
    .withVersionShown(false)
    .withOwnLibsActivityClass(MyLibsActivity::class.java)
    .withActivityTitle(getString(R.string.about_licenses_activity_title))
    .start(requireActivity())

Details

  • Used library version: 8.1.1
  • Used support library version: appcompat 1.2.0-beta01
  • Used gradle build tools version: 6.3
  • Used tooling / Android Studio version: 4.0.0-beta03
  • Other used libraries, potential conflicting libraries: None

Checklist

@mikepenz
Copy link
Owner

@rubengees fun fact: it actually solves correctly https://github.com/google/brotli/blob/master/java/org/brotli/pom.xml#L10
We will have to overwrite this one manually.

Yeah the plugin does not make a difference about parent and child as there is no chance for the lib to detect if it is actually 2 different libraries or just the same with different modules. I suppose there may be great if we could merge them somehow.

I suppose. but in addition you should not have leak canary in your production app :O

@rubengees
Copy link
Contributor Author

Hmm, interesting. I'll report an issue in the brotli repository (I thought all the time this was okhttp-brotli and not brotli itself).

Yes, some merging or deduplication would be great! :)

I do not have Leak Canary in the release variant. I'm not sure which dependency container the plugin looks into, on releaseRuntimeClasspath it's not at least. Could that be an issue?

@mikepenz
Copy link
Owner

luckily we are prepared for such cases with: https://github.com/mikepenz/AboutLibraries/blob/develop/library-definitions/src/main/res/raw/custom_name_mappings.prop :D
you can also provide your own custom mappings as noted in the docs :) this will allow you to resolve it until there's an update.

but that would be a manual process :D

the screenshots above, where they from the release build?

@mikepenz mikepenz self-assigned this Apr 15, 2020
@rfc2822
Copy link
Contributor

rfc2822 commented Apr 24, 2020

I can confirm the brotli thing for 8.1.2, see https://forums.bitfire.at/post/12856

And thanks for all your efforts :)

@mikepenz
Copy link
Owner

@rfc2822 Btw. for the meantime you can for example overwrite the name of brotli doing this:

https://github.com/mikepenz/AboutLibraries/blob/develop/app/src/main/java/com/mikepenz/aboutlibraries/sample/FragmentActivity.kt#L175

Just use its identifier created by the gradle plugin. (you can get it via the findLibraries task)

Hope this helps.

Beste Grüße.

@rubengees
Copy link
Contributor Author

Regarding deduplication: When using the Gradle plugin and if an entry is found for a library that also has an internal definition one of the definitions can be safely omitted, can't it?

For dependencies which have the same content (not sure if that's the case for the leakcanary dependencies), only one can be shown I guess?

You are right, that was a debug build and leakcanary is on the classpath there.

@mikepenz
Copy link
Owner

The entries are not duplicated because of internal definitions. those are no longer showing up anyways :) they are basically deprecated and most likely I plan to remove them at some point.

From legal perspective all dependencies have to be shown. So I am not sure how accurate it is to "magically" merge similar ones.

@rubengees
Copy link
Contributor Author

Ah great, you are right!

I investigated the LeakCanary case further and found that these are indeed various submodules with slightly different names. Couldn't see that because of the truncated titles, but I also remember you said that showing the title multiline is no option somewhere so I guess there is nothing to be done here.

@mikepenz
Copy link
Owner

hmmm the latest version offers a new minimal UI which is not as verbose. but regardless. I will have to think of a "clever" way to merge libraries some way.

or at least link them in some way. also having multi license support.

but that is something for v9 :D

mbiebl added a commit to messageconcept/PeopleSyncClient that referenced this issue Jul 3, 2020
@AllanWang
Copy link
Contributor

What was the fix for deduplication exactly? I've updated a library I'm using to use the plugin, and I'm keeping my main app with a manual include list as the list gets too populated (#520). I'm getting duplicated cards for libraries used in both projects.

Is the main fix to migrate my main app to use the plugin? I'm looking for more of a whitelist filter as opposed to a blacklist one

@mikepenz
Copy link
Owner

For now the approach was via the exclusion list which was deeper included in the plugin so you can have a simple list there to drop libs.

I am still thinking of a good approach to introduce a intelligent "merge" mechanism which would be an optional choice.

Note neither excluding nor a specific allow list is generally suggested as all libs should retrieve their attribution.

@AllanWang
Copy link
Contributor

I think that's fair, but currently a lot of the entries are not really dependencies, but subcomponents of a main dependency. I'm also not sure if it's pulling all dependencies in the project or just the ones added by the project

@mikepenz
Copy link
Owner

On that note, I am still curious about the "I'm getting duplicated cards for libraries used in both projects." As this kind of seems strange to me. the plugin will generate the same resource Ids so they overwrite each other.

Could it be that it's possibly a bug due to your manual inclusion list?

@AllanWang
Copy link
Contributor

My main project uses the old method where I define the Libs + packages. I'm guessing that isn't the supported method and I'd be fine with switching over, but the alternative lists way too many libraries. It's not that I don't want to give credit to everything, but I believe that the list of that size makes it harder for other developers to identify the direct dependencies I'm using. Looking at it again, I think it's listing items for every dependency recursively

@mikepenz
Copy link
Owner

@AllanWang yes the library lists all dependencies. E.g. things which are getting pulled into your project and are required for your project to compile.

MaterialDrawer for example uses FastAdapter. MaterialDrawer won't work without FastAdapter so both get credits.
(In the past it also required Iconics but I removed that in v8, also Materialize is removed)

But similar principle for all the jetpack libs. it kind of needs those deps to serve the features you are using in the end, so it seems correct from that point of view as far as I can say.


And yeah it might be a mix of both versions of the lib.


v8 allows you to have an exclusion list if something is to verbose in dependencies, shouldn't be the case for anything though.

Let's see if we can come up (the other ticket you opened) with a proper merging strategy which shall help with your requirement :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants