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

Introduce UsagesHandler that allows to exclude particular classes fro… #545

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Introduce UsagesHandler that allows to exclude particular classes fro… #545

merged 3 commits into from
Jan 10, 2022

Conversation

nsk-mironov
Copy link
Contributor

I'm working on a project that heavily depends on Data Binding Library.

Data Binding compiler automatically generates DataBinderMapperImpl class for every data binding powered module:

package com.example.compositefeature

public class DataBinderMapperImpl extends DataBinderMapper {
  @Override
  public List<DataBinderMapper> collectDependencies() {
    ArrayList<DataBinderMapper> result = new ArrayList<DataBinderMapper>(14);
    result.add(new androidx.databinding.library.baseAdapters.DataBinderMapperImpl());
    result.add(new com.example.feature1.DataBinderMapperImpl());
    result.add(new com.example.feature2.DataBinderMapperImpl());
    result.add(new com.example.feature3.DataBinderMapperImpl());
    return result;
  }
}

The implementation of collectDependencies method includes all DataBinderMapperImpl classes provided by dependencies of current module. It means dependency-analysis-plugin will never report data binding related dependencies as unused, because there is always at least one usage in generated DataBinderMapperImpl code.

The goal of this PR is to introduce an API that will allow to exclude particular classes (DataBinderMapperImpl in my case) from a list of used classes.

I've implemented a proof of concept version of the API and added new functional tests to verify it solves the problem I'm trying to solve.

@nsk-mironov
Copy link
Contributor Author

I tried running DataBindingUsagesExclusionsSpec with -Dv=2 flag and it doesn't pass anymore because of the following advice:

Advice(
  dependency=androidx.databinding:databinding-compiler:7.2.0-alpha06, 
  usedTransitiveDependencies=[], 
  parents=null, 
  fromConfiguration=annotationProcessor, 
  toConfiguration=null
)

Seems like the best thing here is to add databinding-compiler to the list of dependencies excluded by DataBindingFilter.

@autonomousapps
Copy link
Owner

autonomousapps commented Jan 7, 2022

First, thank you for the contribution. I will think about it for a bit to consider if it's right way to express this use-case (and it might be, but I'm not prepared to say one way or the other just yet).

Regarding the failure, could you elaborate? Which test is failing with v2? Do you have a build scan? D'oh, you say it right there. It's the new spec.

Seems like the best thing here is to add databinding-compiler to the list of dependencies excluded by DataBindingFilter.

That does seem like the right approach, although I'm not 100% sure it works for annotation processor dependencies.

@nsk-mironov
Copy link
Contributor Author

I will think about it for a bit to consider if it's right way to express this use-case (and it might be, but I'm not prepared to say one way or the other just yet).

Sure, no pressure at all. Would love to hear your opinion on idiomatic way to cover the use-case.

That does seem like the right approach, although I'm not 100% sure it works for annotation processor dependencies.

It works without any issues. I've already pushed a commit a49e4259c08526acb4b59d4509463f4c57cc38e7 thats filters out databinding-compiler dependency. Now DataBindingUsagesExclusionsSpec passes both with and without -Dv=2 flag.

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given it some thought and I like the symmetry you achieved between this and the ABI exclusions feature. Please rebase and fix the conflict (should be very small, sorry about that) and then it's good to merge. Thanks for the contribution!

If you like, I wouldn't mind a contribution to the wiki. You can't edit it directly because Github is dumb, but if you file a PR or issue with some text, I can add it myself.

@nsk-mironov
Copy link
Contributor Author

Rebased and fixed the conflict. It looks like there is something wrong with the -Dv=2 flag in the latest main branch changes. The newly added DataBindingUsagesExclusionsSpec doesn't pass anymore with the -Dv=2 and the same happens with other existing specs:

@autonomousapps
Copy link
Owner

Ah, whoops! Thanks for the report. I just pushed a change to main that resolves that issue. This project's CI config doesn't yet run any v2 tests (I run them manually), so it's not surprising to see regressions show up. You can rebase again, and there shouldn't be any conflicts this time.

@nsk-mironov
Copy link
Contributor Author

Great, thank you! I've rebased the PR onto the latest main and everything seems to be working now.

@autonomousapps autonomousapps merged commit 2ce1819 into autonomousapps:main Jan 10, 2022
@nsk-mironov nsk-mironov deleted the usages-exclusions branch January 10, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants