-
Notifications
You must be signed in to change notification settings - Fork 61
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
Metric overhaul #1504
Metric overhaul #1504
Conversation
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.
LGTM 👍 minor stuff!
packages/plugin-compiler/src/main/kotlin/io/realm/kotlin/compiler/RealmCommandLineProcessor.kt
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Outdated
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Outdated
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Outdated
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/RealmCompilerSubplugin.kt
Show resolved
Hide resolved
Co-authored-by: Nabil Hachicha <[email protected]>
// Task to fetch core version from dependency.list from core submodule | ||
tasks.create("coreVersion", Exec::class.java) { | ||
workingDir = project.file(listOf("..", "external", "core").joinToString(File.separator)) | ||
commandLine = listOf("grep", "^VERSION", "dependencies.list") |
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'm not sure if grep is available on Windows? It would probably be safer to use the Kotlin API to read the file content and find it that way?
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.
Damn it. You and your windows machine 😅
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 updated it to read the file through Kotlin API on top of Gradles file content provider API so now it should be fully platform agnostic and appropriately regenerate if external/core/dependencies.list
is updated.
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/analytics/AnalyticsService.kt
Show resolved
Hide resolved
packages/plugin-compiler/src/main/kotlin/io/realm/kotlin/compiler/RealmCommandLineProcessor.kt
Show resolved
Hide resolved
Analytics is now printed if the print option is enabled (with The analytics information is only gathered if neither needed for printing nor submission. This enable us to test collection in CI enviroments by enable printing but keeping it disabled. |
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.
Looking really good. Mostly minor comments and then we should probably also retest this on Windows 😅
dependencies { | ||
classpath("io.realm.kotlin:gradle-plugin:${Realm.version}") | ||
} | ||
extra["realmVersion"] = file("${rootProject.rootDir.absolutePath}/../../../buildSrc/src/main/kotlin/Config.kt") |
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 assume this is because you want to decouple the tests completely?
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.
Yes. It we use buildSrc
to gain access to Config.kt
it is problematic to use Kotlin and AGP versions that are different from the ones in Config.kt
because buildSrc
applies a number of plugins and that will fix the versions.
@@ -106,8 +115,10 @@ tasks.create("pluginVersion") { | |||
// Generated file. Do not edit! | |||
package io.realm.kotlin.gradle | |||
internal const val PLUGIN_VERSION = "${project.version}" | |||
internal const val CORE_VERSION = "${coreVersion}" |
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.
Given that we often point to commits that are not tagged releases, this might not be 100% accurate. But I assume that appending the submodule commit sha might be a bit tricky, so maybe this fine?
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 not tricky ... on the client side. But it will probably clutter the remote view. I guess we should rather just strive to use tagged core-releases 😉 For discussion see https://mongodb.slack.com/archives/C04M17MCY0H/p1693920525143069
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/RealmCompilerSubplugin.kt
Outdated
Show resolved
Hide resolved
packages/gradle-plugin/src/main/kotlin/io/realm/kotlin/gradle/RealmCompilerSubplugin.kt
Outdated
Show resolved
Hide resolved
This just requires manual verification on Windows - @cmelchior let me know when tested. |
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.
Testing this on Windows with Java 17, I got the following failure on the Gradle 6 integration test:
-----------
* What went wrong:
Execution failed for task ':single-platform:compileReleaseJavaWithJavac'.
> java.lang.IllegalAccessError: class org.gradle.internal.compiler.java.ClassNameCollector (in unnamed module @0x6592fe6e) cannot access class com.sun.tools.javac.code.Symbol$TypeSymbol (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.code to unnamed module @0x6592fe6e
Using
gw :single-platform:assemble --configuration-cache
The Gradle 8 test worked fine using the same command.
I don't expect this to be an issue with PR nor our plugin. I can provoke something similar on a Mac:
from our current Does it work assembling the |
After testing, you are correct, this is also happening on |
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.
This PR reworks analytics collection to track individual platform builds and aligns submitted data with other SDKs.
This also introduces introduces the ability to print the collected information to the Gradle log (with warning loglevel) by setting the enviroment variable
There is strictly no need to run this as a service anymore as we don't really gather metrics across tasks anymore. But felt more appropriate to keep it around if we should extent the responsibilities at some point.
TODOs
Closes #127
Closes #1390
Closes #1492