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

Refactor so detekt-gradle-plugin can be added as an included build #4094

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

3flex
Copy link
Member

@3flex 3flex commented Sep 9, 2021

This refactors the build logic so it's possible to include detekt-gradle-plugin as either a composite build or a standard project module. Just replace include("detekt-gradle-plugin") with includeBuild("detekt-gradle-plugin") in settings.gradle.kts to define the composite build.

@3flex 3flex added the housekeeping Marker for housekeeping tasks and refactorings label Sep 9, 2021
@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #4094 (ced2e28) into main (e05eec5) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4094      +/-   ##
============================================
- Coverage     83.45%   83.44%   -0.01%     
+ Complexity     3185     3177       -8     
============================================
  Files           463      465       +2     
  Lines          9095     9104       +9     
  Branches       1768     1775       +7     
============================================
+ Hits           7590     7597       +7     
- Misses          571      572       +1     
- Partials        934      935       +1     
Impacted Files Coverage Δ
.../arturbosch/detekt/rules/style/UseIsNullOrEmpty.kt 54.66% <0.00%> (-2.67%) ⬇️
...bosch/detekt/rules/complexity/LongParameterList.kt 83.33% <0.00%> (-1.12%) ⬇️
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt 80.95% <0.00%> (-0.30%) ⬇️
...ab/arturbosch/detekt/core/config/ValidateConfig.kt 100.00% <0.00%> (ø)
...tlab/arturbosch/detekt/rules/style/UseDataClass.kt 77.94% <0.00%> (ø)
...b/arturbosch/detekt/rules/naming/FunctionNaming.kt 100.00% <0.00%> (ø)
...tlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt 90.00% <0.00%> (ø)
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt 82.92% <0.00%> (ø)
...etekt/rules/style/FunctionOnlyReturningConstant.kt 94.87% <0.00%> (ø)
...ch/detekt/core/suppressors/AnnotationSuppressor.kt 90.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e05eec5...ced2e28. Read the comment docs.

@3flex
Copy link
Member Author

3flex commented Sep 12, 2021

Converting to draft as there's more to be done before this works as advertised.

settings.gradle.kts Show resolved Hide resolved
detekt-gradle-plugin/settings.gradle.kts Outdated Show resolved Hide resolved
@3flex 3flex marked this pull request as ready for review September 17, 2021 13:09
@3flex
Copy link
Member Author

3flex commented Sep 17, 2021

I'm not loving the fact that some of the build config had to move back to the root project's build file instead of a precompiled script plugin in build-logic, but it wouldn't work as a composite build without it.

@3flex
Copy link
Member Author

3flex commented Sep 21, 2021

LGTM? I think this is as good as it will get.

I'm using these commits and a composite build myself for a couple of things and it's working really well.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

To be honest I'm a bit lost here. So I give my approve because it seems to work.

build.gradle.kts Outdated
?: System.getenv("MAVEN_CENTRAL_USER")
password = findProperty("sonatypePassword")
?.toString()
?: System.getenv("MAVEN_CENTRAL_PW")
Copy link
Member

Choose a reason for hiding this comment

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

We are moving this here because we can't apply this in the gradle plugin when we use composite compilation, right?

Could we create another plugin in build-logic so we don't need to have this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd moved it out of build-logic as it wasn't shared build config like mostly everything else that remained, but there's a home for it in releasing so I've moved it there instead. The config only needs to be applied to the root build file, and releasing is only applied in the root file so this makes sense to me.

detekt(project(":detekt-cli"))
detektPlugins(project(":custom-checks"))
detektPlugins(project(":detekt-formatting"))
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we can't keep this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

When detekt-gradle-plugin is an included build, and it includes build-logic, the project dependencies can't be resolved. So they need to be moved out of build-logic - and applying them to subprojects from the root build file is the best way to do that from a single location.

This removes the use of `subprojects` block as it can't be used effectively
with composite builds.
Replace `include("detekt-gradle-plugin")` with
`includeBuild("detekt-gradle-plugin")` in settings.gradle.kts
@3flex 3flex merged commit 755a150 into detekt:main Oct 13, 2021
@3flex 3flex deleted the build-logic3 branch October 13, 2021 05:39
@cortinico cortinico added this to the 1.19.0 milestone Oct 16, 2021
@cortinico
Copy link
Member

Just replace include("detekt-gradle-plugin") with includeBuild("detekt-gradle-plugin") in settings.gradle.kts to define the composite build.

Is there a reason why you haven't replaced this? That would allow us to don't have to rely on the preivously released version of detekt-gradle-plugin for the release and so on. Or are you afraid of a performance impact of the composite build?

@3flex
Copy link
Member Author

3flex commented Oct 20, 2021

I'm not too worried about perf personally, but I think some others are or were.

There's also the issue that running, for example, gradlew build in the root project won't execute the detekt-gradle-plugin:build task without wiring things up as described here: https://docs.gradle.org/current/userguide/composite_builds.html#included_build_task_dependencies

A separate PR could be opened to link up the lifecycle tasks (build, check, run) to the same tasks in the included build so it works as expected.

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

Successfully merging this pull request may close these issues.

3 participants