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.

Sign up for GitHub

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

Jump to bottom

Split okhttp core into Android and JVM #8600

Merged
merged 41 commits into from
Dec 27, 2024
Merged

Split okhttp core into Android and JVM #8600

merged 41 commits into from
Dec 27, 2024

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Dec 1, 2024 •
edited
Loading

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.

yschimke
yschimke commented Dec 1, 2024
View reviewed changes
build.gradle.kts Outdated Show resolved Hide resolved
yschimke
yschimke commented Dec 1, 2024
View reviewed changes
gradle/libs.versions.toml Outdated Show resolved Hide resolved
@yschimke yschimke mentioned this pull request Dec 1, 2024
Merged
swankjesse
swankjesse reviewed Dec 4, 2024
View reviewed changes
okhttp/build.gradle.kts Outdated Show resolved Hide resolved
swankjesse
swankjesse reviewed Dec 4, 2024
View reviewed changes
okhttp-testing-support/src/main/kotlin/okhttp3/internal/concurrent/TaskFaker.kt
@@ -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?

swankjesse
swankjesse reviewed Dec 4, 2024
View reviewed changes
okcurl/build.gradle.kts Outdated Show resolved Hide resolved
swankjesse
swankjesse reviewed Dec 4, 2024
View reviewed changes
build.gradle.kts Outdated Show resolved Hide resolved
swankjesse
swankjesse approved these changes Dec 4, 2024
View reviewed changes
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
Merged
@yschimke yschimke force-pushed the multiplatform_android branch from 14cf0c7 to 7ea2b89 Compare December 8, 2024 20:10
@yschimke
use toml
a668933
@yschimke
Merge branch 'gradle_warnings' into multiplatform_android
2c7c64c 
# Conflicts:
#	okcurl/build.gradle.kts
#	okhttp/build.gradle.kts
@yschimke
more fixes
f1c8396
@yschimke yschimke force-pushed the multiplatform_android branch from 1e18b6a to f1c8396 Compare December 26, 2024 13:44
@yschimke yschimke added documentation Documentation task android Relates to usage specifically on Android build-ci Build infra related graal GraalVM related jdkversions JDK 8, 17, 19 etc. labels Dec 26, 2024
@yschimke yschimke changed the title Experiment with splitting okhttp core into Android and JVM Split okhttp core into Android and JVM Dec 27, 2024
@yschimke yschimke mentioned this pull request Dec 27, 2024
Open
@yschimke
Copy link
Collaborator Author

@swankjesse @JakeWharton heads up, I think it's working now. Going to land based on prior Jesse approval and work through any issues that pop up.

But I think these are working

  • osgi and test (moved to a test module)
  • docs and publish to local maven repo
  • Android tests
  • native image tests

The gradle builds got messier, but I think we should adopt convention plugins, so maybe a good thing to have this in first.

@yschimke
Copy link
Collaborator Author

almost!

Working through android-test-app:minifyReleaseWithR8

@yschimke yschimke marked this pull request as ready for review December 27, 2024 13:02
@yschimke yschimke merged commit 5b2a1e1 into square:master Dec 27, 2024
22 checks passed
@yschimke yschimke mentioned this pull request Dec 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swankjesse swankjesse swankjesse approved these changes

Assignees
No one assigned
Labels
android Relates to usage specifically on Android build-ci Build infra related documentation Documentation task graal GraalVM related jdkversions JDK 8, 17, 19 etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yschimke @swankjesse