-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Re-writing accessors transformer using the new AGP API #7694
Conversation
dependencies.list
Outdated
ANDROID_BUILD_TOOLS=30.0.3 | ||
KOTLIN=1.5.31 | ||
KOTLIN_COROUTINES=1.5.2 | ||
KOTLIN=1.6.20 |
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.
Probably you can try 1.7 as is the latest stable available
@@ -1,5 +1,5 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-bin.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-bin.zip |
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.
7.5 is also available
@rorbech I updated the current WIP
For reference: |
I manually verified that all our |
Update config.yml
Remove export compliance clause
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.
Awesome work 🥇 🕺 ... As I don't really know the new transform API it was a bit hard to track some of the changes, but if all our sample projects are building and tests are running, I think we can assume that things are working 🥳
I only left some minor comments.
...s/architectureComponentsExample/src/main/java/io/realm/examples/arch/PersonListFragment.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/main/java/io/realm/internal/objectstore/OsMutableSubscriptionSet.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/main/java/io/realm/internal/objectstore/OsSubscription.java
Outdated
Show resolved
Hide resolved
val metadataCollector = | ||
io.realm.buildtransformer.asm.visitors.AnnotationVisitor(annotationDescriptor) | ||
|
||
forEachJarEntry { jarEntry, inputStream -> |
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.
It is a bit unclear why we iterate the jar entries twice, some documentation for each block would be nice.
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.
Added some comments
return@iterateDirs // Directory was deleted | ||
forEachJarEntry { jarEntry, inputStream -> | ||
val bytes = inputStream.use { inputStream -> | ||
if (jarEntry.name.endsWith(".class")) { |
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.
What happens with ressource files in the JAR? We seem to skip them here, but I'm not sure how they then end up in the outputprovider?
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.
They are handled by the else branch 🎉 🤪
private val inputs: ListProperty<Directory>, | ||
private val allJars: ListProperty<RegularFile>, | ||
private val referencedInputs: ConfigurableFileCollection, | ||
private val output: RegularFileProperty) { | ||
private lateinit var metadata: ProjectMetaData | ||
private var analytics: RealmAnalytics? = null | ||
|
||
init { |
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.
Same, running all of this logic directly in the constructor feels ...iffy
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.
Same, I just exposed transform
as internal
and call it on the constructed object instead of implicitly from the init-block.
realm-transformer/src/main/kotlin/io/realm/transformer/RealmTransformer.kt
Show resolved
Hide resolved
classPool.close(); | ||
fun copyProcessedClasses() { | ||
for ((fqname: String, clazz: CtClass) in processedClasses) { | ||
outputProvider.putNextEntry(JarEntry("${fqname.replace('.', '/')}.class")) |
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.
Does this also work on Windows or should we use File.separatorChar?
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.
I would argue heavily that the Zip file entry header should be platform agnostic ... otherwise you would need to supply different Zip-files for each platform. And since it works on one platform then I assume that it would also work on the others.
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.
💯
Closes #7714