Repository navigation
Conversation
This is a set of custom lints we wrote internally to help with migration to improved kotlin support in Dagger.
This covers the following cases:
- Using `field:` site target in injected properties
- Using `@JvmStatic` in object modules
- Annotating `companion object`s with `@Module`
These checks also provide lintFixes.
One significant caveat - the lint jar must be applied as an artifact directly, as the bazel tooling for `android_library` is quite lacking and doesn't offer an equivalent to Android Gradle Plugin's `lintPublish`.
For Android consumers, using this would look like so:
```gradle
dependencies {
lintChecks "com.google.dagger:dagger-lint:<version>"
}
```
For plain Kotlin projects, they can use the lint plugin (I believe this is right, but it's been a minute since I've dealt with it)
```gradle
apply plugin: "com.android.lint"
dependencies {
lintChecks "com.google.dagger:dagger-lint:<version>"
}
```
|
I'm currently unable to figure out how to get the tests to run correctly. Sought advice here: bazelbuild/bazel-integration-testing#153 (comment) |
|
Could you add a corresponding AAR artifact as well? |
|
@technoir42 I don't think that works. The ideal case would be to embed the lint.jar in the existing dagger aar artifacts along with a standalone jar artifact for non-android projects, which this currently does the latter. I'm not sure making the lint library an aar itself would actually even work (lints in aars have a specific format, putting the lint in classes.jar isn't it), plus it would prevent its use in non-android projects. I'm also fairly certain lintCheck() allows multiple targets in Gradle, as it just collects jars. The main issue here is bazel's android_library rule doesn't support an equivalent to AGP's lintPublish() configuration. |
|
looking at the sources from 4.1.0 alphas: I would think standalone jar consuption as well as auto via AAR's would be ideal. what happens if the jar is in multiple AAR's? EDIT: |
We do this today with AutoDispose and haven't had any issues reported. I'd imagine lint does some degree of deduping |
|
Actually my guess there is probably not any classpath validation, so it just takes the first it finds when loading up the checks? it is usually R8/D8 that screams about duplicate entries when an AAR repacks a publicly distributed dependency in the libs dir. |
There was a problem hiding this comment.
Thanks creating this PR!
|
Wrote a genrule to encapsulate the |
|
Well... maybe not. This is particularly painful given bazel's learning curve, will keep working at it |
|
It seems like bazel explicitly prohibits modifying rule outputs on linux. Unclear why the behavior on my mac is different, and I'm also not sure how to proceed here. Bazel's android support is too limited to properly support lint if we want to actually embed it in aars. If there's a way to disable the read-only behavior in the genrule sandbox then we'd have a path forward. |
|
Ok, it's as far as I can take it I think. I believe the genrule works now, but I get this latest build error on master as well with my local install of bazel ( |
| "ModuleCompanionObjects", | ||
| "Module companio 10BC8 n objects should not be annotated with @Module.", | ||
| """ | ||
| Companion objects in @Module-annotated classes are considered part of the API. |
| LintFix.create() | ||
| .name("Remove @Module") | ||
| .replace() | ||
| .text("@Module") |
There was a problem hiding this comment.
What if someone types @dagger.Module?
| Args: | ||
| name: The name of the target. | ||
| """ | ||
| native_result = native.android_library(name = name, **kwargs) |
There was a problem hiding this comment.
What if you use a suffix for the default target name, and then the genrule can just put the aar in the place you'd like it to be?
There was a problem hiding this comment.
That was my initial approach, but other android libraries that depend on this expect a particular naming structure for artifacts that would then no longer work for anything else generated by this rule
There was a problem hiding this comment.
What's an example of that?
Did you try using a genrule, and then wrapping that in an aar_import rule?
There was a problem hiding this comment.
One example is Android support will fail to fail due to trying to expose dagger Android. This seems to be a combination of depending on outputs it has plus the type of like libraries it can depend on.
I haven't tried that. I'm not sure I follow either 🤔. Before I go back down the rabbit hole, is there something inherently wrong with this implementation that merits using another one?
There was a problem hiding this comment.
I'm pretty sure modifying bazel outputs breaks the hermeticity model. In this case, you never really want someone to depend on the intermediate android_library, but in general Bazel rules create new outputs only.
Here's something like what I had in mind:
native.android_library(
name = name + "step_1",
**kwargs
)
native.genrule(
name = name + "inject_android_lint",
...
outs = [name + "with_injected_android_lint.aar"],
)
native.aar_import(
name = name + "aar_import",
aar = name + "with_injected_android_lint.aar",
)
kwargs[deps] += [name + "aar_import"]
native.android_library(
name = name, # this is the actual target people will "use"
**kwargs
)| # Restore previous permissions | ||
| chmod -w $(location {created_aar}) | ||
|
|
||
| # Write md5 of the final aar into a the output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
into a the - typo?
This is a set of custom lints along with fixes to help with migration to improved kotlin support in Dagger. This covers the following cases: * Using field: site target in injected properties * Using @JvmStatic in object modules * Annotating companion objects with @module This CL also includes the necessary infrastructure changes to build, package and deploy lint rules in the Dagger project. For Kotlin-only projects, a new artifact 'com.google.dagger:dagger-lint' will be published so that lint checks can be added via the lintChecks configuration in Gradle: ``` apply plugin: "com.android.lint" dependencies { lintChecks "com.google.dagger:dagger-lint:<version>" } ``` Meanwhile for users of Hilt and dagger-android, the lint checks will be applied from a new transitive dependency 'com.google.dagger:dagger-lint-aar'. Closes #1805 RELNOTES="Add custom lint checks to Dagger" ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=310269729
This is a set of custom lints along with fixes to help with migration to improved kotlin support in Dagger. This covers the following cases: * Using field: site target in injected properties * Using @JvmStatic in object modules * Annotating companion objects with @module This CL also includes the necessary infrastructure changes to build, package and deploy lint rules in the Dagger project. For Kotlin-only projects, a new artifact 'com.google.dagger:dagger-lint' will be published so that lint checks can be added via the lintChecks configuration in Gradle: ``` apply plugin: "com.android.lint" dependencies { lintChecks "com.google.dagger:dagger-lint:<version>" } ``` Meanwhile for users of Hilt and dagger-android, the lint checks will be applied from a new transitive dependency 'com.google.dagger:dagger-lint-aar'. Closes #1805 RELNOTES="Add custom lint checks to Dagger" ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=310269729
This is a set of custom lints we wrote internally to help with migration to improved kotlin support in Dagger.
This covers the following cases
Using
field:site target in injected propertiesUsing
@JvmStaticin object modulesAnnotating
companion objects with@ModuleThese checks also provide lintFixes.
Edit: Wrote a custom
dagger_android_librarythat I believe covers the android piece now!One significant caveat - the lint jar must be applied as an artifact directly, as the bazel tooling for
android_libraryis quite lacking and doesn't offer an equivalent to Android Gradle Plugin'slintPublish.For Android consumers, using this would look like so:For plain Kotlin projects, they can use the lint plugin (I believe this is right, but it's been a minute since I've dealt with it)