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

Support JetBrains 2024.1 EAP releases #553

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Support JetBrains 2024.1 EAP releases #553

merged 1 commit into from
Feb 7, 2024

Conversation

jansorg
Copy link
Collaborator

@jansorg jansorg commented Feb 2, 2024

Closes #535

This adds compatibility with 2024.1 eap. Compilation did not show problems and all tests passed.
I'm waiting for 2024.1 eap3 to be fully published before making this PR available for review.

@jansorg jansorg self-assigned this Feb 2, 2024
@apotterri
Copy link
Contributor

@jansorg What's the reason for having ideVersion and ideVersionVerifier? When are each of them used?

@jansorg
Copy link
Collaborator Author

jansorg commented Feb 2, 2024

@apotterri ideVersion defines the SDK used for compilation and for tasks like runIde, CI is overriding it with -PideVersion=... to build and test against a specific version.
ideVersionVerifier is used for the JetBrains plugin verifier, which checks binary compatibility for each version contained in this property. Because it's a list we're not using ideVersion here.

@apotterri
Copy link
Contributor

Why does CI override it? When do you choose to change what's in gradle.properties and what's used by CI?

@jansorg
Copy link
Collaborator Author

jansorg commented Feb 2, 2024

Please take a look at the GitHub Actions of this repo for the details.
CI is overriding the version for Windows because an earlier version is failing for unknown reasons on Windows with the plugins.

Compilation for releases needs to happen with the earliest version for binary compatibility. That's why it's enabled by default.
Later versions in gradle.properties are enabled locally during development to verify compilation and to test or reproduce a particular issue.

@apotterri
Copy link
Contributor

I have looked at the GH action. It's pretty thin on details, which is why I asked the question.... ;)

Ah, so ideVersion is serving double duty: it's used to specify binary compatibility, but it's also the version that's tested against (unless it's overriden, as for Windows). Would it make sense to add a matrix entry that tests a newer (maybe latest) ideVersion?

I'm asking because I added a test to the agent to ensure that it works with the plugin: https://github.com/getappmap/appmap-java/blob/65b816676a7052fdfa56b5d535a548d0fe107425/agent/test/intellij/intellij.bats#L35C17-L35C41 . It pins ideVersion to a version that failed with 1.26.2, because the oldest ideVersion didn't fail. I didn't feel completely comfortable with the pin, and I wondered if it was the right thing to do.

@jansorg
Copy link
Collaborator Author

jansorg commented Feb 5, 2024

Glad to know why you're asking :)

In general ideVersion defines the version of the SDK the plugin is built for. For releases we have to built against the earliest supported SDK, due to binary compatibility.

IMHO we have to run tests at least against the SDK used to compile the published plugin. That's what we're doing now.
We could certainly add a built-matrix, e.g. for the earliest and the latest version (tracking now at #557). I'd appreciate it, unless it's increasing the total runtime of a job.

Regarding your bats-core test:
Because the agent is working fine with the earlier JetBrains SDKs, you'll have to instruct Gradle to use a different SDK. I don't think we should change the default in the plugin because a lot during development requires the earliest version we're supporting.
Pinning the version is okay, but it could become outdated when we're dropping supported SDKs from the plugin.

The possible ways to test with appmap-java I can think of:

  1. Run plugin tests in a matrix against a nightly build on appmap-java
  2. Run plugin tests in a matrix against, then take upstream appmap-java, build it and use it for our tests
  3. appmap-java runs tests against the JetBrains plugin, just like your test

WDYT?

Copy link

github-actions bot commented Feb 5, 2024

AppMap runtime code review

Summary Status
Failed tests ✅ All tests passed
API changes 0️⃣ No API changes
Security flaws ✅ None detected
Performance problems ✅ None detected
Code anti-patterns ✅ None detected
New AppMaps 0️⃣ No new AppMaps

@ahtrotta ahtrotta merged commit ae517a8 into develop Feb 7, 2024
6 checks passed
@ahtrotta ahtrotta deleted the jansorg/224-eap branch February 7, 2024 19:33
@appland-release
Copy link
Contributor

🎉 This PR is included in version 0.56.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Add compatibility with 2024.1 EAP
4 participants