Migrate servers from tapir-loom #3304

kciesielski · 2023-11-07T13:27:39Z

This PR migrates netty-loom and Helidon Níma servers from https://github.com/softwaremill/tapir-loom/, which is about to become archived.

Builds are reconfigured: a new java axis was added to the matrix, with values of 11 and 21.
The goal is to build and release all modules with JDK 11, except 2 new modules: netty-server-loom and nima-server.
These 2 ought to be built, tested, and released using JDK 21.

One disadvantage of such setup is running compileDocumentation, which has to choose its scope (JDK11) and cannot see these 2 new modules. As a result, mdoc code samples referring to them cannot be compiled.

Please carefully review the publish ci job, so that we avoid turbulences with the next version release, which is supposed to be aware of separate needs for JDK 11/21.

Bonus:

This PR also proposes to switch netty from experimental to stabilising.

adamw · 2023-11-07T15:46:30Z

doc/stability.md

@@ -24,7 +24,8 @@ The modules are categorised using the following levels:
 | armeria   | stabilising  |
 | finatra   | stabilising  |
 | http4s    | stabilising  |
-| netty     | experimental |
+| netty     | stabilising  |


adamw · 2023-11-07T15:48:36Z

server/netty-server/loom/src/main/scala/sttp/tapir/server/netty/loom/loom.scala

+
+import sttp.monad.MonadError
+
+package object loom {


maybe this should be an id package?

or ... maybe we can put the whole Id interpreter into the main package? only problem, we'd need to compile it using JDK 21. But maybe we can compile using JDK 21 and target 11 bytecode?

Then we could also share the Id alias across netty-loom / nima

but maybe it's better to keep this separate, I don't know ;)

yeah let's not overcomplicate for now ;)

adamw · 2023-11-07T15:49:28Z

server/netty-server/loom/src/test/scala/sttp/tapir/server/netty/loom/SleepDemo.scala

+
+import sttp.tapir._
+
+object SleepDemo extends App {


this can be removed now?

or ... move the "demos" to examples?

It doesn't seem to add anything more than it's in the docs, so I'll just remove it.

adamw · 2023-11-07T15:59:13Z

.github/workflows/ci.yml

@@ -52,10 +62,13 @@ jobs:
        sudo apt-get update
        sudo apt-get install libidn2-dev libcurl3-dev
        echo "STTP_NATIVE=1" >> $GITHUB_ENV
+    - name: Enable Loom-specific modules
+      if: matrix.java == '21'
+      run: echo "JDK_LOOM=1" >> $GITHUB_ENV


or we can just use https://stackoverflow.com/questions/2591083/getting-java-version-at-runtime in the sbt build?

This env variable makes the project build only jdk21 servers. Checking java version in sbt instead of reading it might cause confusion in local development. For example, I use 21 as my default working JDK, but I still want to build the "JDK11 set". More developers may have such a case. Maybe we should rename the variable to something meaning more "build loom servers only"?

makes sense :) add this info to readme in Contributing?

adamw · 2023-11-07T16:06:47Z

.github/workflows/ci.yml

-    env:
-      STTP_NATIVE: 1
+    strategy:
+      matrix: 


hopefully this will work and the two publishes will publish separate jars - no other way to find out than try ;)

Yeah, unfortunately there's no nice way to do a dry-run. We could probably make publishArtifacts dependent on some variables, but I'm not sure if it's worth adding complexity.

adamw · 2023-11-07T16:09:23Z

.github/workflows/ci.yml

+          - java: "21"
+            target-platform: "Native"
+          - java: "21"
+            target-platform: "JS"


so ... we are trying to only include specific axes? maybe we can either (1) use include instead of exlucde, if possible; (2) if not, at least add a comment, what are the desired combinations that should be included

Thanks for the hint, somehow I wasn't aware that there's include. Turns out it can be combined with exclude, because exclusions are resolved first, so our rules will become:

exclude: - java: "21" - scala-version: "2.12" target-platform: "Native" - scala-version: "2.13" target-platform: "Native" include: - java: "21" scala-version: "2.13" target-platform: "JVM" - java: "21" scala-version: "3" target-platform: "JVM"

Looks clearer to me.

adamw · 2023-11-08T09:26:32Z

.github/workflows/ci.yml

          - scala-version: "2.12"
            target-platform: "Native"
          - scala-version: "2.13"
            target-platform: "Native"
+        include: # Restricted to build only specific Loom-based modules


adamw · 2023-11-08T09:35:06Z

build.sbt

    println("[info] STTP_NATIVE defined, including native in the aggregate projects")
    rawAllAggregates
  } else {
    println("[info] STTP_NATIVE *not* defined, *not* including native in the aggregate projects")
    rawAllAggregates.filterNot(_.toString.contains("Native"))
  }
+  if (sys.env.isDefinedAt("JDK_LOOM")) {


hm maybe this should be called ONLY_LOOM or sth like that? otherwise it sounds additive

- To generate consistend build name

kciesielski added 12 commits November 7, 2023 14:26

Import tapir-netty-loom code

e418a0e

Configure CI to accommodate for JDK11 vs JDK21

76dfa35

Remove leftover empty job

25b4106

Fix Java version references

9a03a46

Filter out steps unnecessary for loom modules

49af16b

Migrate Helidon Nima server

1492132

Document netty-loom

a52779a

Don't build new servers on Scala 2.12

dcf8d22

Silence warning

8fe4593

Update docs

a3f6643

Exclude Scala [2.12 + Java21] from the matrix

ee87d44

Fix exclusion

e5ab8f2

kciesielski marked this pull request as ready for review November 7, 2023 15:05

kciesielski requested review from adamw and DybekK November 7, 2023 15:06

kciesielski changed the title ~~[WIP] Migrate servers from tapir-loom~~ Migrate servers from tapir-loom Nov 7, 2023

kciesielski added Netty server enhancement New feature or request documentation labels Nov 7, 2023

adamw reviewed Nov 7, 2023

View reviewed changes

Review fixes

10c944c

kciesielski linked an issue Nov 8, 2023 that may be closed by this pull request

Move tapir-nima into tapir #3267

Closed

Test graceful shutdown for Netty-Loom

d33aff8

adamw reviewed Nov 8, 2023

View reviewed changes

adamw approved these changes Nov 8, 2023

View reviewed changes

kciesielski added 3 commits November 8, 2023 11:53

Rename JDK_LOOM to ONLY_LOOM and fix detecting Nima

87235c3

Document JDK version nuances

5273d36

Change order of axes

bc70301

- To generate consistend build name

kciesielski merged commit 4772b97 into master Nov 8, 2023
15 checks passed

mergify bot deleted the netty-loom branch November 8, 2023 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate servers from tapir-loom #3304

Migrate servers from tapir-loom #3304

kciesielski commented Nov 7, 2023 •

edited

Loading

adamw Nov 7, 2023

adamw Nov 7, 2023

adamw Nov 7, 2023

adamw Nov 7, 2023

adamw Nov 7, 2023

adamw Nov 7, 2023

adamw Nov 7, 2023

kciesielski Nov 8, 2023

adamw Nov 7, 2023

kciesielski Nov 8, 2023

adamw Nov 8, 2023

adamw Nov 7, 2023

kciesielski Nov 8, 2023

adamw Nov 7, 2023

kciesielski Nov 8, 2023

adamw Nov 8, 2023

adamw Nov 8, 2023

Migrate servers from tapir-loom #3304

Migrate servers from tapir-loom #3304

Conversation

kciesielski commented Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kciesielski commented Nov 7, 2023 •

edited

Loading