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

When DGP is used in a pre-compiled script plugin, correctly generate accessors and disable warning logs #3770

Merged

Conversation

adam-enko
Copy link
Member

@adam-enko adam-enko commented Aug 29, 2024

Gradle generates accessors in a temp project that doesn't have Gradle properties. This breaks the enableV2 flag.

This workaround detects when Gradle is generating accessors, and if so, tries to discover the gradle.properties.

Fixes

Because DGPv2 is heavily promoting pre-compiled script plugins I think it's important to get it right, to make a good first impression.

@adam-enko adam-enko self-assigned this Aug 29, 2024
@adam-enko adam-enko added the runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin label Aug 29, 2024
@adam-enko adam-enko force-pushed the adam/feat/OSIP-355/fix-dokka-in-precompiled-script-plugins branch from 6fda580 to 1b6f0d4 Compare August 29, 2024 10:15
Base automatically changed from adam/feat/OSIP-355/add-migration-helpers to master September 9, 2024 09:13
…rectly generate accessors and disable warning logs

Gradle generates accessors in a temporary project that doesn't have Gradle properties. This breaks the `enableV2` flag.

This workaround detects when Gradle is generating accessors, and if it is, tries to discover the `gradle.properties` for the project.
@adam-enko adam-enko force-pushed the adam/feat/OSIP-355/fix-dokka-in-precompiled-script-plugins branch from 1b6f0d4 to fb5816a Compare September 9, 2024 14:49
@adam-enko adam-enko marked this pull request as ready for review September 9, 2024 14:50
@adam-enko adam-enko added this to the Dokka 2.0.0 milestone Sep 9, 2024
/**
* Walk up the file tree until we discover a `gradle.properties` file.
*/
private fun Project.findGradlePropertiesFile(): File? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, this is very hacky... not sure what I think about it

The main question from me: is there any possibility of false-positives? looks like there are enough guards, but still want to have some thoughts on it

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can add my thoughts!

First, a short recap to explain how we arrived at this PR:

  • We want users to be able to select between DGPv1 and v2.
  • We don't want to create a separate temporary Gradle plugin ID for v2, since the migration from DGPv1 to v2 is temporary, and the plugin ID will be permanent.
  • So, we added a Gradle property could let users select between v1 and v2.

Generally using a Gradle property works.... except that Gradle does its own hackery behind the scenes to generate accessors for pre-compiled script plugins. This causes issues as described in the PR descriptor.

Something to keep in mind is this workaround is temporary, so it doesn't need to be perfect. Once DGPv2 is released as the default, we can remove this entirely.

(BTW, it looks like Gradle has a lot of project.name != "gradle-kotlin-dsl-accessors" checks, so this sort of workaround isn't unprecedented.
https://github.com/search?q=repo%3Agradle%2Fgradle%20gradle-kotlin-dsl-accessors&type=code. And maybe we could even simplify our checks here, to remove everything apart from project.name != "gradle-kotlin-dsl-accessors".)

Of course there's a chance this workaround doesn't work! However, I think the consequences for failure are quite low:

  • the workaround is wrapped in a try/catch, so if something fails the accessors won't be generated and the user will get an annoying log message.
  • Or if the property doesn't get picked up correctly: DGPv2 either gets enabled when it shouldn't, or doesn't get enabled when it should. This could be a more significant issue, but if it does occur then we can catch it in a beta release.

Aside from that, I only see one somewhat reasonable workaround:
Release multiple versions of DGP, with separate suffixes. E.g.

plugins {
  id("org.jetbrains.dokka") version "2.0.0"      // DGPv1
  id("org.jetbrains.dokka") version "2.1.0-Beta" // DGPv2 with migration helpers
  id("org.jetbrains.dokka") version "2.1.0"      // DGPv2
}

Or even with (slightly) more descriptive suffixes, like 2.0.0-v2-migration and 2.0.0-v2.

But juggling different versions seems like much more of a difficult process to manage and organise!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! sounds reasonable

Release multiple versions of DGP, with separate suffixes
Or even with (slightly) more descriptive suffixes, like 2.0.0-v2-migration and 2.0.0-v2.

let's try not to do this. I believe current approach should be enough for migration period

BTW, until this is merged, the workaround is to apply plugin like plugins.apply(DokkaHtmlPlugin::class) and configure extension via extensions.configure<DokkaExtension>("dokka") {} (example) - in this case no warning is emitted

@Tapchicoma
Copy link

Note that there are no tests for this yet. They will be added in upcoming PRs, in particular #3781.

It is a little strange to add tests for changes in follow-up PR. I would prefer to have change and tests for it in a single PR.

@adam-enko
Copy link
Member Author

It is a little strange to add tests for changes in follow-up PR. I would prefer to have change and tests for it in a single PR.

Fair point. I'll either try and get #3781 ready for review, or add a test.

@adam-enko
Copy link
Member Author

It is a little strange to add tests for changes in follow-up PR. I would prefer to have change and tests for it in a single PR.

BuildSrcKotlinDslAccessorsTest added, please re-review.

@adam-enko adam-enko merged commit 7733051 into master Sep 19, 2024
14 checks passed
@adam-enko adam-enko deleted the adam/feat/OSIP-355/fix-dokka-in-precompiled-script-plugins branch September 19, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants