feat: maven kotlin multiplatform support #412

buenaflor · 2022-09-01T13:00:56Z

This introduces some additions to be able to support a Kotlin Multiplatform publish.

Additional artifacts include: *.klib, *-all.jar, *-metadata.jar, *.module.

Example artifacts of Kermit:

ios artifacts: https://repo1.maven.org/maven2/co/touchlab/kermit-sentry-iosarm64/1.2.0-M2/
root artifacts: https://repo1.maven.org/maven2/co/touchlab/kermit-sentry/1.2.0-M2/

For context: for a KMP publish all platforms need to be published and the root artifact contains metadata, see: getsentry/sentry-kotlin-multiplatform#37 (comment)

The generated gpg command example:
root artifact:

"-Dfile=/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-mSNdRu/sentry-kotlin-multiplatform-0.0.1/sentry-kotlin-multiplatform-0.0.1.jar" 
"-Dfiles=/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-mSNdRu/sentry-kotlin-multiplatform-0.0.1/sentry-kotlin-multiplatform-0.0.1-javadoc.jar,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-mSNdRu/sentry-kotlin-multiplatform-0.0.1/sentry-kotlin-multiplatform-0.0.1-sources.jar,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-mSNdRu/sentry-kotlin-multiplatform-0.0.1/sentry-kotlin-multiplatform-0.0.1-all.jar,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-mSNdRu/sentry-kotlin-multiplatform-0.0.1/sentry-kotlin-multiplatform-0.0.1.module" 
"-Dclassifiers=javadoc,sources,all,module" 
"-Dtypes=jar,jar,jar,module" 
...

ios artifact:

"-Dfile=/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosArm64-0.0.1.klib" 
"-Dfiles=/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosArm64-0.0.1-javadoc.jar,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosArm64-0.0.1-sources.jar/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosarm64-0.0.1-cinterop-Sentry.NSException.klib,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosarm64-0.0.1-cinterop-Sentry.Scope.klib,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosarm64-0.0.1-cinterop-Sentry.klib,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosArm64-0.0.1-metadata.jar,/var/folders/j2/r3dsjfkx4pj5n0v8jjlb6lv80000gn/T/craft-QEa8yL/sentry-kotlin-multiplatform-iosArm64-0.0.1/sentry-kotlin-multiplatform-iosArm64-0.0.1.module" 
"-Dclassifiers=javadoc,sources,cinterop,cinterop,cinterop,metadata,module"
"-Dtypes=jar,jar,klib,klib,klib,jar,module"
...

src/targets/maven.ts

iker-barriocanal

The craft codebase uses semicolons, can we follow the same style?

iker-barriocanal · 2022-09-01T15:14:34Z

src/targets/maven.ts

+    if (existsSync(join(distDir, `${moduleName.toLowerCase()}.klib`))) {
+      return `${moduleName}.klib`;
+    }


If the file isn't accessible (e.g. it doesn't exist), the .jar filename will be returned. I don't know if the .jar file exists for kotlin, or what the consequences are of uploading it instead of the .klib. If a required file for a release doesn't exist, we should fail the release.

Another thing to consider is how easy it'd be to debug a failing release. A message like "hey, I can't find xxx file" is more helpful than "hey, I'm uploading the files and can't read yyy file, which isn't related to the release at all".

An alternative approach is to update the target's config to keep track of what we are releasing (android? kotlin?). If we know that, we could return directly the .klib filename, and let the upload fail if it can't read the file (which automatically produces a more helpful failure message). This is what currently happens with the .jar file (no disk operations happen in this method).

Kotlin Multiplatform needs to upload multiple artifacts, you upload either a .jar or a.klib or .aar in case of Android. so if it's not an android artifact then if .klib is not there then it must be a .jar. So it shouldn't fail if it doesn't find a .klib.

Since only Apple targets (ios, macos, etc...) are packaged as .klib, I can add a target config like you said to keep track of those so I don't have to search for it's existence. However, I would still need to search for the existence of *.metadata and *-all.jar in getFilesForMavenPomDist as they have to be added and they don't exist for every artifact (only the root artifact needs all.jar and only apple artifacts have metadata.jar).

I guess what I can do is create a target config for kotlin-multiplatform or something like that to keep the uploadPomDistribution separated and only add those files if it's a kotlin-multiplatform artifact. that way it doesn't interfere with the way it has been before I added the changes

src/targets/maven.ts

iker-barriocanal

Looks much better!

Also -- don't forget to add documentation for this new config in the README.

src/targets/maven.ts

README.md

marandaneto · 2022-09-08T10:49:48Z

@buenaflor did you try to call gpg:sign-and-deploy-file manually with all those params and check the temporary ~/.m2/repository/io/sentry folder if it's correct?
It's also worth checking if the new files are also signed, because all the files need to be, you can see that each file has a .asc, .sha, .md5 extension, so the klib, module etc all need to be signed as well but I believe the command does it automatically, I'd double-check tho.

iker-barriocanal

Tests currently only cover config loading. Put simply, what this PR introduces is spawning a process with a list of arguments executed under a certain condition (when we make a kotlin multiplatform release, run gpg blahblahblah). Can we verify that the process is spawned, when it should and with the expected arguments?

romtsn · 2022-09-26T06:45:04Z

@iker-barriocanal what do you think is still missing for this PR? just tests from your latest comment or some other comments need to be addressed?

iker-barriocanal · 2022-09-27T08:34:29Z

@romtsn I think only tests are missing (verifying the temporary folder as Manoel suggests may be one of them). That said, I'm not blocking the PR. I don't know if @asottile-sentry has other thoughts too.

marandaneto · 2022-09-27T09:09:25Z

Making it a draft since it's not fully tested yet.

buenaflor · 2023-03-08T16:39:01Z

I added a fix so that the published artifacts are correct and signed. I generated the artifacts manually & locally by calling the gpg publish commands as they were produced by craft during dry runs (I only had to change the output url and the path to the distribution folder). Then tested it on sample kmp apps with multiple platforms (android, cocoa, jvm desktop) by adding it as a dependency throughmavenLocal.

Example output of a specific file:

sentry-kotlin-multiplatform-iossimulatorarm64-0.0.1-cinterop-Sentry.PrivateSentrySDKOnly.klib
sentry-kotlin-multiplatform-iossimulatorarm64-0.0.1-cinterop-Sentry.PrivateSentrySDKOnly.klib.asc
sentry-kotlin-multiplatform-iossimulatorarm64-0.0.1-cinterop-Sentry.PrivateSentrySDKOnly.klib.md5
sentry-kotlin-multiplatform-iossimulatorarm64-0.0.1-cinterop-Sentry.PrivateSentrySDKOnly.klib.sha1

I attached a file with all the generated gpg publish commands for doing it locally for the latest KMP build: kmp local publish via gpg sign deploy.txt

README.md

marandaneto · 2023-03-09T07:55:59Z

LGTM from my side but I'd let the owners of this repo approve it.

asottile-sentry

buenaflor added 5 commits August 31, 2022 05:41

Implement klib support

2cc0828

Change classifier

4c7901c

Use existsSync

f77ab81

Add more side artifacts

6abd815

Fix casing of file names

09201e2

buenaflor requested a review from iker-barriocanal September 1, 2022 13:00

asottile-sentry reviewed Sep 1, 2022

View reviewed changes

src/targets/maven.ts Outdated Show resolved Hide resolved

iker-barriocanal reviewed Sep 1, 2022

View reviewed changes

buenaflor added 5 commits September 2, 2022 01:07

Add target config for kmp

bcb16ba

Format code

f0a5d01

Separate default maven pom dist and kmp pom dist

bd5dd4d

Fix failing tests

22d7f53

Make kotlin multiplatform target config optional

a24edbf

iker-barriocanal reviewed Sep 5, 2022

View reviewed changes

src/targets/maven.ts Outdated Show resolved Hide resolved

src/targets/maven.ts Outdated Show resolved Hide resolved

buenaflor added 6 commits September 5, 2022 09:02

Separate upload kmp pom distribution to another function

8123dcb

Fix build error

a6835fa

Update README to include Kotlin Multiplatform

8a0e2e7

Add tests

a39cc68

Fix test

8868102

Fix test

74099a1

iker-barriocanal reviewed Sep 6, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

buenaflor marked this pull request as ready for review September 6, 2022 09:27

buenaflor requested a review from iker-barriocanal September 6, 2022 09:28

iker-barriocanal reviewed Sep 9, 2022

View reviewed changes

adinauer mentioned this pull request Sep 14, 2022

Gradle pom.xml incorrect metadata redirection getsentry/sentry-java#2233

Closed

marandaneto marked this pull request as draft September 27, 2022 09:08

adinauer assigned romtsn Sep 28, 2022

marandaneto mentioned this pull request Oct 14, 2022

Switch maven target from using maven to gradle as build system #404

Closed

buenaflor added 2 commits March 6, 2023 10:42

Merge branch 'master' into feat/maven-klib-support

56f13a6

fix transforming side artifacts

1833f48

buenaflor marked this pull request as ready for review March 8, 2023 16:39

buenaflor requested a review from iker-barriocanal March 8, 2023 16:40

buenaflor self-assigned this Mar 8, 2023

buenaflor requested a review from asottile-sentry March 8, 2023 16:41

marandaneto reviewed Mar 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

asottile-sentry approved these changes Mar 10, 2023

View reviewed changes

change kotlinMultiplatform to kmp

4e25003

buenaflor merged commit 8f84181 into master Mar 13, 2023

buenaflor deleted the feat/maven-klib-support branch March 13, 2023 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: maven kotlin multiplatform support #412

feat: maven kotlin multiplatform support #412

buenaflor commented Sep 1, 2022 •

edited

Loading

iker-barriocanal left a comment

iker-barriocanal Sep 1, 2022

buenaflor Sep 1, 2022

buenaflor Sep 1, 2022

iker-barriocanal left a comment

marandaneto commented Sep 8, 2022 •

edited

Loading

iker-barriocanal left a comment

romtsn commented Sep 26, 2022

iker-barriocanal commented Sep 27, 2022

marandaneto commented Sep 27, 2022

buenaflor commented Mar 8, 2023 •

edited

Loading

marandaneto commented Mar 9, 2023

asottile-sentry left a comment

feat: maven kotlin multiplatform support #412

feat: maven kotlin multiplatform support #412

Conversation

buenaflor commented Sep 1, 2022 • edited Loading

iker-barriocanal left a comment

Choose a reason for hiding this comment

iker-barriocanal Sep 1, 2022

Choose a reason for hiding this comment

buenaflor Sep 1, 2022

Choose a reason for hiding this comment

buenaflor Sep 1, 2022

Choose a reason for hiding this comment

iker-barriocanal left a comment

Choose a reason for hiding this comment

marandaneto commented Sep 8, 2022 • edited Loading

iker-barriocanal left a comment

Choose a reason for hiding this comment

romtsn commented Sep 26, 2022

iker-barriocanal commented Sep 27, 2022

marandaneto commented Sep 27, 2022

buenaflor commented Mar 8, 2023 • edited Loading

marandaneto commented Mar 9, 2023

asottile-sentry left a comment

Choose a reason for hiding this comment

buenaflor commented Sep 1, 2022 •

edited

Loading

marandaneto commented Sep 8, 2022 •

edited

Loading

buenaflor commented Mar 8, 2023 •

edited

Loading