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

Gradle 8 updates #2669

Merged
merged 9 commits into from
Apr 17, 2024
Merged

Gradle 8 updates #2669

merged 9 commits into from
Apr 17, 2024

Conversation

shubham1g5
Copy link
Contributor

cross-request: dimagi/commcare-core#1277

@shubham1g5
Copy link
Contributor Author

shubham1g5 commented May 5, 2023

This will need more work to resolve Kotlin compilation errors due to us using a non compile time constant in Kotlin files like - @ManagedUi(R.layout.http_request_layout). We will probably need some other mechanism to initialise the managed UIs like standardising a public method to get the layout instead and then use method reflection to get the layout value instead.

fun getLayoutResource() : int { return R.layout.http_request_layout}

@avazirna
Copy link
Contributor

@shubham1g5 should we plan this for 2.55?

@shubham1g5 shubham1g5 modified the milestones: 2.54, 2.55 Sep 14, 2023
@shubham1g5
Copy link
Contributor Author

should we plan this for 2.55?

yes

@avazirna
Copy link
Contributor

avazirna commented Mar 4, 2024

@damagatchi retest this please

@avazirna
Copy link
Contributor

From Gradle 8.0, R classes are generated with non-final fields by default, this can be prevented by setting the flag to false.

@avazirna
Copy link
Contributor

@damagatchi retest this please

…ndencies

From Gradle 8.0, R classes are non transitive by defaut, so when referencing
from other modules/dependencies it's required their fully qualified classe name
OR switch off the flag
@avazirna
Copy link
Contributor

@damagatchi retest this please

1 similar comment
@avazirna
Copy link
Contributor

@damagatchi retest this please

@@ -0,0 +1,15 @@
-dontwarn com.sun.javadoc.**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should the proguard rules for test be same as prod ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that, in previous version, ProGuard rules were mostly applied to production apks, very little (if any) optimizations to test apks. Gradle 8 with R8 in fullMode by default changed that, so now we also need specific rules for the tests apk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but should not the test apk should have the same set of rules as prod apk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or curious how did we came up with these rules ?

Copy link
Contributor

Choose a reason for hiding this comment

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

from the missing_rules file after a failed build. Here's a snippet of the report:

Missing class com.sun.javadoc.Doc (referenced from: java.lang.String org.checkerframework.org.plumelib.options.OptionsDoclet.javadocToHtml(com.sun.javadoc.Doc) and 3 other contexts)
Missing class com.sun.javadoc.DocErrorReporter (referenced from: boolean org.checkerframework.org.plumelib.options.OptionsDoclet.validOptions(java.lang.String[][], com.sun.javadoc.DocErrorReporter))
Missing class com.sun.javadoc.FieldDoc (referenced from: void org.checkerframework.org.plumelib.options.OptionsDoclet.processEnumJavadoc(org.checkerframework.org.plumelib.options.Options$OptionInfo) and 1 other context)
Missing class com.sun.javadoc.RootDoc (referenced from: com.sun.javadoc.RootDoc org.checkerframework.org.plumelib.options.OptionsDoclet.root and 5 other contexts)
Missing class com.sun.javadoc.SeeTag (referenced from: java.lang.String org.checkerframework.org.plumelib.options.OptionsDoclet.javadocToHtml(com.sun.javadoc.Doc))
Missing class com.sun.javadoc.Tag (referenced from: java.lang.String org.checkerframework.org.plumelib.options.OptionsDoclet.javadocToHtml(com.sun.javadoc.Doc))
Missing class com.sun.source.tree.AnnotatedTypeTree (referenced from: java.lang.Boolean annotator.find.ASTPathCriterion$1.visitAnnotatedType(com.sun.source.tree.AnnotatedTypeTree, com.sun.source.tree.Tree) and 26 other contexts)
Missing class com.sun.source.tree.AnnotationTree (referenced from: boolean annotator.find.TreeFinder.alreadyPresent(com.sun.source.util.TreePath, annotator.find.Insertion) and 27 other contexts)
Missing class com.sun.source.tree.ArrayAccessTree (referenced from: com.sun.source.tree.Tree annotator.find.ASTPathCriterion.getNext(com.sun.source.tree.Tree, scenelib.annotations.io.ASTPath, int) and 28 other contexts)
Missing class com.sun.source.tree.ArrayTypeTree (referenced from: com.sun.source.tree.ArrayTypeTree org.checkerframework.dataflow.cfg.node.ArrayTypeNode.tree and 29 other contexts)
Missing class com.sun.source.tree.AssertTree (referenced from: com.sun.source.tree.Tree annotator.find.ASTPathCriterion.getNext(com.sun.source.tree.Tree, scenelib.annotations.io.ASTPath, int) and 13 other contexts)
Missing class com.sun.source.tree.AssignmentTree (referenced from: com.sun.source.tree.Tree annotator.find.ASTPathCriterion.getNext(com.sun.source.tree.Tree, scenelib.annotations.io.ASTPath, int) and 23 other contexts)
Missing class com.sun.source.tree.BinaryTree (referenced from: com.sun.source.tree.BinaryTree org.checkerframework.checker.interning.InterningVisitor$2.val$node and 98 other contexts)

app/proguard.cfg Outdated
@@ -102,6 +104,7 @@
-dontnote okhttp3.**

#Retrofit Stuff
-keep class retrofit2.** { *; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does Gradle update affect proguard rules ?

Copy link
Contributor

@avazirna avazirna Mar 20, 2024

Choose a reason for hiding this comment

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

Gradle 8 is on full mode by default, which means more aggressive optimizations. In this case, it was causing IllegalArgumentExceptions with the message Call return type must be parameterized as Call<Foo> or Call<? extends Foo> during get requests, which made me suspect that maybe some references had been obfuscated. After some digging, I found some posts that pointed that way too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, although I don't think it's a good practice to retain all code from a package. Probably we should instead take the rules from here

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like this is still pending.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, just pushed that also

@avazirna avazirna self-requested a review March 20, 2024 07:02
@shubham1g5
Copy link
Contributor Author

@avazirna checking if you are stuck on something with this ?

@avazirna
Copy link
Contributor

@avazirna checking if you are stuck on something with this ?

Nothing, all good. Let me just check why did the build fail.

avazirna
avazirna previously approved these changes Apr 12, 2024
@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

3 similar comments
@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

@avazirna
Copy link
Contributor

@damagatchi retest this please

@shubham1g5
Copy link
Contributor Author

Great, looks good to merge to me as well.

@avazirna avazirna merged commit b2a3665 into master Apr 17, 2024
4 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants