-
Notifications
You must be signed in to change notification settings - Fork 22
fix: Download LSP bundle with nodejs runtime #26
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
fix: Download LSP bundle with nodejs runtime #26
Conversation
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
7de6c1b to
e9b8434
Compare
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
.github/workflows/IJ.yml
Outdated
| strategy: | ||
| matrix: | ||
| IJ: [IC-2018.1, IC-2018.2, IC-2018.3, IC-2019.1, IC-2019.2, IC-2019.3, IC-2020.1, IC-2020.2, IC-2020.3] | ||
| IJ: [IC-2019.1, IC-2019.2, IC-2019.3, IC-2020.1, IC-2020.2, IC-2020.3] |
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 is the rationale for removing those versions ?
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.
Because those weren't supporting new APIs Service and PersistentState. Do we really need to support those old versions as well?
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.
We will add telemetry that will give use versions of IJ that are just. Before that, keep current state
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.
@jeffmaury how about keep the supported version in sync with tekton? That seems reasonable to me. WDTY?
https://github.com/redhat-developer/intellij-tekton/blob/master/.github/workflows/IJ.yml#L17
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.
As I told you, the motivation to change supported version will be based on telemetry. Let me know if you can't cope with the current state and I will do the job to adapt your PR
| this.lspBundleName = lspBundleName; | ||
| } | ||
|
|
||
| static Platform detect(Properties systemProperties) { |
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.
Not required, IntelliJ SDK provides com.intellij.openapi.util.SystemInfo
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.
Ack!
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.
Fixed it.
| <externalAnnotator id="LSPAnnotator-xml" language="XML" implementationClass="org.wso2.lsp4intellij.contributors.annotator.LSPAnnotator"/> | ||
| <externalAnnotator id="LSPAnnotator-json" language="JSON" implementationClass="org.wso2.lsp4intellij.contributors.annotator.LSPAnnotator"/> | ||
| <externalAnnotator id="LSPAnnotator-txt" language="TEXT" implementationClass="org.wso2.lsp4intellij.contributors.annotator.LSPAnnotator"/> | ||
| <postStartupActivity implementation="org.jboss.tools.intellij.analytics.PostStartupActivity"/> |
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.
The reason for having it implemented as application component is that postStartupActivity is delayed and thus if you have opened documents when you open a project, the LSP may not be started.
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 works. Also intellij doc says application component is deprecated.
https://plugins.jetbrains.com/docs/intellij/plugin-components.html
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 but deprecated does not mean removed. So my concern is that if the file is already opened then seems the LSP is started but I can't see the decorations on the file.
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.
@jeffmaury I see similar issue even without this change. Can you just try adding a dummy line in the manifest to see whether it could decorate? it is a bug for sure, just want to 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.
Not sure to understand. If I comment this line out, then LSP won't be started at all. So If this is like before then we're good.
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
e63e3b9 to
8d09142
Compare
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
4bff267 to
b3048cf
Compare
3313c4e to
f4726c6
Compare
| //LSP plugin causes this task to fail | ||
| buildSearchableOptions.enabled = false | ||
|
|
||
| task downloadFile(type: Download) { |
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.
The download plugin must be removed as well
src/main/java/org/jboss/tools/intellij/analytics/AnalyticsPersistentSettings.java
Outdated
Show resolved
Hide resolved
|
Also while testing it, I got the following error: |
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
f4726c6 to
d89fbb8
Compare
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
|
@jeffmaury rebased and kept 2020.1 as minimum support IJ version. PTAL |
jeffmaury
left a comment
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.
Last piece before we can merge: remove the download plugin from build.gradle
build.gradle
Outdated
| plugins { | ||
| id "org.jetbrains.intellij" version "0.4.21" | ||
| id 'de.undercouch.download' version '3.4.3' | ||
| id 'de.undercouch.download' version '3.4.3' |
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.
To be removed
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.
Done!
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
|
Kudos, SonarCloud Quality Gate passed!
|
jeffmaury
left a comment
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
This PR eliminates the need to have nodejs runtime on the developer machine. It is just a first step towards making IJ analytics plugin in sync with it's vscode counterpart.