-
Notifications
You must be signed in to change notification settings - Fork 651
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
Migrate to org.jetbrains.intellij.platform v2.0.0 #5657
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
a57a3ff
to
ddad3e6
Compare
ddad3e6
to
59c9233
Compare
c2eb372
to
7ca2a30
Compare
1a97abb
to
319c233
Compare
it.inputs.property("allVersions", Callable { | ||
it.inputs.property("allVersions", project.provider { |
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.
Interesting... Was the issue that the callable is called eagerly?
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.
Exactly! (Credits to @hsz 🙏). However this works locally but just noticed we still got the ConcurrentModificationException
on the CI 😭
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.
IIRC intellij-plugin
uses the published version of the Gradle plugin so we won't be able to tell before the next release if the fix actually worked.
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.
intellij-plugin
uses the published version of the Gradle plugin
You're right! And my mistake - thought it was working locally but it's only because I ran with --no-configuration-cache
:(
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'd say the concurrent modification happens on the configurations
collection because we're iterating all of them but addAllLater
(here) may create new configurations while iterating (here)
Proposed fix to iterate a read-only copy of the configurations:
private fun getDeps(configurations: ConfigurationContainer): List<String> {
- return configurations.flatMap { configuration ->
+ // see https://github.com/apollographql/apollo-kotlin/pull/5657
+ val currentConfigurations = configurations.toList()
+ return currentConfigurations.flatMap { configuration ->
configuration.dependencies
Means we won't check the newly created configurations for conflicting Apollo dependencies but it's probably 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.
@hsz sorry, messages crossed. Regarding SNAPSHOTs, it's about Apollo releases. AKIJP (Apollo Kotlin IntelliJ Plugin) uses AKGP (Apollo Kotlin Gradle Plugin) . We could verify using Apollo snapshots but means we'd need a separate PR to check the plugin-ception 🙃 .
All in all, my vote goes to disabling the check 😄
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.
Yeah, I figured too late. Anyway, I'm around if you need me!
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.
@martinbonnin unfortunately disabling the task doesn't work - the exception is thrown at configuration time - not 100% sure why, but looks like getDeps
ends up being called during org.gradle.configurationcache.ConfigurationCacheIO.writeConfigurationCacheState
.
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.
Mmmpff... Disable CC (configuration cache) in that CI job? Until we make a release and bump that AKGP dep?
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.
Haha that should work! And I'll open an PR for your suggested fix above, so it can be in the next release, hopefully fixing the issue.
… to avoid a ConcurrentModificationException
01de58c
to
74f4029
Compare
Resolves #5647.
Keeping as draft for now, while this is beta..