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

Initial Kotlin Multiplatform setup #196

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Jul 24, 2022

Initial groundwork for Kotlin Multiplatform #192

Depends on #194

I was expect this to be a lot more difficult! I indended just to do one module, but I found that they were all very easy to migrate. html-parser was the most involved.

That said, I can't run most of the tests (I'm on Windows), so I could have broken some stuff. And the really hard work of actually implementing JS and/or Native code can be done later.

WIP

  • Migrate test-utils
  • Update Kover config (or disable Kover if this is too difficult)
  • Configure Maven publishing buildSrc plugin (shouldn't be too much work to do, I can copy & paste some existing config that works) I've briefly tested this locally and it seems to work as expected.
  • Verify that the new publications are correct and work. This means checking the POMs are correct and expose the right API dependencies.
  • ~jsExecution feature variant - I can't find an alternative for this with Kotlin Multiplatform. https://youtrack.jetbrains.com/issue/KT-33432. ~ I've simply added the 'maven publishing' config to the browser-fetcher project. I think that will achieve the same result.

Notes

  • bump Kotlin to 1.7.10, and the language level to 1.7, and - ⚠ breaking change - the api level to 1.5 (from 1.4). Kotlin 1.7 has some nice improvements for Kotlin Multiplatform. And level 1.4 is deprecated. This seemed like a good time to bump it.
  • JVM only
  • All tests are still JUnit
  • I didn't try migrating any code, just moving things into the correct source sets
  • The real work was creating expect/actual definitions - so check them out and see if they make sense. The expect definitions are essentially like interfaces that the platform code will implement.
  • The 'JS browser execution' feature probably won't work - I disabled the Gradle option for it
  • The HttpFetcher and BrowserFetcher objects are pretty redundant, as they don't significantly extend from the BlockingFetcher interface. I think you can refactor the common code to only rely on the interface.
  • Publishing and releasing are still TODO

@aSemy aSemy marked this pull request as draft July 24, 2022 13:31
…atform

# Conflicts:
#	assertions/build.gradle.kts
#	build.gradle.kts
#	buildSrc/build.gradle.kts
#	buildSrc/src/main/kotlin/buildsrc/convention/kotlin-jvm.gradle.kts
#	buildSrc/src/main/kotlin/buildsrc/convention/kotlin-multiplatform.gradle.kts
#	dsl/build.gradle.kts
#	fetcher/async-fetcher/build.gradle.kts
#	fetcher/base-fetcher/build.gradle.kts
#	fetcher/browser-fetcher/build.gradle.kts
#	fetcher/http-fetcher/build.gradle.kts
#	html-parser/build.gradle.kts
#	test-utils/build.gradle.kts
…atform

# Conflicts:
#	buildSrc/src/main/kotlin/Deps.kt
#	html-parser/src/commonMain/kotlin/it/skrape/selects/DocElement.kt
#	html-parser/src/jvmMain/kotlin/it/skrape/selects/Doc.kt
#	html-parser/src/jvmMain/kotlin/it/skrape/selects/DomTreeElement.kt
@aSemy
Copy link
Contributor Author

aSemy commented Aug 26, 2022

I've picked this back up again, but I've come across an issue with Kover. 0.5 had a bug Kotlin/kotlinx-kover#183, and now 0.6 does too Kotlin/kotlinx-kover#221

@christian-draeger
Copy link
Collaborator

christian-draeger commented Sep 7, 2022

Hey awesome news you proceed working on the PR 🙏🚀
If kover is blocking the further development I think we can just proceed while waiting for a new kover release that hopefully includes a fix.

Is kover / make the gradle plugins work the last thing that is missing to finish the PR?

And big thanks again for your contribution 🔥 I really appreciate.

@aSemy
Copy link
Contributor Author

aSemy commented Sep 7, 2022

Apologies for the delay, and thanks for the nudge! :) I'll try and get this ready-for-review in the next couple of hours.

I've got an idea of how to fix Kover, so I'll give that a go. I've updated the main description with the TODOs.

html-parser/build.gradle.kts Outdated Show resolved Hide resolved
Comment on lines +34 to +48
publishing {
repositories {
// publish to local dir, for testing
maven(rootProject.layout.buildDirectory.dir("maven-internal")) {
name = "LocalProjectDir"
}
}
publications.withType<MavenPublication>().configureEach {
artifact(javadocJar)

createSkrapeItPom {
name.set("skrape{it} ${project.name}")
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason Sonatype isn't defined here..

I would expect to see something like this:

https://github.com/kotest/kotest/blob/945e029b4aea4ca39487f2dcf10d9a066dd7050a/buildSrc/src/main/kotlin/kotest-publishing-conventions.gradle.kts#L22-L31

The same in the publish-jvm convention plugin...

Was it removed, and it should be re-added now?

@aSemy aSemy marked this pull request as ready for review September 7, 2022 21:51
@McDjuady McDjuady mentioned this pull request Oct 19, 2022
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.

None yet

2 participants