potential fix for compatibility with KGP 1.9

[whyoleg]

attempt to fix #3068 which is blocking building documentation for https://github.com/whyoleg/cryptography-kotlin/tree/dev (dev branch, KGP 1.9.0)
for recent changes consult: #3081 (comment)

[IgnatBeresnev]

Thank you for taking a shot at this!

I explained in more detail the high-level reason for why this is happening here. TLDR: some changes were made in KGP, and it doesn't fully support single-target multiplatform projects (not in the usual way), so Dokka is having issues in exactly the place you made changes to :)

We also had a discussion with the KMP team a few days ago about what exactly the technical reason was, and what could be done on Dokka's end to fix it, both short and long term solutions. I plan to write down a summary at the beginning of next week, so it should hopefully provide enough background and context for you to proceed with the PR

[IgnatBeresnev]

I've finally had time to have a closer look at this PR and the issues with Kotlin 1.9.0 in general.

It seems like these are two separate problems:

• support for single-target multiplatform projects
• inter-module native dependency resolution thing

The first problem about single-target KMP projects is not completely fixed in this PR - I'll provide more details in one of the comments. We also decided that we need to include a fix for it to Dokka 1.9.0, which is already overdue, so we'd like to take this particular issue off your hands :)

The second issue, however, is interesting - you've potentially found the problem and a fix for it, so I'd like to discuss it further and merge it if the fix is OK. I'll also leave a more detailed comment below

[IgnatBeresnev]

Single-target projects were somewhat "fixed" in #3103 (sorry that you need to resolve merge conflicts now), so we can focus on addressing #3068 in this PR

If you don't mind, I'll resolve the long conversations above as there were mostly clarifications, and I'll sum them up here instead. I've also discussed the proposed solutions with Sebastian from the Kotlin Multiplatform team, and will include his thoughts below.

• KotlinCompile is indeed JVM only, so KotlinCompileTool could/should be used instead. Not sure how and why it worked before 😅
• It should be safe to remove this.compileDependencyFiles - Sebastian agreed that kotlinCompileTool.libraries should provide compile dependency files already, and in a better/safer way
• Removing platformDependencyFiles fixes the issue from Failed to resolve inter-module dependencies: Cannot change dependencies of dependency configuration #3068, but Sebastian mentioned that this configuration might contain some native-specific dependencies that are not present in kotlinCompileTool.libraries. Unfortunately, I don't have enough context to understand the details, but as far as I remember it's related to some dependencies that are either system-provided or just provided by default, so they're not added to libraries, or something along those lines. He mentioned iOS x64 as something that might uncover the bugs with this.

---

Given the tight schedule for 1.9.0 (it's pretty much ready for final testing), and that this place in general is quite scary to change and has led to serious problems before, I would propose the following:

For < 1.9.0, use the current behaviour (along with keeping compatibility with < 1.7 and so on).

For >= 1.9.0:

• replace KotlinCompile with KotlinCompileTool
• remove compileDependencyFiles
• add a fallback flag (false by default), which, if enabled, will use pre-1.9.0 behaviour.

Additionally:

• use platformDependencyFiles by default the same way as now, but
• introduce a second flag (false by default) that removes platformDependencyFiles from classpath calculation, so that projects affected by Failed to resolve inter-module dependencies: Cannot change dependencies of dependency configuration #3068 have a workaround

The reason for not enabling the platformDependencyFiles flag by default: there are not that many projects that reported this problem, so we don't know what the exact reason is, or what they have in common. And there's a considerable chance of something breaking, judging by what Sebastian mentioned.

Right after merging the PR and cherry-picking the commit into the release branch:

• change the flag for platformDependencyFiles to true in Dokka's master branch

There should be about a month between 1.9.0 and 1.9.10, which gives us time to dogfood and find problems with platformDependencyFiles. If we find bugs - we'll just revert the it to false by default.

Code-wise, to avoid a bunch of if-else branches, maybe it'll be easier to refactor the file a little, and extract pre-1.9.0 and >=1.9.0 solutions into separate classes / functions, it's up to you

What do you think? :)

[whyoleg]

@IgnatBeresnev Nice plan! I like it. Also big thanks for summarising everything, good job!
Don't worry about merge conflicts, I expected them :)
I also really like the idea of splitting behaviour of pre and post 1.9 - while it could cause a little of duplications, we could try to stick for almost public API in post 1.9, this is good especially in scope of stabilisation of multi-platform support in KGP in 1.9.
I will actualise branch with what proposed, do checks on my project and promote PR from draft in coming days (likely by Monday)

[IgnatBeresnev]

@whyoleg awesome, thank you!

[whyoleg]

So, I've pushed changes. 2 flags introduced:

• org.jetbrains.dokka.classpath.excludePlatformDependencyFiles - if true, will exclude platformDependencyFiles
• org.jetbrains.dokka.classpath.useOldResolution - when KGP >= 1.9, if true will use old resolution with compileDependencyFiles and all other stuff, if false will only use KotlinCompileTool.libraries (+ platformDependencyFiles in both cases)

I've tested it on my project - everything works fine here with flag org.jetbrains.dokka.classpath.excludePlatformDependencyFiles=true.

After that, I've decided to run some integration tests with this flag set to true and it-multiplatform-0 failed. It failed, because now it can not resolve CPointer and CPointed classes - this means, that they are coming from deprecated implementationMetadataConfigurationName.
I checked is there a way to provide those dependencies in other way and found that now they are added in KotlinNativePlatformDependencies.kt and I didn't found an easy way, on how we could access them in Dokka, as everything is super internal.

Then I've decided to check on replacement of deprecation of implementationMetadataConfigurationName and found resolvableMetadataConfiguration which is also used in IDE import in KGP, but it's also highly internal and not exposed anywhere else.

So, to summarise, excluding platformDependencyFiles is not an option for projects, which expose kotlinx.cinterop and other platform libraries in documentation :(

It's up to you to decide, should we leave this flag, for excluding platformDependencyFiles, or not, while for my project it's a solution (as there is no such declarations exposed), for other it will be not acceptable.

[whyoleg]

Also, is there a good place to document those flags?

[IgnatBeresnev]

Well done! I'll have a closer look towards the evening

So, to summarise, excluding platformDependencyFiles is not an option for projects, which expose kotlinx.cinterop and other platform libraries in documentation :(

Oh, thank you for researching and confirming what Sebastian mentioned! I believe in these situations kotlinx.cinterop types will be rendered as Error class, while everything else should rendered as expected, so I think it's still a valid workaround for the time being - at least then affected users will be able to build documentation, even if some types are rendered as Error class

Also, is there a good place to document those flags?

As there are no guarantees and we don't plan to support these flags for long, I think it's better to only post them in the comments of issues (which should be googlable by the error message), as opposed to some official documentation. I need to close the duplicates though, so that all of the related bugs lead to a single issue in which these flags will be suggested as a workaround - I'll do it this week.

I guess I should also mention it in the release notes, that some projects with kotlinx.cinterop might experience issues, and link to the workaround comment.

But maybe you have some other suggestions?

[IgnatBeresnev]

I've also approved the GitHub Actions that run integration tests, and it seems like there are some compatibility problems:

java.lang.NoSuchMethodError: 'org.gradle.api.Project org.jetbrains.kotlin.gradle.plugin.KotlinCompilation.getProject()'

Because this is your first contribution, I have to manually approve the runs after your every commit, but if you want - you can send a PR with updating some library version (like kotlinx-bcv), I'll merge it quickly, and after that the actions should be run automatically. Or you can initialize git submodules locally, and then run kotlinx.coroutines / kotlinx.serialization from IDEA (CoroutinesGradleIntegrationTest)

[whyoleg]

I believe in these situations kotlinx.cinterop types will be rendered as Error class, while everything else should rendered as expected

Yeah, that's what I've seen when checking produced by test results.

I think it's better to only post them in the comments of issues

Yes, I agree, that will be enough, as those flags are mostly for workarounds

and it seems like there are some compatibility problems:

Oops, haven't run all integration tests (too much resources needed), will recheck locally later today, thx!

[whyoleg]

I've pushed fix for compatibility with older kotlin versions. Haven't spotted, that previously project was resolved from argument, but after extraction from KotlinCompilation.project which was introduced in recent KGP versions...

Also, run SerializationGradleIntegrationTest, CoroutinesGradleIntegrationTest and Multiplatform0GradleIntegrationTest locally - everything works (and now I have 11 K/N compilers installed locally :) )

[IgnatBeresnev]

Looking good, thank you!

Just in case, I'll trigger longer integration tests on TeamCity now, as well as thorough tests with more platforms, and will merge the PR tomorrow if there are no problems

(upd to not lose the link: thorough tests run)

[IgnatBeresnev]

as a side note (and, I guess, as acknowledgement that it's a conscious decision for anyone looking at it): names with old and new get repetitive quick, it's easy to fall into "old vs old old" / "really old vs old", but I couldn't think of a better alternative to suggest, and hopefully we'll get rid of this code altogether by Kotlin 2.0, so it will definitely do for now 👍

[whyoleg]

yeah, I agree, first idea at my side for naming was like pre19* and post19* or just use pre19 for old function and no prefix for new, but it also started to look a bit odd for me

[IgnatBeresnev]

yeah, I had the same idea and also found it weird looking :)

[IgnatBeresnev]

The integration tests failed a few times due to TeamCity agents not having enough space, and just due to some test being flaky, but I restarted it and everything passed in the end, so it looks like everything is alright

Congrats on the first contribution, off to a great start! :)