Remove usages of ComponentResolveMetadata #29123
Conversation
jvandort
force-pushed
the
jvandort/dm/delete-component-resolve-metadata
branch
from
db2c87a
to
54f5e71
Compare
|
@bot-gradle test this |
|
I've triggered the following builds for you. Click here to see all build failures. |
...ement/src/main/java/org/gradle/api/internal/artifacts/repositories/resolver/IvyResolver.java
Show resolved
Hide resolved
...org/gradle/api/internal/artifacts/ivyservice/ivyresolve/RepositoryChainArtifactResolver.java
Outdated
Show resolved
Hide resolved
...main/java/org/gradle/internal/component/external/model/ExternalComponentResolveMetadata.java
Show resolved
Hide resolved
| if (metadata instanceof IvyModuleResolveMetadata) { | ||
| IvyModuleResolveMetadata ivyMetadata = (IvyModuleResolveMetadata) metadata; | ||
| return new DefaultIvyComponentGraphResolveState(idGenerator.nextComponentId(), ivyMetadata, attributeDesugaring, idGenerator); | ||
| } | ||
|
|
||
| return new DefaultModuleComponentGraphResolveState(idGenerator.nextComponentId(), metadata, attributeDesugaring, idGenerator); |
There was a problem hiding this comment.
🤔 This seems backwards from an OOP point of view.
Why not:
public ModuleComponentGraphResolveState stateFor(ModuleComponentResolveMetadata metadata) {
return metadata.buildResolveState(idGenerator, attributeDesugaring);
}
There was a problem hiding this comment.
The intention is that the State types are mutable/stateful and track all the data for a given component while the Metadata types are fully immutable and track a subset of data for a given component.
Each component should have one state type, but that state type may track multiple metadata types as the state changes (we may learn about a component's variants through different network requests/operations compared to its artifacts). In that case we would have a metadata for the artifacts, a metadata for the variants, and some base metadata that describes the identity of the components and some other basic stuff that we know up-front about it.
Currently for external module components (and local components -- but I am working to change that), the Metadata type (ModuleComponentResolveMetadata in this file) is stateful and has a lot of logic that the State type that wraps it should do.
The point that I'm trying to make here is that the State type should be "in charge" of the Metadata type. It feels wrong to me for the metadata type to construct a state type for itself.
There was a problem hiding this comment.
OK, I see your point. Can you add some of that to the Javadoc on this factory?
...n/java/org/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveState.java
Show resolved
Hide resolved
| * | ||
| * @return Null if the configuration does not exist. | ||
| */ | ||
| @Nullable |
There was a problem hiding this comment.
❓ Why return null here? From the callers, looks like an empty list could signal just as well. Otherwise, there's Optional, which would even allow a slight simplification of the calling code via ifPresent.
There was a problem hiding this comment.
I worry there is a slight difference between a Configuration existing with no artifacts and a configuration not existing. There is a lot of logic related to caching and stuff around here that I am not familiar with and would rather not subtly change.
Specifically, this method is called by resolveJavadocArtifacts and resolveSourceArtifacts. In the prior code it only called resolved if the configuration exists -- regardless if that configuration had artifacts or not.
Using an empty list instead of null (or optional) would mean we call resolved if there was a configuration AND it had artifacts.
There was a problem hiding this comment.
Optional with ifPresent then to remove the null. I know you like functional coding paradigms. 😄
...ent/src/main/java/org/gradle/internal/component/model/DefaultComponentGraphResolveState.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public ModuleComponentResolveMetadata getModuleResolveMetadata() { | ||
| return getMetadata(); | ||
| } | ||
|
|
||
| @Override | ||
| public IvyComponentArtifactResolveMetadata getResolveMetadata() { | ||
| return new DefaultIvyComponentArtifactResolveMetadata(getArtifactMetadata()); | ||
| } |
There was a problem hiding this comment.
💭 This bit is quite confusing. I don't have any concrete suggestions to improve it, just a lot of different kinds of metadata in play.
There was a problem hiding this comment.
Yeah I 100% agree there are way too many interfaces involved here. This whole part of the code is in a middle-ground state right now and this change hopes to be a small part of that.
In practice, it makes sense to have a different metadata/state type for ivy components compared to other components, because ivy components have configurations -- while other components do not (local Gradle components do have configurations for now, but this PR is the start of the work to remove the concept of configurations from local components from a DM perspective).
There was a problem hiding this comment.
I realize it's in an intermediate state and things will get better, but can anything be done to clarify the distinctions between these methods? Can the names be made more specific? If I'm a client trying to determine which of these methods I need to call, is there anyway other than trial-and-error to get it right?
The primary motivation here is to simplify LocalComponentMetadata (now renamed to LocalComponentGraphResolveMetadata), by reducing the type hierarchy. This will make it much easier to heavily modify the way local component metadata works. ComponentResolveMetadata was a legacy type, previously used to represent a component when resolving artifacts. Now, the modern type, ComponentArtifactResolveMetadata, has been propogated to all prior places that used the legacy type. All methods on the removed type have been moved to its remaining subinterface, ExternalComponentResolveMetadata. This subinterface is not heavily used, but removing it would cause more work than was desired for this change.
jvandort
force-pushed
the
jvandort/dm/delete-component-resolve-metadata
branch
from
18a5a14
to
8333a20
Compare
* ArtifactMetadata on the component graph resolve state and the module resolve metadata were always the same instance. * Creating the new artifact metadata (ComponentArtifactResolveMetadata) relied on data from the legacy metadata anyways * Remove getSources from graph state since ModuleSources exposes how to fetch artifacts. getSources already existed on the new artifact metadata * Introduce missing ExternalComponentGraphResolveState interface * Add javaodc, improve method names
|
@bot-gradle test this |
|
I've triggered the following builds for you. Click here to see all build failures. |
The primary motivation here is to simplify LocalComponentMetadata (now renamed to LocalComponentGraphResolveMetadata), by reducing the type hierarchy. This will make it much easier to heavily modify the way local component metadata works.
ComponentResolveMetadata was a legacy type, previously used to represent a component when resolving artifacts. Now, the modern type, ComponentArtifactResolveMetadata, has been propagated to all prior places that used the legacy type.
All methods on the removed type have been moved to its remaining subinterface, ExternalComponentResolveMetadata. This subinterface is not heavily used, but removing it would cause more work than was desired for this change.
Reviewing cheatsheet
Before merging the PR, comments starting with