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

feat: add jpms support with module-info.java #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sgammon
Copy link

@sgammon sgammon commented Mar 1, 2024

Summary

This PR adds a module-info.java definition, bringing kotlinx-collections-immutable to full JPMS support; downstream users can use the new module with the following directive:

module-info.java

module your.module.here {
  requires kotlinx.collections.immutable;
}

Fixes and closes #157

Other changes

In general, the build was cleaned up, and Dependency Locking and Dependency Verification are now turned on, as is the case for other Kotlin projects.

Trying it out

This is still a draft because it is being tested downstream. If you would like to test this module, you can do so with:

build.gradle.kts

dependencies {
  implementation("org.jetbrains.kotlinx:kotlinx-collections-immutable:0.3.8-rc-1")
}

settings.gradle.kts

dependencyResolutionManagement {
  repositories {
    mavenCentral()

    // 👇  this repo
    maven {
      name = "elide-snapshots"
      url = uri("https://elide-snapshots.storage-download.googleapis.com/repository/v3/")
    }
  }
}

Changelog

  • feat: add module-info.java
  • feat: add Java9Modularity used by other kotlinx projects
  • feat: build core project as multi-release jar
  • chore: upgrade gradle → 8.6 (to run under jvm21)
  • chore: upgrade kotlin → 1.9.22
  • chore: general build cleanup (use kotlin for most build scripts)
  • chore: add gradle lockfiles and locking config
  • chore: add gradle dependency verification config

- feat: add `module-info.java`
- feat: add `Java9Modularity` used by other kotlinx projects
- feat: build `core` project as multi-release jar
- chore: upgrade gradle → `8.6` (to run under jvm21)
- chore: general build cleanup (use kotlin for most build scripts)
- chore: add gradle lockfiles and locking config
- chore: add gradle dependency verification config
@sgammon sgammon marked this pull request as ready for review March 1, 2024 05:20
Comment on lines -6 to +7
implementation "org.jetbrains.kotlin:kotlin-stdlib"
implementation 'org.openjdk.jmh:jmh-core:1.21'
implementation libs.kotlin.stdlib
implementation libs.jmh.core
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General build cleanup: moved dependencies to a version catalog.

Comment on lines +50 to +52
lockAllConfigurations()
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency locking and verification were activated, but kept in lenient mode for now, to account for native dependency shift in CI.

Comment on lines +30 to +33
object Java9Modularity {
@JvmStatic
@JvmOverloads
fun Project.configureJava9ModuleInfo(multiRelease: Boolean = true) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java9Modularity added from here

Comment on lines -9 to +13
archivesBaseName = "kotlinx-collections-immutable" // doesn't work
archivesName = "kotlinx-collections-immutable"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes deprecation

@@ -1,3 +1,7 @@
@file:OptIn(org.jetbrains.kotlin.gradle.targets.js.dsl.ExperimentalWasmDsl::class)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes warnings

Comment on lines +1 to +4
module kotlinx.collections.immutable {
requires java.base;
requires transitive kotlin.stdlib;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New module-info.java descriptor. I thought about adding it as kotlinx.collections.immutable.jvm, which is the suggested module name in IntelliJ; but I don't think users are using this value yet, because I can't locate it anywhere as an Automatic-Module-Name.

Thus, the format here matches other modules, like:

  • kotlinx.coroutines.[core|guava|...]
  • kotlinx.serialization.[core|json|...]

Comment on lines +1 to +6
[versions]
kotlin-sdk = "1.9.22"
kotlinx-benchmark = "0.4.10"
kotlinx-validator = "0.13.2"
guava-testlib = "18.0"
jmh = "1.21"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version catalog; other than updating kotlin-sdk to 1.9.22, no version changes were applied.

distributionUrl=https\://services.gradle.org/distributions/gradle-7.4.2-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gradle upgrade → latest stable so that this project can run under JVM 21.

Comment on lines +1 to +2
pluginManagement {
repositories {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote this file as Kotlin because it helps a bit with build speed, since Groovy build scripts can't be build cached until Gradle 8.7, which is at RC2 (not yet released).

@sgammon
Copy link
Author

sgammon commented Mar 4, 2024

repositories {
mavenCentral()
mavenLocal()
if (project.hasProperty("kotlin_repo_url")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the nullable findProperty method to check and retrieve property. The properties property performs a copy of the underlying map.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, although this block wasn't supposed to make it in at all; I'll drop

@daniel-jasinski
Copy link

Good PR, but there is a lot of housekeeping and the original intention gets lost in all the minutiae.
I would suggest splitting this PR, at least into the chore and feature parts.

Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this project contains only one module where module-info should be added, it would be better to avoid introducing an included build with common logic like buildSrc - it complicates build and increases configuration time.

It's enough to add a compilation task directly in the core subproject, like it is done in kotlinx.datetime

We can refactor it later to shared build logic if we introduce another published module.

@sgammon
Copy link
Author

sgammon commented Mar 6, 2024

@daniel-jasinski Thank you for that review

@ilya-g Okay, no problem. I will apply those changes. Thank you as well

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.

Provide module-info descriptor for java 9 and later
3 participants