-
Notifications
You must be signed in to change notification settings - Fork 47
Inject some identifying properties to the gradle connection #76
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
Conversation
Some builds/plugins depend on identifying gradle "sync" to configure their build properly
@@ -25,6 +25,16 @@ import com.jetbrains.ls.imports.utils.toIntellijUri | |||
import java.io.ByteArrayOutputStream | |||
|
|||
object GradleWorkspaceImporter : WorkspaceImporter { | |||
private const val IDEA_SYNC_ACTIVE_PROPERTY = "idea.sync.active" |
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.
Hi,
Thank you for your contribution!
A few things I need to tell you about the state of Gradle support:
- The import mechanism we're currently using is very different from the one used by IntelliJ
- It lacks many features.
- We're going to replace it with a new implementation, which will be much closer to the one used in IntelliJ, but still might differ in some aspects
To avoid confusion, I would propose to add a new property named something like jetbrains.lsp.sync.version
. We could set it to 1.0
for now and then upgrade to 2.0
after switching to the new mechanism.
Please tell me if this makes sense or not
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.
Thanks! I understand that the implementation is different from that of IntelliJ and will likely remain so to an extent.
The reason I made this PR is that because our gradle build is so massive (~7000 projects) it takes a very long time and a lot of heap to load the entire project into an IDE. Both IntelliJ and this lsp extension use the gradle tooling API to configure the project and then retrieve information about the project from gradle. One way that we optimize this is by trimming projects out of settings.gradle
when the project is loaded in an IDE. A user can select subsections of the repository to work on via some tool (e.g. spotlight which i linked in the description or other tools like focus). Our gradle build then looks during configuration phase to see if it is being invoked in a sync context (using the idea.sync.active
property) to conditionally apply this behavior, which is not needed for builds because we use the configuration on-demand feature.
The reason I included the IntelliJ property is to provide some instant compatibility with these existing tools. To the best of my knowledge none of these tools use this flag to change the gradle model builders, only the project configuration phase, which is shared by both intellij and this lsp extension. I also added a new distinct flag in case it would ever be important to distinguish between the invocation contexts. Making the new property a version is a good idea, IntelliJ/Studio also inject their respective versions into the process.
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.
Thanks for the clarification. I think it's okay to postpone versioning for now, as we don't know what the versioning scheme is going to look like yet. It's never too late to add yet another property, right?
@@ -25,6 +25,16 @@ import com.jetbrains.ls.imports.utils.toIntellijUri | |||
import java.io.ByteArrayOutputStream | |||
|
|||
object GradleWorkspaceImporter : WorkspaceImporter { | |||
private const val IDEA_SYNC_ACTIVE_PROPERTY = "idea.sync.active" | |||
private const val KOTLIN_LSP_IMPORT_PROPERTY = "com.jetbrains.ls.imports.gradle" |
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.
@strangepleasures do you have any feedback about the name of this property? I just based it on the package name of this code, but your suggestion for the injected version was a bit different. Would you prefer jetbrains.lsp.sync=true
?
Some builds/plugins depend on identifying gradle "sync" to configure their build properly
for example: https://github.com/joshfriend/spotlight/blob/d1903ac03a780278743a230f674d0c2aa7992204/spotlight-gradle-plugin/src/main/kotlin/com/fueledbycaffeine/spotlight/utils/IdeProperties.kt#L6-L12