Move identifiernamestring to proguard config + enable default #1720
Conversation
This moves the `identifiernamestring` directly to the shipped proguard config. R8 is the default now, so this should be safe to enable by default at this point and I've updated the flag to match. This is important also because the current implementation was not compatible with incremental annotation processing, and would force full rebuilds if stringKeys() was enabled. This was because it did not report originating elements. Since it's also a hardcoded construct and not attached to a particular element, it would have nothing to originate from even if we did want to add them.
34fc0ff to
acc4614
Compare
java/dagger/android/proguard.cfg
Outdated
|
|
||
| -identifiernamestring class dagger.android.internal.AndroidInjectionKeys { | ||
| java.lang.String of(java.lang.String); | ||
| } |
|
Looks like Bazel is complaining about the Proguard rule, presumably because it doesn't know that it's valid in vanila Proguard |
|
Yeah we'll need to do the split up files approach for this, which I'm not thrilled about. Bazel's Android rule leaves a lot to be desired for this as well, so I think the actual best solution may honestly be to run an annotation processor over the dagger-android project that generates resource files in the structure AGP expects. Unfortunately doesn't seem like the Android rule exposes a way to declare jar resources like a normal java_library rule does, and dagger's unconventional project structure may prevent that approach regardless because the rule wouldn't be able to infer a path |
|
The CI failure seems unrelated. Master is also failing |
| import static java.lang.annotation.RetentionPolicy.SOURCE; | ||
|
|
||
| /** | ||
| * Our proguard rules generator needs one annotation to hook into for it to run, so we use this |
There was a problem hiding this comment.
Maybe we can make ProguardProcessor requests all annotations (returning "*" on getSupportedAnnotations()), that way we don't need to make a dummy annotation, since either way we always want to apply this processor.
There was a problem hiding this comment.
Good idea! I always forget that's possible 21b6d6e
| @@ -0,0 +1,117 @@ | |||
| /* | |||
| * Copyright (C) 2017 The Dagger Authors. | |||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package dagger.android.proguardprocessor; |
There was a problem hiding this comment.
Move to dagger.android.internal.proguard
| writer.write(proguardRules); | ||
| } | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
throw new RuntimeException(e) ?
| @@ -0,0 +1,45 @@ | |||
| # Copyright (C) 2017 The Dagger Authors. | |||
| try (Writer writer = filer.createResource(CLASS_OUTPUT, | ||
| "", | ||
| "META-INF/com.android.tools/proguard/dagger-android.pro") | ||
| .openWriter()) { | ||
| writer.write(proguardRules); | ||
| } |
There was a problem hiding this comment.
Extract to a method that takes the file path and content? So that we don't repeat the same writing maneuver.
|
Confirmation they're being generated
|
This moves the `identifiernamestring` directly to the shipped proguard config. R8 is the default now, so this should be safe to enable by default at this point but the flag is not yet updated in this change. This change is important also because the current implementation was not compatible with incremental annotation processing, and would force full rebuilds if stringKeys() was enabled. This was because it did not report originating elements. Since it's also a hardcoded construct and not attached to a particular element, it would have nothing to originate from even if we did wanted to add them. Closes #1720 RELNOTES=Generate 'identifiernamestring' r8 rule by default. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=291056440
This moves the `identifiernamestring` directly to the shipped proguard config. R8 is the default now, so this should be safe to enable by default at this point but the flag is not yet updated in this change. This change is important also because the current implementation was not compatible with incremental annotation processing, and would force full rebuilds if stringKeys() was enabled. This was because it did not report originating elements. Since it's also a hardcoded construct and not attached to a particular element, it would have nothing to originate from even if we did wanted to add them. Closes #1720 RELNOTES=Generate 'identifiernamestring' r8 rule by default. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=291056440
This moves the
identifiernamestringdirectly to the shipped proguard configs. R8 is the default now, so this should be safe to enable by default at this point and I've updated the flag to match.This is important also because the current implementation was not compatible with incremental annotation processing, and would force full rebuilds if stringKeys() was enabled. This was because it did not report originating elements. Since it's also a hardcoded construct and not attached to a particular element, it would have nothing to originate from even if we did want to add them.
Note that this option is not available in standard proguard, so this generates custom proguard rules into resources under the custom layout that AGP supports for configuring different rules for R8 and proguard. The processor solution isn't pretty, but bazel doesn't allow specifying custom jar resources and setting up a throwaway intermediary java package seemed strictly worse than just generating them.