JetBrains / java-annotations

Annotations for JVM-based languages.
Apache License 2.0
404 stars 47 forks source link

Add multiplatform artifact #103

Closed serjsysoev closed 5 months ago

serjsysoev commented 5 months ago

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.

amaembo commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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