Separate swift-jni package from swift-java

[marcprux]

This PR follows on from https://forums.swift.org/t/android-java-and-swiftjni/78890 and various discussions in the Android Workgroup. It relocates the low-level JNI types (theJavaValue module and the JavaVirtualMachine parts of SwiftJava) and headers (CSwiftJavaJNI) into a separate swift-jni package, which exposes a single zero-dependency SwiftJNI library product that will be used by SwiftJava as well as other JNI-dependent swift packages.

This is purely a refactoring; no logic has been changed in this PR, and all tests pass in the Ubuntu CI and my local macOS machine. There are some sundry Android-related tweaks (e.g., changing import Bionic to import Android) needed to test building for Android; CI for Android is forthcoming. Also vendoring the JNI headers enables us to clean up the Package.swift of swift-java and the various sample projects.

Note

Note that this PR's swift-jni package is provisionally in a sub-directory referenced from swift-java's Package.swift as a .package(path: "swift-jni") dependency. This is good enough for testing and iteration of this PR; a final step will be to move it out to a separate top-level package which will be separately versioned.

[ktoso]

Thanks Marc! This looks good however I'd like us to explore if we truly need to make a separate package or not.

Here's some ideas to discuss

Package trait SwiftJavaMacrosSupport

We could use package traits (Swift 6.1+) to not even depend on swift-syntax if macros support is disabled for example.

This way you could disable and not even resolve swift-syntax and friends if you don't want it:

    .package(
        url: "https://github.com/swiftlang/swift-java.git",
        from: "1.0.0",
        traits: [] // disabled the macros trait!
    ),
]

The downside would be that that's a Swift 6.1+ feature, could this be acceptable though? I.e. that from 6.1+ you could use these types, and use them from swift-java like that -- with disabling any of the macro bits.

Need to double check the dependency resolution though... Reached out to @bripeticca to confirm if that's a 6.2 only feature or would be good enough with 6.1 as well 🤔

Update: Yeah, sadly Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution is a 6.2 feature... so even with traits we'd be pulling down swift syntax 😢 So if we have to support y'all on 6.2 it seems there's no other choice but to spin off a new package, just like you're proposing.

Separate package after all

An additional package is something we could do, but I'd like to exhaust all other options which come with a lesser maintenance burden.

[finagolfin]

There are no plans for an official 6.1 bundle, so 6.2 with traits should work.

[ktoso]

That would be preferable, so we don't have to split up artificially into separate packages.
It would take the shape of something like #386 (not finished, first I want to make sure it's viable)

[marcprux]

Update: Yeah, sadly Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution is a 6.2 feature... so even with traits we'd be pulling down swift syntax 😢 So if we have to support y'all on 6.2 it seems there's no other choice but to spin off a new package, just like you're proposing.

That is indeed unfortunate, but even is this was working, the traits solution would have other issues. For example, from the draft PR:

    .default(enabledTraits: [
       SwiftJavaMacrosSupport
    ]) // enabled by default, but downstream libraries may disable macros if they don't use them

Skip itself can't have a dependency on a package with macros, but at the same time, we don't want to make it expressly incompatible. That is to say, a Skip package can work alongside swift-java and their macros (and we fully expect/hope that many projects will indeed use swift-java as their mechanism for integrating various parts of the Android SDK or third-party Java code), but Skip cannot depend on the macros package. Since traits are globally merged, we wouldn't be able to disable the SwiftJavaMacrosSupport trait from Skip while also leaving open the ability for other dependencies to enable them.

Aside from macros and swift-syntax, a number of other dependencies that the swift-java package brings in are non-starters for us, including package-benchmark, jemalloc, hdrhistogram-swift, swift-argument-parser, swift-atomics, swift-numerics, and swift-system.

While I understand the reservations about adding another top-level package and the potential maintenance complexity that might arise from that, I think it is beneficial to have a simple zero-dependency and implementation-agnostic module whose scope is limited to providing an interface to the common JNI features that are needed by a variety of different bridging solutions (not just Skip's, but the dozen-or-so other Swift/Java integration layers that have evolved over the past 10 years). Also, a swift-jni package with a narrow scope has the potential to have a stable 1.0 release much sooner, while swift-java can take its time to "get things right" before committing to a release. This would fit our time-table better, since there are real-world apps being shipped right now that would benefit from the interoperability that a common JNI layer would facilitate.

[ktoso]

I understand the need, but still, we have to fully explore the solution space, so please bear with me here.

Aside from macros and swift-syntax, a number of other dependencies that the swift-java package brings in are non-starters for us, including package-benchmark, jemalloc, hdrhistogram-swift, swift-argument-parser, swift-atomics, swift-numerics, and swift-system.

• benchmark, hdrhistogram-swift, and jemalloc are just for benchmarks, easily a trait that's off by default
• swift-argument-parser, swift-system are because of the command line tool -- I wonder if we can figure out a way to only enable these selectively... The use of the jextract swiftpm plugin would have to enable it, but otherwise it's not used code at all at runtime 🤔 I'm not sure that's possible to pull off, would you have some insight here @bripeticca ?

The others I'm not sure and we'd have to check.

Would you mind clarifying why they are non-starters. It's stated outright but I don't see the reason given, just so we're on the same page.

Again, if we end up in a world with a new package that's okey, but I want us to have the full picture first, and to be able to say "if we had ... we would not have had to do this", which may inform feature development on traits etc.

[bripeticca]

@ktoso Just catching up a bit on the thread here - like @marcprux mentioned, traits are unified across the entire package graph. That is, if I have a root package that disables a dependency's trait but this same trait is enabled by another dependency elsewhere, then the result would be that the trait for that package dependency is enabled in the package graph - SwiftPM calculates the union of enabled traits per package across the entire graph.

[marcprux]

Would you mind clarifying why they are non-starters. It's stated outright but I don't see the reason given, just so we're on the same page.

We are very parsimonious about dependencies. A stock Skip app doesn't have any non-Skip SwiftPM dependencies by default (swift-jni would be the first). They can be added by app developers — either directly, or through our various integration frameworks — but our philosophy that that the baseline project structure must contain as few dependencies as possible.

Aside from aesthetics, there is a practical concern: until we have an official Swift Android SDK release and a commitment that packages will be maintained for Android, we cannot risk some package with a low-level dependency (like, say, swift-atomics) breaking out from under us. As our native Android support has evolved, we have on a number of occasions needed to temporarily rely on forks of various common packages until PRs have been accepted, merged and trickled into official releases. But if we needed to keep a swift-android-sdk/swift-atomics port working on Android, that would conflict with a swiftlang/swift-atomics if we relied on the transitive dependency from swift-java.

[ktoso]

Thanks for the discussion folks. I think we'll need to do the separate package indeed -- I'll look into making this happen soon. I'll loop back here with more info!

Thank you for the PR, it helps kick off the package nicely. Probably doesn't make sense to merge this specific PR, but instead: do the new package, adjust this PR (or make a new one).

[marcprux]

Great! Once a new top-level repository gets created, I'll be happy to break up this PR into two separate PRs for the two packages if that would help.