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

[K2] Add a check that source roots do not intersect #3465

Merged

Conversation

vmishenev
Copy link
Member

@vmishenev vmishenev commented Jan 23, 2024

The pre-generation check throws DokkaException here. Probably it could be a logging error messages instead of an exception. Need to discuss.

@vmishenev vmishenev linked an issue Jan 23, 2024 that may be closed by this pull request
@vmishenev vmishenev force-pushed the vmishenev/3239-add-a-check-that-source-roots-do-not-intersect branch 2 times, most recently from ff0688b to 18a0479 Compare January 23, 2024 13:06
@vmishenev vmishenev force-pushed the vmishenev/3239-add-a-check-that-source-roots-do-not-intersect branch from 18a0479 to bfdc31f Compare January 23, 2024 13:24
@vmishenev vmishenev marked this pull request as ready for review January 23, 2024 14:32
Copy link
Collaborator

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

LGTM.

I think, that it should be still an error, because if not, we will anyway fail during execution with K2, so error log message doesn't make sense to me, or do you have a reason why it could be better?

But, may be we need to add checker in K1 which will show warning message if something like this happens, to prepare users, that after switching to K2 current setup will not work? Or warning in KGP (from some version) will be enough?

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

👍 👍

I also believe it makes sense to throw an exception in this case, so that it fails with a user-understandable error instead of a very technical compiler error

And Oleg's idea also seems very useful: to log a warning in K1 that in K2 it will be an error, and to ask users who see this log warning in K1 to report their use case. Something like

Source sets X and Y have common source roots: Z. Every Kotlin source file should belong to only one source set (module). This will become an error when Dokka migrates to K2-based code analysis. If you cannot fix this and you think you have a valid use case, please report it: https://github.com/Kotlin/dokka/issues/3239

}
}
}
return PreGenerationCheckerOutput(messages.isEmpty(), messages)
Copy link
Member

Choose a reason for hiding this comment

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

Took me a while to figure out if / how / why Dokka would throw an exception in K2, but not in K1 (because the code looks the same), so leaving a message for Oleg in case he also doesn't see it right away: the first parameter of PreGenerationCheckerOutput is basically a isSuccess boolean, so Dokka fails if it's false.

@vmishenev vmishenev merged commit 722d082 into master Feb 26, 2024
10 of 12 checks passed
@vmishenev vmishenev deleted the vmishenev/3239-add-a-check-that-source-roots-do-not-intersect branch February 26, 2024 16:47
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.

[K2] Add a check that source roots do not intersect
3 participants