Skip to content
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

Update for IDEA 2020.1 #188

Merged

Conversation

carymrobbins
Copy link
Contributor

This updates the build to target IDEA 2020.1. I'm not sure if we want to leave the build.gradle as-is or update it to 2020.1 as I have in this PR. There's certainly value in updating it to verify that the build actually compiles with a recent version of IntelliJ. If we want to ensure cross-IntelliJ compatiblity, we should probably update the CI job to deal with that in some way.

I've titled this PR as WIP as the FileUtilsTest is failing for me. However, HEAD doesn't build for me for other reasons, so if it passes on CI I'd be ok with moving forward.

> Task :test

org.wso2.lsp4intellij.utils.FileUtilsTest > initializationError FAILED
    org.objenesis.ObjenesisException
        Caused by: java.lang.reflect.InvocationTargetException
            Caused by: java.lang.IllegalAccessError

9 tests completed, 1 failed

@CLAassistant
Copy link

CLAassistant commented May 19, 2020

CLA assistant check
All committers have signed the CLA.

@carymrobbins
Copy link
Contributor Author

Just noticed that this intersects with PR #187 which was just opened a few hours ago. However, this PR fixes additional issues that were causing compilation errors when building against 2020.1.

@NipunaRanasinghe
Copy link
Contributor

NipunaRanasinghe commented May 20, 2020

@carymrobbins First of all, I really appreciate your effort on this as I've been busy with few other works and just had a chance to continue this work.
Regarding the idea build version, we've been keeping the minimum compatible version in order to make sure that all our changes + compatibility fixes are doesn't affect the oldest version of the compatible range (and if not, we have to bump the minimum compatible version). But as you mentioned, we also have to validate against the latest stable version as well. So I'm okay with either approach until we figure out a way validate the build against multiple IDEA versions.
(And I can see that with your changes on LSPSymbolContributor, the minimum compatible version won't be 2017.3 anymore. So I'll update the REAME with a compatibility matrix with the next patch release.)

Regarding the test failure, make sure you are using jdk8. Anyway I guess we are all good to merge after resolving the conflicts which caused by #187.

@nixel2007
Copy link
Contributor

Regarding the test failure, make sure you are using jdk8

AFAIR intellij platform 2020.1+ uses jdk11 by default. should we stick with jdk8?

@NipunaRanasinghe
Copy link
Contributor

Regarding the test failure, make sure you are using jdk8

AFAIR intellij platform 2020.1+ uses jdk11 by default. should we stick with jdk8?

Yeah AFAIK default JBRE version for IntelliJ 2020.x versions will be jdk11. I just meant that until we verify that the library is compatible with higher jdk versions (and fix if not), we have to use the jdk version we've been using. BTW have you tried to build with higher jdk versions and encountered any issues?

@nixel2007
Copy link
Contributor

nixel2007 commented May 20, 2020

BTW have you tried to build with higher jdk versions and encountered any issues?

build - no. sorry. but have tested latest versions in 2020.1 with bundled bsl ls without problems.

@carymrobbins
Copy link
Contributor Author

Mind if I rebase to fix the merge conflicts?

@carymrobbins
Copy link
Contributor Author

The conflicts were only because of a change I made to LSPDefaultIconProvider.java, which I've reverted, so we should be all good now.

@carymrobbins carymrobbins changed the title WIP: Update for IDEA 2020.1 Update for IDEA 2020.1 May 20, 2020
@NipunaRanasinghe
Copy link
Contributor

NipunaRanasinghe commented May 21, 2020

The conflicts were only because of a change I made to LSPDefaultIconProvider.java, which I've reverted, so we should be all good now.

perfect! Thanks @carymrobbins. LGTM!
Could you please also verify the oldest IDEA version we can support after the change, so that we can update the compatibility details?

@NipunaRanasinghe NipunaRanasinghe merged commit aea3146 into ballerina-platform:master May 21, 2020
@carymrobbins carymrobbins deleted the idea-2020.1 branch May 21, 2020 12:14
@carymrobbins
Copy link
Contributor Author

Could you please also verify the oldest IDEA version we can support after the change, so that we can update the compatibility details?

I'm not really sure what the best way to go about this. From what I can tell, the method signatures changed in this commit - JetBrains/intellij-community@0ac6e72

That commit appears in these tags -

% cd intellij-community
% git fetch --tags
% git tag --contains 0ac6e724c94bb94ccba51a378d89a4f86af2b8d5
idea/201.4515.24
idea/201.4865.12
idea/201.5259.13
idea/201.5616.10
idea/201.5985
idea/201.5985.32
idea/201.6073.9
idea/201.6251.22
idea/201.6251.9
idea/201.6487.11
idea/201.6668.113
idea/201.6668.121
idea/201.6668.13
idea/201.6668.60
idea/201.7223.18
idea/201.7223.58
idea/201.7223.91
idea/201.7846.29
idea/201.7846.6

So I'm guessing that the minimum compatible IntelliJ would be the 201.4515.24 build, which I believe is some variant of the 2020.1 release.

@NipunaRanasinghe
Copy link
Contributor

@carymrobbins I also tried to build with IntelliJ 2019.3 and it failed, which means that the minimum supported IDEA version with the API change will be 2020.1 itself. @gayanper WDYT?

@gayanper
Copy link
Contributor

@NipunaRanasinghe well this will cause all the plugins that depend on this library to have their minimum version as 2020.1, which is fine by me, but probably not by many. Of course for my plugin I don't see why not upgrade to the latest CE version. But for plugins that are targetting the Ultimate version would be a problem. It's always good to keep the compatibility to at least to version (latest version) - 1 IMO.

@carymrobbins
Copy link
Contributor Author

One thing you could do is have some sort of cross build that targets multiple IDEA versions. You would need to have conditional source directories that are only used when a particular version of IDEA is being built against.

However, I wonder if the plugin will still "just work" when used on an older IDEA version? Unless I'm missing something, the only breaking change was the generic type params, which are subject to erasure and shouldn't produce incompatible JVM bytecode for earlier versions.

@gayanper
Copy link
Contributor

One thing you could do is have some sort of cross build that targets multiple IDEA versions. You would need to have conditional source directories that are only used when a particular version of IDEA is being built against.

However, I wonder if the plugin will still "just work" when used on an older IDEA version? Unless I'm missing something, the only breaking change was the generic type params, which are subject to erasure and shouldn't produce incompatible JVM bytecode for earlier versions.

I think we should have kept them compatible for 2019.1 as well without changing the signatures as it was before, because the same type erasure applies there for 2020.1 as well. Keeping the compilation against 2019.1 is a good balance as a library.

@nixel2007
Copy link
Contributor

2019.1

but latest CE - 1 version is 2019.3...

@gayanper
Copy link
Contributor

gayanper commented May 23, 2020

2019.1

but latest CE - 1 version is 2019.3...

True, what i wanted to say is we need to support the previouse major version (including all 3 minor versions).

@NipunaRanasinghe
Copy link
Contributor

@carymrobbins @nixel2007 please refer the discussion in #184, where I also think that it would be better keep the compatibility at least for the previous set of "major"(2019.x) IntelliJ releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants