Add no-op JVM target #626

konpach · 2024-01-30T18:12:33Z

Relates Issue: #380

Context: Until library gets a proper implementation for jvm targets we want to allow the usage of the library in supported targets along with jvm. Currently is not possible to compile shared if jvm target exists (as there is no corresponding implementation in kable).
Solution: Provide an empty implementation that throws runtime exceptions for kable for jvm target.

… with other targets

This reverts commit bc1056e.

konpach · 2024-01-30T18:41:35Z

Build failed due to

Expected folder with API declarations '/Users/runner/work/kable/kable/core/api/android' does not exist.
  Please ensure that ':apiDump' was executed in order to get API dump to compare the build against

I thought that these are generated during the build, will run locally and add.

twyatt · 2024-01-30T19:23:30Z

This may have changed/improved, but I thought historically having a JVM and Android target in the same module would confuse Gradle on the consumer side?

In other words, would an Android app project be able to consume Kable if it had both an Android target and a (no-op) JVM target? How would it know to use the Android when both are available?

If you have a chance (if not, then I'll try to find some time) to test these changes in the SensorTag sample app to ensure it doesn't break Android app consumers.

If it does break Android consumers, then I believe we'll have to introduce a new -android suffixed artifact for Android consumers. 🤔

konpach · 2024-01-30T20:00:01Z

This may have changed/improved, but I thought historically having a JVM and Android target in the same module would confuse Gradle on the consumer side?

In other words, would an Android app project be able to consume Kable if it had both an Android target and a (no-op) JVM target? How would it know to use the Android when both are available?

TBH I haven't experienced this before. I thought it's handled by gradle. But good point

If you have a chance (if not, then I'll try to find some time) to test these changes in the SensorTag sample app to ensure it doesn't break Android app consumers.

If it does break Android consumers, then I believe we'll have to introduce a new -android suffixed artifact for Android consumers. 🤔

I tried it with SensorTag and it works as expected. Also, I tried it with a project that has iOS, android and desktop targets and it compiles and runs as expected for all of the platforms. After you comment I also did a sanity check with my android app that scanning and connecting to a BLE device works as before (without the no-op jvm impl).

@twyatt

twyatt · 2024-01-30T20:19:12Z

Awesome! Thanks for testing/validating!

cedrickcooke

Looks good to me.

Weighing in on the build issues re. having both jvm and android targets, I think I remember it being a problem specifically when there was a diamond hierarchy. If jvm-library depends on kable, android-library depends on kable, and your-android-app depends on both jvm-library and android-library, problems show up.

Supporting JVM doesn't automatically break anything so I'm cool with this going in, but it does leave some room to shoot yourself in the foot.

core/build.gradle.kts

core/api/android/core.api

core/src/androidMain/kotlin/Profile.kt

twyatt · 2024-02-02T18:59:28Z

core/src/jvmMain/kotlin/com/juul/kable/Exceptions.kt

@@ -0,0 +1,6 @@
+package com.juul.kable
+
+public fun jvmNotImplementedException(): Nothing = throw NotImplementedError(


Can this be internal?

Suggested change

public fun jvmNotImplementedException(): Nothing = throw NotImplementedError(

internal fun jvmNotImplementedException(): Nothing = throw NotImplementedError(

No, if it's internal, the compiler gives error about the visibility.

I presume the error is related to other modules not being about to access it?

In that case, can we make it internal and duplicate the function in any module that uses it?

That way we aren't exposing a new function on the public API.

Good point. Actually, I miss understood the error. It seemed relevant but actually it was hiding a missing impl in the exceptions module! Made this internal and also added the implementation for the jvm target for the IOException.

Saw your revert, makes sense, thank you. Don't know why building locally failed but passing here in GHA. Will try to understand.

core/src/jvmMain/kotlin/com/juul/kable/logs/SystemLogEngine.kt

Co-authored-by: Travis Wyatt <travis.i.wyatt@gmail.com>

konpach · 2024-02-06T21:31:05Z

@twyatt anything more to look into?

twyatt · 2024-02-07T17:31:34Z

Apologies for the delay.

Looks good aside from one last comment I left re: removing the exception function from the public API.

twyatt

Looks great!
I'll do some final testing this weekend and get it merged/released.
Thanks @konpach!

konpach and others added 6 commits January 30, 2024 19:09

Add jvm target with void implementation to allow library to sit along…

6060df2

… with other targets

Remove locally generated api

bc1056e

Merge branch 'main' into konpach/add-jvm-target-with-void-implem

32ffe57

Revert "Remove locally generated api"

21ff32d

This reverts commit bc1056e.

Revert api doc changes

d096687

Remove api doc changes

dd9ad8c

konpach force-pushed the konpach/add-jvm-target-with-void-implem branch from 7d88b05 to dd9ad8c Compare January 30, 2024 18:22

Update log-engine-tuulbox.api

6088cc8

twyatt added the minor Changes that should bump the MINOR version number label Jan 30, 2024

konpach changed the title ~~Add jvm target with void implementation to allow compilation along with supported targets~~ Add jvm target with void implementation to allow compilation along with supported targets - Relates Issue: #380 Jan 30, 2024

konpach marked this pull request as ready for review January 30, 2024 18:25

konpach requested review from twyatt and a team as code owners January 30, 2024 18:25

konpach requested a review from sdonn3 January 30, 2024 18:25

Generate api docs

8bc9e0a

cedrickcooke approved these changes Jan 31, 2024

View reviewed changes

twyatt reviewed Feb 2, 2024

View reviewed changes

konpach and others added 3 commits February 3, 2024 08:13

Update core/build.gradle.kts

157401b

Co-authored-by: Travis Wyatt <travis.i.wyatt@gmail.com>

PR review

917d080

remove core api file

d9ab458

Make jvmNotImplementedException internal

67ff1b0

twyatt changed the title ~~Add jvm target with void implementation to allow compilation along with supported targets - Relates Issue: #380~~ Add no-op JVM target Feb 8, 2024

Revert IOException back to a typealias of java.io.IOException

d353e4a

twyatt approved these changes Feb 8, 2024

View reviewed changes

twyatt added 2 commits February 11, 2024 22:32

Simplify exception message and add link to GitHub issue

2364c8f

Delete old log-engine-tuulbox API dump

cedd3b6

twyatt enabled auto-merge (squash) February 12, 2024 06:35

twyatt merged commit c3c8adc into JuulLabs:main Feb 12, 2024
2 checks passed

vermasrijan19 mentioned this pull request Sep 29, 2024

Adding WASM target artifact #765

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no-op JVM target #626

Add no-op JVM target #626

konpach commented Jan 30, 2024 •

edited

Loading

konpach commented Jan 30, 2024

twyatt commented Jan 30, 2024 •

edited

Loading

konpach commented Jan 30, 2024

twyatt commented Jan 30, 2024

cedrickcooke left a comment •

edited

Loading

twyatt Feb 2, 2024

konpach Feb 3, 2024 •

edited

Loading

twyatt Feb 7, 2024

konpach Feb 8, 2024

konpach Feb 8, 2024

konpach commented Feb 6, 2024

twyatt commented Feb 7, 2024 •

edited

Loading

twyatt left a comment

		@@ -0,0 +1,6 @@
		package com.juul.kable

		public fun jvmNotImplementedException(): Nothing = throw NotImplementedError(

	public fun jvmNotImplementedException(): Nothing = throw NotImplementedError(
	internal fun jvmNotImplementedException(): Nothing = throw NotImplementedError(

Add no-op JVM target #626

Add no-op JVM target #626

Conversation

konpach commented Jan 30, 2024 • edited Loading

konpach commented Jan 30, 2024

twyatt commented Jan 30, 2024 • edited Loading

konpach commented Jan 30, 2024

twyatt commented Jan 30, 2024

cedrickcooke left a comment • edited Loading

Choose a reason for hiding this comment

twyatt Feb 2, 2024

Choose a reason for hiding this comment

konpach Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

twyatt Feb 7, 2024

Choose a reason for hiding this comment

konpach Feb 8, 2024

Choose a reason for hiding this comment

konpach Feb 8, 2024

Choose a reason for hiding this comment

konpach commented Feb 6, 2024

twyatt commented Feb 7, 2024 • edited Loading

twyatt left a comment

Choose a reason for hiding this comment

konpach commented Jan 30, 2024 •

edited

Loading

twyatt commented Jan 30, 2024 •

edited

Loading

cedrickcooke left a comment •

edited

Loading

konpach Feb 3, 2024 •

edited

Loading

twyatt commented Feb 7, 2024 •

edited

Loading