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

Add multiplatform artifact #103

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

serjsysoev
Copy link
Collaborator

This pull request is a third one in the series of PRs that will add Kotlin Multiplatform support (previous: #101, #102). This PR adds multiplatform artifact.

@serjsysoev serjsysoev force-pushed the multiplatform-final branch from f853857 to 6702390 Compare April 26, 2024 11:11
@amaembo amaembo self-assigned this May 3, 2024
@amaembo
Copy link
Collaborator

amaembo commented May 3, 2024

I've checked that the resulting Java artifacts are mainly identical to the previous ones, which is good. I have a few questions though. Are we going to deploy/distribute the Kotlin JVM artifact? Does it make sense, or Java JVM artifact is enough? If yes, then should we probably name the JPMS module differently for Kotlin JVM artifact? Currently, it's named exactly the same (org.jetbrains.annotations) as Java one, which might be confusing for users. Second, what is the target JVM version for this artifact? I see that some annotations are compiled with Java 8 target, while some other (e.g., Flow) are compiled for some reason with Java 11 target. E.g.:

javap.exe -cp multiplatform-annotations-jvm-25.0.0-SNAPSHOT.jar -v org.intellij.lang.annotations.Flow

This command shows major version 55 which corresponds to Java 11.

  Last modified 1 Feb 1980; size 1050 bytes
  SHA-256 checksum 0d19432f9aecd82ec5c10435f7e89a684da55bbddcaefe4d4c96a10a63da17ce
  Compiled from "Flow.java"
public interface org.intellij.lang.annotations.Flow extends java.lang.annotation.Annotation
  minor version: 0
  major version: 55

Is it necessary to have 11 as the minimal JVM? If yes, it probably should be documented somewhere.

@serjsysoev
Copy link
Collaborator Author

I think it is necessary for us to distribute Kotlin JVM artifacts too, since unfortunately they are not equivalent to Java JVM artifacts (they add some meta information). When Kotlin artifact is used in common code, Kotlin JVM artifact will be used for compilation into JVM.
We could rename JPMS module, but then analysis in IntelliJ won't work for the new artifact, they only work for org.jetbrains.annotations. We either have to rewrite all analysis to also include org.jetbrains.multiplatform_annotations, or keep the the package name. If we go the first route, we might as well create a separate repository for those new annotations, since they will be independent.
I don't think keeping the package name would be a huge problem. It will be the same story as with Java 5 and Java 8 org.jetbrains.annotations. Two artifacts with the same package with different annotations. We will keep Kotlin artifacts backwards compatible to Java artifacts (it will be possible to switch from Java artifact to Kotlin, but not vice versa). This means that users of Java artifacts will continue using them without any problems, users who want to make their project multiplatform will have the ability to do so by switching to Kotlin org.jetbrains.annotations. The only problematic scenario is if you want to switch back to Java org.jetbrains.annotations, but I don't know why you would want to do this...

Regarding the second question, it's unexpected, I set kotlinOptions.jvmTarget = "1.8", will investigate it.

@serjsysoev
Copy link
Collaborator Author

serjsysoev commented May 4, 2024

Found the cause of the issue. Turns out kotlinOptions.jvmTarget = "1.8" doesn't apply to .java files in JVM part of Kotlin Multiplatform project. I've explicitly set jvmToolchain(8) and that fixed the issue.

@amaembo
Copy link
Collaborator

amaembo commented May 6, 2024

We either have to rewrite all analysis to also include org.jetbrains.multiplatform_annotations, or keep the the package name. If we go the first route, we might as well create a separate repository for those new annotations, since they will be independent.

I don't suggest renaming the package, only JPMS module. Do you think any analysis depends on the JPMS module name?

@serjsysoev
Copy link
Collaborator Author

Oh, I misunderstood you, renamed the JPMS module to multiplatform_annotations

@amaembo amaembo merged commit 8dded6b into JetBrains:master May 10, 2024
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.

2 participants