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

Experiment with splitting okhttp core into Android and JVM #8600

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Dec 1, 2024

Uses this approach for a shared Java core between Android and JVM.

https://jeroenmols.com/blog/2021/03/17/share-code-kotlin-multiplatform/

Source structure under okhttp

  • androidMain
  • androidUnitTest
  • commonJvmAndroid
  • jvmMain
  • jvmTest

Outputs

image

TODO

  • Get it working fully for running tests
  • Get builds working and identify blockers around OSGI, publishing?

@yschimke yschimke requested a review from swankjesse December 1, 2024 13:16
@yschimke
Copy link
Collaborator Author

yschimke commented Dec 1, 2024

It's noisier because something is persistently picking up the main source set in the android build, and warning about it. Worse it makes builds fail because of duplicate sources.

@yschimke
Copy link
Collaborator Author

yschimke commented Dec 1, 2024

Also more invasive because a lot is changing here, so building this against 1.9.24 and AGP 8.2 sounds like a dead end.

build.gradle.kts Outdated
apply(plugin = "org.jetbrains.dokka")
apply(plugin = "com.diffplug.spotless")
//apply(plugin = "org.jetbrains.dokka")
//apply(plugin = "com.diffplug.spotless")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Disabled because of Gradle Sync issues when enabled.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
kotlin("plugin.serialization")
id("org.jetbrains.dokka")
id("com.vanniktech.maven.publish.base")
// id("com.vanniktech.maven.publish.base")
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • needs something!

@@ -360,15 +360,17 @@ class TaskFaker : Closeable {
timeout: Long,
unit: TimeUnit,
): T? {
taskRunner.withLock {
return taskRunner.withLock {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what’s with this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kotlin 2 didn't like the inline with return?

}
//animalsniffer {
// isIgnoreFailures = true
//}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • needs something

build.gradle.kts Outdated
// tasks.withType<JavaCompile> {
// sourceCompatibility = JavaVersion.VERSION_17.toString()
// targetCompatibility = JavaVersion.VERSION_17.toString()
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

??

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

I love it. There’s some Gradle stuff to clean up before landing though!

@yschimke yschimke mentioned this pull request Dec 7, 2024
@yschimke yschimke force-pushed the multiplatform_android branch from 14cf0c7 to 7ea2b89 Compare December 8, 2024 20:10
@yschimke
Copy link
Collaborator Author

yschimke commented Dec 8, 2024

# Conflicts:
#	build.gradle.kts
#	native-image-tests/build.gradle.kts
#	okcurl/build.gradle.kts
#	okcurl/src/main/kotlin/okhttp3/curl/Main.kt
#	okhttp/src/main/resources/META-INF/native-image/okhttp/okhttp/native-image.properties
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.

2 participants