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

Simplify our buildSrc #3322

Merged
merged 2 commits into from
Jan 4, 2021
Merged

Simplify our buildSrc #3322

merged 2 commits into from
Jan 4, 2021

Conversation

BraisGabin
Copy link
Member

The idea is to simplify how we configure our own gradle.

Right now we force that all our modules have a specific configuration and with this new approuch we provide a plugin that configures the module as we want and is the module that opts-in this configuration.

An advantage is that we don't need to add exceptions for :detekt-bom. It just don't use this module.

The idea is that this is just the first step. Once we do this I'll like to use the plugin https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin to improve how we manage our dependencies because we add a lot by default and we probably don't need that much. And, probably, we could remove all the code in commons.gradle.kts at some point.

This will help with the integration with the plugin binary-compatibility-validator too. This last one was the trigger to start this refactor.

@BraisGabin BraisGabin force-pushed the gradle-plugins branch 2 times, most recently from 6c1b702 to 241e2fd Compare December 26, 2020 14:33
@codecov
Copy link

codecov bot commented Dec 26, 2020

Codecov Report

Merging #3322 (8e6aa03) into master (6b065a8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3322   +/-   ##
=========================================
  Coverage     80.28%   80.28%           
  Complexity     2714     2714           
=========================================
  Files           445      445           
  Lines          8196     8196           
  Branches       1558     1558           
=========================================
  Hits           6580     6580           
  Misses          778      778           
  Partials        838      838           

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 6b065a8...da9a8a6. Read the comment docs.

from(components["java"])
artifact(sourcesJar.get())
artifact(javadocJar.get())
if (project.name == "detekt-cli") {
Copy link
Member

Choose a reason for hiding this comment

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

Could we refactor this condition to detekt-cli?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree that this should be removed too. But I'd prefer to do it in other PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can remove this if directly: #3329

@@ -95,32 +82,3 @@ subprojects {
logger.info("Signing Disabled as the PGP key was not found")
}
}

configure(subprojects.filter { it.name != "detekt-bom" }) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks great!

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Great stuff 👏

@chao2zhang
Copy link
Member

What's the recommended merging order between this PR and #3320? I am open to merge in this PR first then see if the hack in #3320 would be fixed

@BraisGabin
Copy link
Member Author

I think that this is a big change and we should wait for other maintainer to approve it. I know that @arturbosch worked a lot refactoring our gradle scripts (they were a real mess all in one file) so I think that we should wait a bit to merge this.

@chao2zhang
Copy link
Member

chao2zhang commented Dec 28, 2020

I think that this is a big change and we should wait for other maintainer to approve it. I know that @arturbosch worked a lot refactoring our gradle scripts (they were a real mess all in one file) so I think that we should wait a bit to merge this.

Is it a good time to merge #3320 before this PR? #3320 creates merge conflicts for a few PRs already.

Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Looks good!

build.gradle.kts Outdated
@@ -4,11 +4,15 @@ plugins {
releasing
detekt
id("org.jetbrains.dokka") apply false
id("com.github.johnrengelman.shadow") apply false
id("com.github.johnrengelman.shadow") version "5.2.0" apply false
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a new place to remember where a version needs to be changed.
Is this change necessary?

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 don't remember why I did that. Probably because we don't need the shadow pugin in buildSrc now. But I moved it to the buildSrc again 👍

@BraisGabin BraisGabin merged commit 945f075 into detekt:master Jan 4, 2021
@BraisGabin BraisGabin deleted the gradle-plugins branch January 4, 2021 23:06
@arturbosch arturbosch added this to the 1.16.0 milestone Jan 14, 2021
@arturbosch arturbosch added the housekeeping Marker for housekeeping tasks and refactorings label Jan 14, 2021
arturbosch pushed a commit that referenced this pull request Jan 16, 2021
* Move cli gradle configuration to its build.gradle.kts

* Create plugin module so we can add it to each module
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.

None yet

4 participants