Make "belongsTo" create virtual components by default #6452
Conversation
| * @param id the original component identifier | ||
| * @return a virtual component identifier | ||
| */ | ||
| public static VirtualComponentIdentifier makeVirtual(final ComponentIdentifier id) { |
There was a problem hiding this comment.
Is there really no solution simpler than a dynamic proxy?
There was a problem hiding this comment.
Didn't find any obvious one, but I'm open to suggestions.
There was a problem hiding this comment.
Maybe there is for now no need to be this generic and we can verify that belongsTo only produces DefaultModuleComponentIdentifier and replace these with a VirtualModuleComponentIdentifier.
There was a problem hiding this comment.
Yeah but it would introduce a very specific type for this. I'm not sure I like it more.
There was a problem hiding this comment.
❌ I don't really like this sort of magic. Maybe I'm just too "traditional".
How about:
a) Fail if belongsTo is called with anything other than a ModuleComponentIdentifier
b) Use a concrete wrapper that implements VirtualComponentIdentifier
c) Maybe make VirtualComponentIdentifier extends ModuleComponentIdentifier
| * @param id the original component identifier | ||
| * @return a virtual component identifier | ||
| */ | ||
| public static VirtualComponentIdentifier makeVirtual(final ComponentIdentifier id) { |
There was a problem hiding this comment.
❌ I don't really like this sort of magic. Maybe I'm just too "traditional".
How about:
a) Fail if belongsTo is called with anything other than a ModuleComponentIdentifier
b) Use a concrete wrapper that implements VirtualComponentIdentifier
c) Maybe make VirtualComponentIdentifier extends ModuleComponentIdentifier
| * that we never "find" a virtual component, by shortcutting resolution if we find the marker interface | ||
| * for virtual components. | ||
| */ | ||
| class VirtualComponentMetadataResolver implements ComponentMetaDataResolver { |
There was a problem hiding this comment.
To me, this is much better than the 'discovery' of virtual components.
Now that we know up-front if a component is virtual or not, I'd like to revert the changes made in #5957. If a platform is not virtual, then we should fail if it does not resolve. I suspect that we've left the fallback code in place to construct a virtual platform if we are told (via belongsTo("", false) that a platform is published and it turns out to be missing.
The retryIfMissing boolean is the sort of complexity that might seem harmless enough, but could cause us pain further down the line.
This commit changes the behavior of "belongsTo" in component metadata rules, so that it creates a virtual platform by default. By doing this, we optimize the engine which can now avoid checking for a real, published platform, avoiding costly repository calls to fetch metadata. If a component needs to declare that it participates in a published platform, or if we don't know if the platform is published or not, then the alternate, new `belongsTo(id, virtual)` method needs to be called. This is a performance optimization only. Closes #6447
melix
force-pushed
the
cc/dm/always-virtual
branch
from
18c70ed
to
658d80b
Compare
| import org.gradle.api.artifacts.component.ModuleComponentIdentifier; | ||
| import org.gradle.internal.DisplayName; | ||
|
|
||
| public class DefaultVirtualModuleComponentIdentifier implements VirtualComponentIdentifier, ModuleComponentIdentifier, DisplayName { |
There was a problem hiding this comment.
I would drop the Default from the name, leaving VirtualModuleComponentIdentifier
There was a problem hiding this comment.
I'd better leave it as it would be inconsistent with the current naming strategy.
|
Yes, I had changed the uses of |
This commit removes the ability of the engine to determine that a platform is virtual when it's not marked as such. This means that we effectively trust the user calling the right `belongsTo` method. By doing this, it allows us to remove the `retryIfMissing` complexity.
melix
force-pushed
the
cc/dm/always-virtual
branch
from
480a5aa
to
7900a90
Compare
Context
This PR is a performance optimization: it makes sure that platforms declared via
belongsToin a rule are assumed virtual. By doing so, we avoid polling external repositories to check if a module exists or not.The
belongsTo(Object, boolean)method can be used to "restore" the previous behavior. By usingfalseas a parameter, we declare that the target platform is either published, or we don't know. The consequence is that the engine would ping the external repository to check, and therefore initiate network requests like it did before this PR.Implementation-wise, this PR wraps a
ComponentIdentifierby marking it with a special interface.