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: kotlin 1.9.22 #56

Merged
merged 10 commits into from
Feb 2, 2024
Merged

feat: kotlin 1.9.22 #56

merged 10 commits into from
Feb 2, 2024

Conversation

samdozor
Copy link
Member

@samdozor samdozor commented Feb 2, 2024

Summary

This PR updates to the latest Kotlin Multiplatfom version (1.9.22) for better support for Mac M1 (and other goodies that come along with latest Kotlin such as performance improvements..). That then required updating various other things:

  • Kotlin 1.9.22, and required project refactors due to various breaking Kotlin project API changes
  • Mac M1 CI (to test the change actually worked...)
  • Java 17
  • Gradle 8

Testing Plan

  • [x ] Was this tested locally?
  • also added new CI building - we were generating the iOS smartype framework but not actually building the app

Reference Issue

Copy link

sonarcloud bot commented Feb 2, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -11,24 +11,24 @@ env:
jobs:
build-smartype:
name: "Build Smartype"
runs-on: macOS-latest
runs-on: macOS-14
Copy link
Member Author

Choose a reason for hiding this comment

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

this is new on Github - uses ARM machines

Copy link
Contributor

Choose a reason for hiding this comment

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

steps:
- uses: actions/checkout@v3
- name: Set up Ruby
uses: ruby/setup-ruby@359bebbc29cbe6c87da6bc9ea3bc930432750108
uses: ruby/setup-ruby@22fdc77bf4148f810455b226c90fb81b5cbc00a7
Copy link
Member Author

Choose a reason for hiding this comment

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

required for ARM-based runners

Copy link
Contributor

Choose a reason for hiding this comment

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

with:
ruby-version: '3.1.0'
- uses: actions/checkout@v2
- name: set up JDK 1.15
- name: set up JDK 1.17
Copy link
Member Author

Choose a reason for hiding this comment

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

With the update to Gradle 8 as well as Kotlin, as well as latest Android build tools, Java 17 or later is recommended.

- name: Install Cocoapods
run: sudo gem install cocoapods; sudo gem install cocoapods-generate
- name: Use Node.js
uses: actions/setup-node@v1
with:
node-version: '12.x'
node-version: '20.x'
Copy link
Member Author

Choose a reason for hiding this comment

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

Also needed for latest mac os

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -188,26 +188,28 @@ jobs:
- name: Run Tests
working-directory: test-json
run: bash run-all-tests.sh --platform=ios --jar=../examples/iosExample/smartype.jar
- name: Build iOS app
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the actually building of the iOS app, whereas before we were just generating the framework

@@ -28,11 +28,32 @@ kotlin {
js("js") {
browser()
}
jvm("jvm") {}
android("android") {
jvm("jvm") {
Copy link
Member Author

Choose a reason for hiding this comment

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

the below are API changes/updated for the new kotlin 1.9 project structure

@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.mparticle.smartype.api">
<manifest xmlns:android="http://schemas.android.com/apk/res/android">
Copy link
Member Author

Choose a reason for hiding this comment

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

deprecated field, replaced by "namespace" (in build.gradle)

pb5.redirectError(ProcessBuilder.Redirect.INHERIT)
val p5 = pb5.start()
p5.waitFor();
val typescriptDefBuildDir = File(projectDirectory).resolve("build")
Copy link
Member Author

Choose a reason for hiding this comment

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

they decided to change where JS outputs distribution files to in Kotlin 1.9, so we need to copy them from a different place to the final output

import kotlin.native.concurrent.freeze

@OptIn(ExperimentalForeignApi::class)
Copy link
Member Author

Choose a reason for hiding this comment

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

required for kotlin 1.9

mParticle = window["mParticle"]
}
var mParticle: mParticle? = null
if (react == null){
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a BUG. flipped the !=

@samdozor samdozor changed the title Feature/update kotlin 1.9.22 feat: kotlin 1.9.22 Feb 2, 2024
@rmi22186 rmi22186 self-requested a review February 2, 2024 16:49
Copy link
Contributor

@BrandonStalnaker BrandonStalnaker left a comment

Choose a reason for hiding this comment

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

LGTM commented or reacted on everything I was 100% on or found supporting evidence for

@@ -11,24 +11,24 @@ env:
jobs:
build-smartype:
name: "Build Smartype"
runs-on: macOS-latest
runs-on: macOS-14
Copy link
Contributor

Choose a reason for hiding this comment

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

steps:
- uses: actions/checkout@v3
- name: Set up Ruby
uses: ruby/setup-ruby@359bebbc29cbe6c87da6bc9ea3bc930432750108
uses: ruby/setup-ruby@22fdc77bf4148f810455b226c90fb81b5cbc00a7
Copy link
Contributor

Choose a reason for hiding this comment

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

- name: Install Cocoapods
run: sudo gem install cocoapods; sudo gem install cocoapods-generate
- name: Use Node.js
uses: actions/setup-node@v1
with:
node-version: '12.x'
node-version: '20.x'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@samdozor samdozor merged commit c55bba1 into main Feb 2, 2024
16 of 18 checks passed
@samdozor samdozor deleted the feature/update-kotlin-1.9.22 branch February 2, 2024 21:15
Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

Nothing on the js/web portion of things seems off base. My approval is based only on those files though, as my smartype knowledge is not as in depth as the other reviewers.

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.

3 participants