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

Overcome fastutil issue #3409

Merged
merged 2 commits into from
Dec 27, 2023
Merged

Overcome fastutil issue #3409

merged 2 commits into from
Dec 27, 2023

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Dec 12, 2023

fixes #3329

I've added description of the issue directly in code (same for both K1 and K2 modules). Will duplicate/expand on it here:
Related KT issue: https://youtrack.jetbrains.com/issue/KT-47150

What is the issue: kotlin-compiler artifact includes unshaded (not-relocated) STRIPPED fastutil dependency, STRIPPED here means, that it doesn't provide full fastutil classpath, but only a part of it which is used and so when shadowJar task is executed it takes classes from fastutil from kotlin-compiler and adds it to shadow-jar then adds all other classes from fastutil coming from markdown-jb, but because fastutil from kotlin-compiler is STRIPPED, some classes (like IntStack) lacks some methods and so such classes are not replaced afterward by shadowJar task - it visits every class once

Current solution (presented in PR) is to exclude fastutil from shadowJar and add runtime dependency on fastutil which will be resolved like if using runtimeOnly.
This is not ideal solution, as theoretically we could have issues with third-party plugins, that could provide fastutil on their side, but I think, that it should be more-or-less fine for now because:

  1. analysis-kotlin-* dependencies will be only used for execution of dokka in a separate classpath, so there should be no issues for final users except they will for some reason use some old version of fastutil
  2. analysis-markdown-jb (which is used by base and all-modules plugins) depends on markdown library which also depends on fastutil as api - and so it will be available in classpath anyway

Other ways to fix the issue:

  1. May be a better solution will be to include fastutil in shadow JAR - in this case we will need to create additional shadowJar task, which will first build jar without fastutil from kotlin-compiler and then build another shadowJar task which will depend on it + will include full fastutil (same as used in markdown lib). There is no now possibility in shadow plugin to select, from which dependency to get classes, when they differs (because it's really should not happen in real world).
  2. Another possible way to fix the issue, is to use kotlin-compiler-embeddable which shoud correctly embed and shade dependencies (such as fastutil), but in this case, we should explicitly add other dependencies from kotlin-compiler which are relocated in kotlin-compiler-embeddable - then it will be needed to investigate which dependencies are really used (from my experiment I found, that at least some of the intelliij dependencies will be needed). Though, it gets more complicated with K2 analysis as on current moment there is no kotlin-compiler-embeddable artifact in declared repositories for version 2.0.0-dev-8561, even kotlin/dev doesn't contain it - most likely analysis API and kotlin/dev are published independently.
  3. Another possibility, when https://youtrack.jetbrains.com/issue/KT-47150 will be fixed we need to do nothing - as fastutil will be correctly relocated in kotlin-compiler

So, in the end, what is implemented in PR is fine, but if there are some concerns we can do what described above in point1 - other ways are not available at the moment.

@whyoleg whyoleg self-assigned this Dec 12, 2023
@@ -29,6 +29,7 @@ kotlinx-html = "0.9.1"

## Markdown
jetbrains-markdown = "0.5.2"
fastutil = "8.5.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just in case, I haven't mentioned it, but this is the same version, which is used in jetbrains-markdown right now

Copy link
Member

Choose a reason for hiding this comment

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

Does this version need to be aligned with the version of fastutil from some other dependency, like the version used in jetbrains-markdown or in kotlin-compiler?

If it needs to be aligned, I think there should definitely be a comment about it with a link, similar to how it's done for intellij-platform up above, so that we are not trigger-happy with updating it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall, the version should be compatible with what is used in kotlin-compiler (AFAIK the same version is used in IntelliJ dependencies) and with markdown-jb library.
For now, markdown-jb uses newer version - so I would think we need to align with it.
I will write additional comment here

@IgnatBeresnev
Copy link
Member

Thank you for such a detailed explanation, and especially for the in-code comments! Everything makes sense now, and I think this solution is indeed decent given the constraints

analysis-markdown-jb (which is used by base and all-modules plugins)

oof, this doesn't look right; I must've forgotten about it when breaking analysis down. after #3366 it should be pretty easy to get rid of the dependency on analysis-markdown-jb in base and all-modules-page. I'll send a follow up PR, hopefully it'll make the chances of something blowing up smaller

Another possible way to fix the issue, is to use kotlin-compiler-embeddable which shoud correctly embed and shade dependencies (such as fastutil)

I think I researched it a bit, and yeah, I definitely remember having problems with it. If I recall correctly, kotlin-compiler ships with its own relocated PSIs, but we actually need PSIs in Java analysis, especially when it comes to inheriting documentation, and so I think I couldn't pass PSIs from analysis-kotlin into analysis-java and vise versa, because they were different classes. Something like that

@whyoleg
Copy link
Contributor Author

whyoleg commented Dec 19, 2023

I'll send a follow up PR, hopefully it'll make the chances of something blowing up smaller

Note, that in all-modules it's used to parse includes files - so full blown MarkdownParser is initiated - not sure, that we will be able to easily remove there dependency on analysis-markdown

kotlin-compiler ships with its own relocated PSIs, but we actually need PSIs in Java analysis

Hm, yes, that sounds legit. Then, I think, may be we need to somehow discuss with team developing analysis-api regarding this/similar things - so that at least with K2 stable API we will somehow be able to get rid of such hacks, it that's possible

@IgnatBeresnev
Copy link
Member

Note, that in all-modules it's used to parse includes files - so full blown MarkdownParser is initiated - not sure, that we will be able to easily remove there dependency on analysis-markdown

That's why the internal package in analysis-kotlin-api exists :) Could just add a signature for the markdown parser

@whyoleg whyoleg merged commit cefa907 into master Dec 27, 2023
13 checks passed
@whyoleg whyoleg deleted the fix-fastutil branch December 27, 2023 11:36
vmishenev pushed a commit that referenced this pull request Mar 20, 2024
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.

Dokka failing for email-like string in angle brackets
3 participants