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

Tracking of artifact dependency coordinates by BootJar and BootWar may break artifact transforms in sub-projects #31216

Closed
wilkinsona opened this issue May 31, 2022 · 9 comments
Labels
type: bug A general bug
Milestone

Comments

@wilkinsona
Copy link
Member

#31215 is a prerequisite for this.

From the Gradle 7.5 release notes:

Tasks may need to access dependency resolution results. For example, built-in tasks like dependencies and dependencyInsight do so in order to provide reporting about resolved artifacts and dependency graphs. Other tasks may produce file outputs based on dependency resolution results. Previously, it was only possible by performing dependency resolution in a task action. However, this resulted in suboptimal performance.

Starting with Gradle 7.5 it is now possible to declare dependency resolution results as task inputs.

This allows writing tasks which consume dependency resolution results. Declaring such inputs instead of doing undeclared dependency resolution in task actions allows Gradle to optimise for build incrementality. Additionally, these new types of task inputs are fully supported by the configuration cache.

You can learn more in the Authoring Tasks user manual chapter and with the dedicated sample.

@wilkinsona wilkinsona added this to the 3.0.x milestone May 31, 2022
@wilkinsona wilkinsona added the type: task A general task label May 31, 2022
@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label May 31, 2022
@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Jul 21, 2022
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Oct 5, 2022
@philwebb
Copy link
Member

philwebb commented Oct 5, 2022

Since this is an optimization, perhaps we can push it back to 3.1

@wilkinsona
Copy link
Member Author

Unfortunately, I think it'll require changes to the public API of some tasks. I am not sure that those changes can be done in a backwards compatible way.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 5, 2022
@wilkinsona
Copy link
Member Author

Having looked at this more closely, I'm pretty confident that it can be done in a backwards compatible way.

It's the BootJar and BootWar tasks that will be affected by a change in this area. They currently use an afterResolve hook to create a mapping from the files of a configuration's resolved artifacts to the dependency coordinates that resulted in the resolved configuration containing that file. This is currently an implementation detail of the task which makes testing difficult and we current rely on extensive mocking in some tests as a result.

My current thinking is that we can replace the afterResolve-based mechanism with a new task that uses a Provider<ResolvedComponentResult> or perhaps a Provider<List<ResolvedComponentResult>> as an input and has a Map<File, DependencyCoordinates> as an output. This output can then be wired into an input of BootJar and BootWar and used for allocating jars in BOOT-INF/lib into the appropriate layers, etc. By convention the ResolvedComponentResult from the runtimeClasspath configuration will be used.

Making the Map<File, DependencyCoordinates> an input of BootJar and BootWar will require a change to their public API but it should be purely additive and therefore backwards compatible. It will also make it far easier to test BootJar and BootWar as the input can be set with a pre-prepared Map, replacing the current extensive mocking of Configuration and related classes.

@DanielThomas
Copy link
Contributor

DanielThomas commented Apr 21, 2023

@wilkinsona just a heads up that this issue can break artifact transforms against sub-projects if any task resolves a configuration before the library file is built:

nebula-plugins/gradle-jakartaee-migration-plugin#7

@wilkinsona
Copy link
Member Author

The more I think about this, the more it feels like it may be a Gradle bug. AFAIK, we're not doing anything wrong here. It may be suboptimal from a performance perspective, but AFAIK there's nothing to say that we should not call getFiles() on an already resolved configuration.

I've asked the Gradle team before if there's a better way to map from a file back to the dependency coordinates from which it was resolved but didn't manage to get much traction. I'll ask them again.

@DanielThomas
Copy link
Contributor

It's not a bug: resolving artifacts inside an afterResolve hook means you're resolving artifacts files for tasks that have no declaration of a dependency on the files and the tasks that produce them. They're explicitly only resolving dependency metadata, so there's no way for Gradle to wire the task dependencies such that would make this work. In fact we have tasks that resolve metadata that have to run before the artifacts provided by getFiles() exist, so this logic also causes cycle in some cases.

In addition to being incompatible with transforms, we notice the performance impact to dependency locking and other metadata resolution only use cases, and have occasional failures of dependencies, dependencyInsight tasks because of unconditional artifact resolves when the underlying metadata has failed, making those tasks useless for troubleshooting the resolution failure.

@DanielThomas
Copy link
Contributor

DanielThomas commented May 2, 2023

This is the reduced version of what's going on:

plugins {
	id 'java'
}

repositories {
	mavenCentral()
}

configurations {
    someConfiguration {
        canBeConsumed = true
        canBeResolved = false
    }
    runtimeClasspath.incoming.afterResolve { it.getFiles().each { assert it.exists() } }
}

def resolve = tasks.register('resolve') {
	doFirst {
		configurations.runtimeClasspath.resolvedConfiguration
	}
}

tasks.register('someJar', Jar).configure {
	it.dependsOn resolve
}

artifacts {
    someConfiguration(someJar)
}

dependencies {
    implementation project(path: ":", configuration: 'someConfiguration')
}
❯ ./gradlew resolve
> Task :resolve FAILED

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/dannyt/Projects/resolve-cycle/build.gradle' line: 14

* What went wrong:
Execution failed for task ':resolve'.
> Failed to notify dependency resolution listener.
   > assert it.exists()
            |  |
            |  false
            /Users/dannyt/Projects/resolve-cycle/build/libs/test.jar

@wilkinsona
Copy link
Member Author

Thanks, Danny. I'm not sure I agree that it isn't a bug. For Boot's purposes, we don't need the file to exist – a File object that points to the file that may exist in the future is sufficient. If calling getFiles() as Boot is doing makes it impossible in some circumstances for those files to exist in their correct form in the future, I'd argue that it's either a functional bug (although you say that it can't be made to work) or a usability bug. The latter could perhaps be addressed by failing fast so that plugin or build authors don't inadvertently create hard-to-diagnose problems.

@DanielThomas
Copy link
Contributor

Yeah, I'd definitely consider Gradle not failing fast on this a bug.

@wilkinsona wilkinsona changed the title Review Gradle plugin for places where resolution results can be used as task inputs Tracking of dependency coordinates by BootJar and BootWar may break artifact transforms in sub-projects May 5, 2023
@wilkinsona wilkinsona added type: bug A general bug and removed type: task A general task labels May 5, 2023
@wilkinsona wilkinsona modified the milestones: 3.x, 3.0.x May 5, 2023
@wilkinsona wilkinsona modified the milestones: 3.0.x, 3.0.7 May 5, 2023
@wilkinsona wilkinsona changed the title Tracking of dependency coordinates by BootJar and BootWar may break artifact transforms in sub-projects Tracking of artifact dependency coordinates by BootJar and BootWar may break artifact transforms in sub-projects May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants