-
Notifications
You must be signed in to change notification settings - Fork 1
Fix plugin ID mismatch between Gradle plugin and compiler plugin #55
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,10 @@ package org.jetbrains.kotlin.formver.cli | |
| import org.jetbrains.kotlin.compiler.plugin.CompilerPluginRegistrar | ||
| import org.jetbrains.kotlin.compiler.plugin.ExperimentalCompilerApi | ||
| import org.jetbrains.kotlin.config.CompilerConfiguration | ||
| import org.jetbrains.kotlin.config.messageCollector | ||
| import org.jetbrains.kotlin.fir.extensions.FirExtensionRegistrarAdapter | ||
| import org.jetbrains.kotlin.formver.common.* | ||
| import org.jetbrains.kotlin.cli.common.messages.CompilerMessageSeverity | ||
| import org.jetbrains.kotlin.formver.plugin.compiler.FormalVerificationPluginExtensionRegistrar | ||
|
|
||
| @OptIn(ExperimentalCompilerApi::class) | ||
|
|
@@ -42,6 +44,7 @@ class FormalVerificationPluginComponentRegistrar : CompilerPluginRegistrar() { | |
| logLevel, errorStyle, behaviour, conversionSelection, verificationSelection, | ||
| checkUniqueness | ||
| ) | ||
| configuration.messageCollector.report(CompilerMessageSeverity.INFO, "Formal verification plugin: $config") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ligee Does this look reasonable? I am a bit concerned about adding a diagnostic to every build that uses our plugin. |
||
| FirExtensionRegistrarAdapter.registerExtension(FormalVerificationPluginExtensionRegistrar(config)) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,8 +60,7 @@ class FormVerGradleSubplugin | |
| } | ||
| } | ||
|
|
||
| override fun getCompilerPluginId(): String = | ||
| "${BuildConfig.COMPILER_PLUGIN_GROUP}.${BuildConfig.COMPILER_PLUGIN_NAME}" | ||
| override fun getCompilerPluginId(): String = FormalVerificationPluginNames.PLUGIN_ID | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my understanding: why is this the correct place to adjust? (If the problem is because of a mismatch between two places, why make the change at A instead of B?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old code produced
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I think I see. Invalidating existing configurations isn't a big deal (we have zero users). Is it okay that the PLUGIN_ID here ends up being shared by the gradle plugin and the compiler plugin? My understanding is that PLUGIN_ID was intended to refer to the gradle one. |
||
|
|
||
| override fun getPluginArtifact(): SubpluginArtifact = | ||
| SubpluginArtifact( | ||
|
|
||
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.
Why not just make this a data 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.
It seemed like the safer option for permanent modification, but after a bit of research, this is unsupported. Should I change it?
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 please