Skip to content

Update Gradle docs to use module replacement rather than dependency substitution #25944

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

Closed
wants to merge 1 commit into from
Closed

Update Gradle docs to use module replacement rather than dependency substitution #25944

wants to merge 1 commit into from

Conversation

xenoterracide
Copy link
Contributor

Signed-off-by: Caleb Cushing xenoterracide@gmail.com

Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
@xenoterracide
Copy link
Contributor Author

I don't know if it's possible but the syntax is valid for both kotlin and groovy so if it's possible to say the block is both...

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 8, 2021
@wilkinsona wilkinsona changed the title use modern DSL as described in the gradle docs Update Gradle docs to use module replacement rather than dependency substituation Apr 9, 2021
@wilkinsona wilkinsona changed the title Update Gradle docs to use module replacement rather than dependency substituation Update Gradle docs to use module replacement rather than dependency substitution Apr 9, 2021
@wilkinsona wilkinsona added type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 9, 2021
@wilkinsona wilkinsona added this to the 2.3.x milestone Apr 9, 2021
@wilkinsona wilkinsona self-assigned this Apr 9, 2021
@wilkinsona
Copy link
Member

This doesn't quite work as proposed due to this behaviour that's described in Gradle's docs:

Note that if only google-collections appears in the dependency graph (e.g. no guava) Gradle will not eagerly replace it with guava. Module replacement is an information that Gradle uses for resolving conflicts. If there is no conflict (e.g. only google-collections or only guava in the graph) the replacement information is not used.

To get it to work, an explicit dependency on spring-boot-starter-log4j2 must be declared as well as the replacement:

dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-web'
    implementation 'org.springframework.boot:spring-boot-starter-log4j2'
    modules {
        module("org.springframework.boot:spring-boot-starter-logging") {
            replacedBy("org.springframework.boot:spring-boot-starter-log4j2", "Use Log4j2 instead of Logback")
        }
    }
}

This is one line shorter than what we currently document:

dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-web'
}
configurations.all {
    resolutionStrategy.dependencySubstitution.all { dependency ->
        if (dependency.requested instanceof ModuleComponentSelector && dependency.requested.module == 'spring-boot-starter-logging') {
            dependency.useTarget("org.springframework.boot:spring-boot-starter-log4j2:$dependency.requested.version", 'Use Log4j2 instead of Logback')
        }
    }
}

The currently documented approach has the advantage of applying the substitution to every configuration with a dependency on spring-boot-starter-logging. The replacement approach will only work in configuration where both spring-boot-starter-logging and spring-boot-starter-log4j2 are present where the former will then be replaced by the latter.

I'm in two minds now as to whether or not we should make this change. In some situations, a module replacement will be better while in others a dependency substitution is a better choice. The module replacement is slightly more concise and I find it easier to read, but it's also slightly less likely to produce the desired outcome in all configurations. Let's see what the rest of the team thinks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 9, 2021
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Apr 14, 2021
@xenoterracide
Copy link
Contributor Author

I'm going to wait for further feedback on acceptance before making requested changes, but thanks for alerting me ;)

@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Apr 30, 2021
@wilkinsona wilkinsona modified the milestones: 2.3.x, 2.3.11 May 17, 2021
wilkinsona pushed a commit that referenced this pull request May 17, 2021
See gh-25944

Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
@wilkinsona
Copy link
Member

Thank you, @xenoterracide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: documentation A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants