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

Port to kotlinx-serialization #101

Open
sschuberth opened this issue Jan 21, 2024 · 10 comments
Open

Port to kotlinx-serialization #101

sschuberth opened this issue Jan 21, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@sschuberth
Copy link
Contributor

sschuberth commented Jan 21, 2024

I'd like to port the Jackson code to kotlinx-serialization, mostly to avoid reflection, but also to have clear annotations on classes that are user for serialization (and thus need special attention when refactoring). before I start, are there any objections to use kotlinx-serialization?

@sschuberth sschuberth changed the title Port to kotlinx-serialization Port to *kotlinx-serialization* Jan 21, 2024
@sschuberth sschuberth changed the title Port to *kotlinx-serialization* Port to kotlinx-serialization Jan 21, 2024
@JLLeitschuh
Copy link
Contributor

I think I originally thought about using KotlinX, but the problem I quickly ran into was the range of versions of Kotlin I needed this project to support in order to support the wide number of Gradle Versions this needed to support.

I don't remember what the earliest version of Gradle this project attempts to support is, but the version of of Kotlin shipped with that version of Gradle didn't have spectacular KotlinX json support IIRC.

This is also why this project wasn't able to use the latest version of Jackson, because later versions of Jackson depend upon Kotlin reflection API features that don't exist in some of the earlier releases of Gradle.

So, I guess, in summary, I'd check if it's even possible first, before trying to do a whole overhaul

@sschuberth
Copy link
Contributor Author

I'd check if it's even possible first

Would a simple stub implementation and see if it compiles / passes test be enough for that check?

@JLLeitschuh
Copy link
Contributor

Passing tests is the most important part. You'll want to make sure it passes a test with the oldest version of Gradle.

Also, at the end of the day, this isn't my codebase anymore. I'm not with Gradle anymore unfortunately. You'll have to get @bigdaz to sign off on this change too.

My comment from yesterday was mostly to provide information about the potential pitfalls you may encounter.

@sschuberth
Copy link
Contributor Author

You'll have to get @bigdaz to sign off on this change too.

Yes, waiting for @bigdaz to confirm this being a good thing as well.

@bigdaz
Copy link
Member

bigdaz commented Jan 22, 2024

I'm actually considering going the other way, and porting the entire codebase to Java. Using Kotlin for a plugin that needs to support a wide range of Gradle versions adds complexity and limitations. For one example, we are stuck with Kotlin 1.3 (which is now deprecated for removal).

That said, I'm OK with any refactoring that retains the current set of supported Gradle versions: https://github.com/gradle/github-dependency-graph-gradle-plugin?tab=readme-ov-file#gradle-compatibility

@bigdaz
Copy link
Member

bigdaz commented Jan 22, 2024

Would a simple stub implementation and see if it compiles / passes test be enough for that check?

Not really. The best way would be to fork the project and ensure that the Java CI with Gradle workflow continues to pass.

@JLLeitschuh
Copy link
Contributor

I'm actually considering going the other way, and porting the entire codebase to Java. Using Kotlin for a plugin that needs to support a wide range of Gradle versions adds complexity and limitations. For one example, we are stuck with Kotlin 1.3 (which is now deprecated for removal).

In hindsight, this was likely the way I should have gone when writing it originally. I really wanted to work on a project in Kotlin though. And my idealism and preference for language drove the decision over the practical problem I was trying to solve.

Sorry Daz for the added headache I left you with because of that original decision. 😬 Hope you can forgive me.

@sschuberth
Copy link
Contributor Author

sschuberth commented Jan 23, 2024

For one example, we are stuck with Kotlin 1.3 (which is now deprecated for removal).

Why is that actually so? If the plugin would "statically link" everything needed from Kotlin / its stdlib (i.e. be a shaded fat JAR), would the Kotlin version even matter? In the end it just gets compiled to JVM bytecode and should be completely independent of the Kotlin version that Gradle itself might use.

That's the approach I'm trying to follow with ORT's GradleInspector and accompanying plugin: Make the plugin completely self-contained though being written in Kotlin. Theoretically, it should even work with Gradle versions that do not support Kotlin DSL at all yet.

@bigdaz
Copy link
Member

bigdaz commented Jan 23, 2024

@sschuberth I'm not a Kotlin expert by any means, and this approach sounds very promising, if feasible. The project is already using the ShadowJar task to create a single shaded jar, but I don't think it's done in a way to make the plugin Kotlin-independent. (The ShadowJar code was implemented by @JLLeitschuh and I've never really investigated).

If you are willing to take a look at improving this, it would be most appreciated! Otherwise I'll certainly take a look before I undertake any conversion to Java.

@JLLeitschuh
Copy link
Contributor

The ShadowJar logic was originally what I found within the closed-source plugin-publish-plugin, which lives internally at Gradle within the same repository as the Gradle Plugin Portal codebase.

I then took that same logic and revised it for the ktlint-gradle plugin: https://github.com/JLLeitschuh/ktlint-gradle/blob/main/plugin/build.gradle.kts

Then that same logic got migrated to this project as well.

One thing you need to be careful about when it comes to the shadowJar solution is that if you expose any API's with kotlin stdlib types you need to make sure those types aren't accidentally shaded as well.

But you've probably already had to deal with that if you're writing plugins using this solution. I'd be curious to see what you come up with

@bigdaz bigdaz added the enhancement New feature or request label Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants