Mpp sample #1494

StefMa · 2018-04-12T14:50:04Z

I've created a sample implementation from the Multiplatform doc.
Don't know if you are interested in it or not.
But I've just added it to the samples and refer it in the multiplatform doc.

Let me know if you want to use it (then I have to adjust the package names (guru.stefma to org.jetbrains) and stuff like that.
Otherwise feel free to just close it 😄

olonho · 2018-04-12T15:28:26Z

samples/multiplatform/androidApp/gradlew

@@ -0,0 +1,172 @@
+#!/usr/bin/env sh


Better reuse shared gradlew in ../../.

Android Studio generates this wrapper while creating the project. I think it's better to not remove it because we have two separate builds in any case. The first one is the multiplatform library and is managed manually. The second one is the Android Studio build which compiles the Android app and is fully managed by Android Studio (including Gradle's version and its updates).

The Gradle guys also create a separate wrapper for each sample project in their samples (e.g. https://github.com/gradle/native-samples/tree/master/cpp/application)

olonho · 2018-04-12T15:28:37Z

samples/multiplatform/androidApp/gradlew.bat

@@ -0,0 +1,84 @@
+@if "%DEBUG%" == "" @echo off


StefMa · 2018-04-13T06:27:52Z

@olonho I can' remove these two gradle wrapper files. But I can't remove the androidApp/gradle/ (since it is needed for Android Studio. Otherwise it will download Gradle 4.0 which isn't working with the Android Plugin 3.0+).
Having just the gradle dir inside but not the wrappers looks very strange (and not common) IMO.
I would keep it like it is.

Beside of that and I wanted to be as close as possible with the documentation. And there stands nothing to use the wrapper from the root dir in the android project.
If we really decided to use a architecture like that I would recommend that we update the documentation as well...?!

StefMa · 2018-04-23T10:42:19Z

@olonho @ilmat192
Do you want to merge it?
Otherwise I would close it :D

ilmat192 · 2018-04-24T05:04:17Z

I think yes, adding such a sample makes sense. But I didn't check this PR on my machine yet so let me take a closer look at it first :)

@olonho What do you think about this MPP sample?

thevery · 2018-05-03T16:55:45Z

samples/multiplatform/androidApp/app/build.gradle

+}
+
+dependencies {
+    implementation "org.jetbrains.kotlin:kotlin-stdlib-jre7:$kotlin_version"


jre dependency is actually deprecated since 1.2.0 and you should use jdk:
http://kotlinlang.org/docs/reference/whatsnew12.html#kotlin-standard-library-artifacts-and-split-packages

thevery · 2018-05-03T17:08:01Z

samples/multiplatform/androidApp/build.gradle

+        jcenter()
+    }
+    dependencies {
+        classpath 'com.android.tools.build:gradle:3.1.1'


3.1.2 is actually latest version

ilmat192 · 2018-05-15T10:30:18Z

Hi @StefMa!

I want to bring some improvements in the sample (in particular rewrite the framework building script as shown in #1583 and fix the remarks above). What is the best way to do it? May you give me access rights to push in this branch in you repo? Or it's better to create a separate branch in our repo and move your commits in it?

StefMa · 2018-05-15T11:19:25Z

Hi @ilmat192

I've checked that little checkbox in this PR. So normally you have already push rights to this PR :)

See also this article and this one.

Feel free to checkout this branch and push anything you like :) 👍

thevery · 2018-05-16T05:58:15Z

@ilmat192 @StefMa maybe add multiplatform tests as well?

+ Switch to Gradle 4.7

Since 0.7 the Gradle plugin uses project properties instead of environment variables to manage debug/optimizations. This patch takes it into account.

ilmat192 · 2018-05-23T13:17:42Z

@StefMa I've rebased the sample onto master and added some new changes. Please look at them and, if all is ok, I'll merge the PR.

StefMa · 2018-05-23T16:40:18Z

MULTIPLATFORM.md

+
+Kotlin/Native requires Gradle 4.7 or higher so you need to make sure that the AS project uses the correct
+Gradle version. To do this, open `androidApp/gradle/gradle-wrapper.properties` and check the `distributionUrl`
+property. Fix it if it's necessary.


Maybe:

Upgrade if necessary

sounds a little bit better.
But its just a minor. Keep it if you like :)

StefMa · 2018-05-23T16:41:11Z

samples/multiplatform/androidApp/app/build.gradle

    implementation 'com.android.support:appcompat-v7:27.1.1'
-    implementation 'org.greeting:android:1.0'
+    implementation project(':greeting:android')


I wanted this sample to reflect 100% of the documentation.
If we use this the documentation and the sample is not the same anymore.
Either change the docu or revert this...

Update

Just seen that there is a big note in the documentation 🙃
Maybe we don't "note" it anymore but used it as a general rule?!

StefMa · 2018-05-23T16:41:56Z

samples/multiplatform/androidApp/gradle/wrapper/gradle-wrapper.properties

@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-all.zip


Have you upgraded via the wrapper task?
It's weird that the *jar haven't changed...

Yes, just forgot about it. Thanks!

StefMa · 2018-05-23T16:44:43Z

samples/multiplatform/greeting/build.gradle

@@ -11,7 +11,7 @@ buildscript {
    // Specify all the plugins used as dependencies
    dependencies {
        classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
-        classpath "org.jetbrains.kotlin:kotlin-native-gradle-plugin:0.7-dev-1407"
+        classpath "org.jetbrains.kotlin:kotlin-native-gradle-plugin:${project.property('konan.plugin.version')}"


Please update the documenation with somehting like $kotlinNativeGradlePluginVersion ...
Then it isn't hardcoded 👍

StefMa · 2018-05-23T16:45:17Z

samples/multiplatform/settings.gradle

@@ -1,4 +1,9 @@
+enableFeaturePreview('GRADLE_METADATA')


Is that needed for the sample?
Then we have to documented that as well

The sample in a separate repo uses KN 0.7 and there is no need to add this line for that version.

StefMa · 2018-05-23T16:46:13Z

samples/multiplatform/settings.gradle

 include ':greeting'
 include ':greeting:common'
 include ':greeting:android'
 include ':greeting:ios'
+
+includeBuild '../../shared'


I would recommend to add a comment here that this is not part "of the sample" and only used for this sample...
Know what I mean? 🙃

Yes, I see :)

The sample becomes a little bit complicated with all these composite builds.
May be it's better to move it in a separate repo?

We use composite build to allow a user to build all samples in the main repo with the compiler and the Gradle plugin built from sources right now. But in a separate repo we can use a fixed KN version and where will be no differences between the documentation and the sample itself. I can make a repo under JB organisation and move your changes into it. What do you think about it?

Indeed. The sample blows a little bit 🙃
Yes, I think it make sense to move it in a new repository.
The only thing we have to make sure is that the documentation and the new repo are always in sync.
We can also move the documentation into the new repository and link in this repo to the new repo?! 🤔
Having the documentation here but the implementation in a new repo is a little bit strange...

Anyway.
I would welcome to have a separate repository for it.
The rest can be discussed then 👍

Having the documentation here but the implementation in a new repo is a little bit strange

Having all docs in one place is more convenient though. But you're right too and I would rather move the MPP doc in the new repo. @olonho what do you think?

thevery · 2018-05-24T17:06:13Z

samples/multiplatform/.gitignore

@@ -0,0 +1,6 @@
+.iml


this one is not required

thevery · 2018-05-24T17:08:28Z

samples/multiplatform/androidApp/build.gradle

+// Top-level build file where you can add configuration options common to all sub-projects/modules.
+
+buildscript {
+    ext.kotlin_version = '1.2.31'


it 1.2.31 required? can we use latest 1.2.41?

thevery · 2018-05-24T17:12:53Z

samples/multiplatform/greeting/android/build.gradle

+// By default the project directory name is used as an artifact name thus the full dependency
+// description will be 'org.greeting:android:1.0'
+group = 'org.greeting'
+version = 1.0


maven-style dependency is actually not used in this sample anymore, only project dependency

We will use the maven-style dependency in a separate repo.

thevery · 2018-05-24T17:15:19Z

samples/multiplatform/iosApp/iosAppTests/iosAppTests.swift

+
+import XCTest
+import Greeting
+@testable import iosApp


thevery · 2018-05-24T17:44:46Z

samples/multiplatform/androidApp/app/build.gradle

@@ -0,0 +1,28 @@
+apply plugin: 'com.android.application'
+apply plugin: 'kotlin-android'
+apply plugin: 'kotlin-android-extensions'


since we don't have android app itself (only empty stub) is makes no sense to add extensions here

It may be useful if a user wants to write an application on basis of this sample.

AS will add this plugin automatically on creating new activity, but this is not a big deal anyway

Ok, removed this.

thevery · 2018-05-24T17:46:14Z

samples/multiplatform/androidApp/app/build.gradle

+    compileSdkVersion 27
+    defaultConfig {
+        applicationId "org.konan.multiplatform"
+        minSdkVersion 24


imho sample should use either 15 or 21

thevery · 2018-05-24T17:46:52Z

samples/multiplatform/greeting/common/build.gradle

+
+dependencies {
+    // Set up compilation dependency on common Kotlin stdlib
+    compile "org.jetbrains.kotlin:kotlin-stdlib-common:$kotlin_version"


can we use implementation here?

The project has been moved in a separate repo.

StefMa · 2018-05-28T11:36:36Z

If I could I would approve that PR 👍
Nice work. Thanks @ilmat192

ilmat192 · 2018-05-28T11:42:40Z

Thanks! The separate repo is here: https://github.com/JetBrains/kotlin-mpp-example. I've fixed all the remarks added to this PR there.

If there are no objections from others, I'll merge this PR.

CC @thevery @olonho

thevery · 2018-05-28T11:57:48Z

Looks almost ok, but there are still some issues:

android build is not working -

* What went wrong:
Execution failed for task ':app:transformResourcesWithMergeJavaResForDebug'.
> More than one file was found with OS independent path 'META-INF/main.kotlin_module'

Add

    packagingOptions {
        exclude 'META-INF/main.kotlin_module'
    }

to fix
2) Why still includeBuild in https://github.com/JetBrains/kotlin-mpp-example/blob/master/androidApp/settings.gradle?

ilmat192 · 2018-05-28T12:14:14Z

@thevery Thanks, I've fixed the issue with packaging.

We add the Greeting build as an included one to allow resolving the Greeting library in the maven-like manner here.

thevery · 2018-05-28T13:07:44Z

ic
LGTM

The MPP sample is implemented in a separate repo: https://github.com/JetBrains/kotlin-mpp-example

olonho reviewed Apr 12, 2018

View reviewed changes

samples/multiplatform/androidApp/gradlew.bat Outdated

@@ -0,0 +1,84 @@

@if "%DEBUG%" == "" @echo off

Copy link

Contributor

olonho Apr 12, 2018

Choose a reason for hiding this comment

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

Same

StefMa force-pushed the mpp_sample branch 2 times, most recently from db2774d to 580275b Compare April 13, 2018 12:38

thevery reviewed May 3, 2018

View reviewed changes

thevery mentioned this pull request May 14, 2018

iOS linker failure with 0.7 (ld: framework not found Greeting) #1583

Closed

StefMa mentioned this pull request May 19, 2018

how to use kotlin-native at an exist iOS and Android project? #1609

Closed

StefMa and others added 9 commits May 23, 2018 18:39

Add multiplatform sample

a54bebb

Add README to multiplatform sample

86bcf15

Add sample to Multiplatform doc

6ee0b29

Use correct gradle task based on iOS simulator or real device

d38f820

Give jetbrains credit

41496fd

Add Greeting section to multiplatform readme

39baa35

Rename package and app id for androidApp

d33ac8e

Use the compiler built from sources in the MPP sample

35abb1a

+ Switch to Gradle 4.7

Fix Xcode build according to changes in the Gradle plugin

0fcdbab

Since 0.7 the Gradle plugin uses project properties instead of environment variables to manage debug/optimizations. This patch takes it into account.

ilmat192 force-pushed the mpp_sample branch from 580275b to 0fcdbab Compare May 23, 2018 13:13

StefMa commented May 23, 2018

View reviewed changes

ilmat192 added 2 commits May 24, 2018 12:47

Minor improvement in MULTIPLATFORM.md

3be2a28

Update Gradle wrappers for the MPP sample

7c91ff3

thevery reviewed May 24, 2018

View reviewed changes

Remove the MPP sample from main repo

fadfbfd

The project has been moved in a separate repo.

Add a link to the MPP sample repo in the MPP doc

0a8c8f3

ilmat192 merged commit 705e39a into JetBrains:master May 28, 2018

SvyatoslavScherbina pushed a commit that referenced this pull request Jun 8, 2018

Add a link to the MPP sample repo in the MPP doc (#1494)

e07ad00

The MPP sample is implemented in a separate repo: https://github.com/JetBrains/kotlin-mpp-example

Mpp sample #1494

Mpp sample #1494

Conversation

StefMa commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefMa commented Apr 13, 2018

StefMa commented Apr 23, 2018

ilmat192 commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmat192 commented May 15, 2018

StefMa commented May 15, 2018 • edited Loading

thevery commented May 16, 2018

ilmat192 commented May 23, 2018

StefMa May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update

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

ilmat192 May 25, 2018 • 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

StefMa commented May 28, 2018

ilmat192 commented May 28, 2018 • edited Loading

thevery commented May 28, 2018

ilmat192 commented May 28, 2018

thevery commented May 28, 2018

StefMa commented May 15, 2018 •

edited

Loading

StefMa May 23, 2018 •

edited

Loading

ilmat192 May 25, 2018 •

edited

Loading

ilmat192 commented May 28, 2018 •

edited

Loading