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

Find KSP Configurations that are Added Later #881

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

mgroth0
Copy link
Contributor

@mgroth0 mgroth0 commented Sep 23, 2024

This is a continuation of the changes in #647 and #842. Here I address the issue of configurations being added later.

First, to recap my scenario. In my project that I use dataframes in, I need to use the custom configuration kspCommonJvmAndroidMainMetadata because I have modules that have custom intermediate common source set that compiles to both jvm desktop and to android.

The previous PRs above begin to address this issue. However, there seems to be one more (hopefully last) issue. And that is that in my build logic, for some reason, the configuration kspCommonJvmAndroidMainMetadata is not initialized by Gradle until after the dataframes gradle plugin is applied. I am guessing that this is because I am using a custom configuration, so it is initialized at some point later after the kotlin plugin has already been applied and default configurations have been added. This is unfortunate because build logic can get unmanageable for large projects if we have to worry about the ordering of when different plugins and configurations are applied in gradle buildscripts. Ideally, gradle configuration should not care about order.

This PR fixes that issue by looking for the gradle configurations using the live Project.configurations list. It still retains the ability to warn the user when a configuration is not found by leveraging Gradle.projectsEvaluated.

I personally dislike using Gradle.projectsEvaluated and only resort to it when there is no other option. If anyone knows an alternative way to accomplish this I'd be curious, but I'm unsure if there is any currently. Essentially the problem is that we need to observe the live Project.configurations list and apply KSP when the target configuration is initialized, but still must warn the user if the configuration is never found. Technically I don't know any other way to do this other than to warn the user inGradle.projectsEvaluated, because at that point we know that the Project.configurations list should be done adding elements (I think).

It works for me.

} catch (e: UnknownConfigurationException) {
target.logger.warn(
"Configuration '$cfg' not found. Please make sure the KSP plugin is applied.",
target.configurations.named { it == cfg }.configureEach {
Copy link
Collaborator

@Jolanrensen Jolanrensen Sep 24, 2024

Choose a reason for hiding this comment

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

It may be clearer to use .findByName(cfg) (which is the nullable version of .getByName()). I was a bit confused by configureEach since there's no possibility there are ever multiple configurations with the same name, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I used configureEach is because it is live. Meaning configureEach will continue to search for the configuration with the target name and run the operation on the new configuration when it is added even if that configuration is added after this plugin is applied. Using findByName is not live, so it wouldn't solve the issue that this PR is trying to solve. Or am I misunderstanding?

Copy link
Contributor Author

@mgroth0 mgroth0 Sep 24, 2024

Choose a reason for hiding this comment

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

By the way, your point is well taken that configuraitons.named will continue searching even after it found the configuration. This is undesirable in terms of performance and code cleanness but shouldn't cause any further issues I think. named { it == cfg } will just run for each configuration and return false, which should take a negligible amount of time ... Though I do think gradle suffers in terms of performance from all of these observers because gradle introduces lots of overhead into a lot of its operations (like sending events to the IDE for example).

See gradle/gradle#16543 , which is the same issue but for tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gradle ought to eventually implement something like gradle/gradle#16543 but for any live collection (like the live colleciton of configurations). It would replace the technique I used here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright,thanks for the clarification :)

@Jolanrensen
Copy link
Collaborator

Could you run ktlintCheck/ktlintFormat? If that passes successfully, I'll merge :)

@mgroth0
Copy link
Contributor Author

mgroth0 commented Sep 30, 2024

Thanks!

@Jolanrensen Jolanrensen self-requested a review September 30, 2024 15:40
@Jolanrensen Jolanrensen merged commit cab218c into Kotlin:master Oct 1, 2024
5 checks passed
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.

2 participants