search

gradle / github-dependency-graph-gradle-plugin

Gradle Plugin for Extracting Dependency Information to send to GitHub
Apache License 2.0
77 stars 15 forks source link

Port to kotlinx-serialization #101

Open sschuberth opened 6 months ago

sschuberth commented 6 months ago

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?

JLLeitschuh commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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