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

Make configuration inheritance consistent with Gradle #165

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

rafalh
Copy link
Contributor

@rafalh rafalh commented Dec 10, 2024

Changes

In my project I have a custom test suite that is supposed to inherit dependencies from the integration test classpath.
It is achieved by extending configurations used by the new suite from ones used by integration tests:

configurations {
    named("myTestImplementation") {
        extendsFrom(getByName("integrationImplementation"))
    }
    named("myTestRuntimeOnly") {
        extendsFrom(getByName("integrationRuntimeOnly"))
    }
    named("myTestAnnotationProcessor") {
        extendsFrom(getByName("integrationAnnotationProcessor"))
    }
    named("myTestCompileOnly") {
        extendsFrom(getByName("integrationCompileOnly"))
    }
}

It used to work in version 1.x of this plugin but doesn't work anymore in 2.x.
After improvements in #144 I could achieve what I need extending from integrationCompileClasspath configuration (which extends directly from testCompileClasspath and indirectly from testImplementation and testCompileOnly). My code currently fails because integrationImplementation does not depend on testImplementation.
Why I think current approach may be not optimal:

  • Gradle extends testImplementation configuration in example setup for integration tests:
  • built-in test source-set configurations extend from implementation and runtimeOnly configs (from main source-set) and not compileClasspath
  • Gradle extends implementation config in multiple samples, e.g. here

Not sure if compileOnly and annotationProcessor configurations should be extended. Gradle does do it in its test source-set or integration tests example, which IMHO makes some sense (those dependencies should only be needed to compile the source-set, not to use it). I left it as is to avoid potential breaking changes.

Old behavior:

  • integrationCompileClasspath extends from testCompileClasspath
  • integrationRuntimeOnly extends from testRuntimeClasspath`

New behavior:

  • integrationImplementation extends from testImplementation
  • integrationRuntimeOnly extends from testRuntimeOnly`

I think new behavior is more consistent with Gradle recommendations, which should cause less confusion when extending integration test configurations. At the same time I think this change should not break existing setups, because integrationCompileClasspath already extends integrationImplementation and integrationCompileOnly. Runtime classpath should also not be affected because it's implementation+runtimeOnly.

Checklist

  • I have tested that there is no similar pull request already submitted.
  • I have read contributing.md and applied to the rules. (I think you mean DEVELOPMENT.md, CONTRIBUTING.md does not exist)
  • I have updated the documentation if feature was added or changed.
  • I have unit tested code changes and performed a self-review.
  • I have manually tested change locally.

Old behavior:
* `integrationCompileClasspath` extends from testCompileClasspath
* `integrationRuntimeOnly extends from `testRuntimeClasspath`

New behavior:
* `integrationImplementation` extends from testImplementation
* `integrationRuntimeOnly extends from `testRuntimeOnly`

This is more consistent with what Gradle does with test -> main inheritance and what it recommends in its documentation, resulting in less confusion when extending integration configurations
@coditory coditory bot added the code label Dec 10, 2024
@bgalek
Copy link

bgalek commented Dec 10, 2024

LGTM! Thanks @rafalh!
@pmendelski hope you find some time to take a look :D

@pmendelski
Copy link
Contributor

@rafalh thanks for the thorough analysis with all the links! Really appreciated.

I left few small nitpicks in the review. Thanks for it as well.

Not sure if compileOnly and annotationProcessor configurations should be extended

I believe we should extend it as well, but it can be done in a separate PR. It would be more aligned with the 1.x.x branch where the approach was much more brute force.

P.S. @bgalek I feel summoned by you 😄

Copy link
Contributor

@pmendelski pmendelski left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the thorough analysis!

@rafalh rafalh requested a review from pmendelski December 11, 2024 09:01
@pmendelski pmendelski merged commit 5b31ee0 into coditory:main Dec 11, 2024
3 checks passed
@rafalh rafalh deleted the fix-configuration-inheritance branch December 11, 2024 10:43
@rafalh
Copy link
Contributor Author

rafalh commented Dec 11, 2024

Thanks for merging! Is there any ETA for next release? @pmendelski

@ogesaku
Copy link
Contributor

ogesaku commented Dec 11, 2024

@rafalh It's released under v2.2.2. Great contribution, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants