Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180

asodja · 2024-05-16T12:27:57Z

Fixes #29177

…rFileProperty

asodja · 2024-05-16T12:29:24Z

@bot-gradle test this

bot-gradle · 2024-05-16T12:29:29Z

I've triggered the following builds for you. Click here to see all build failures.

PullRequestFeedback build

asodja · 2024-05-16T13:33:53Z

A failure looks unrelated to changes

lptr · 2024-05-16T13:41:59Z

.../file-collections/src/main/java/org/gradle/api/internal/file/DefaultFilePropertyFactory.java

@@ -193,6 +197,19 @@ public Provider<File> getAsFile() {
            return new MappingProvider<>(File.class, this, new ToFileTransformer());
        }

+        @Override
+        public void conventionFromAny(Provider<Object> file) {
+            convention(file.map(any -> {


How about:

Suggested change

convention(file.map(any -> {

convention(provider.map(value -> {

This would make it more understandable for me.

My R$ 0,02 is that since file reflects the role of the parameter, and provider reflects the type, the former would be preferable (given that Java variables are explicitly typed). Maybe fileProvider would be a decent middle ground. I am fine with any of the 3 options, just sharing an opinion about parameter and local variable naming.

Both comments seem like a good improvements, thanks!

I also renamed method to conventionFromAnyFile since we actually allow just some types of a file (either File or Directory) and not any object.

I take that back, file is also reflecting kind of type information (though something that is not available to the language), and conventionSource or source would be a better name for conveying its role. But again, I am fine with any of the options, just promoting the general principle.

I changed that parameter to provider so it's consisent with Property.convention(Provider<? extends T> provider).

I think readability won't change much if we rename it some more :)

lptr · 2024-05-16T13:43:44Z

...ration/model-core/src/main/java/org/gradle/internal/extensibility/ConventionAwareHelper.java

@@ -99,7 +102,10 @@ private MappedProperty map(String propertyName, MappedPropertyImpl mapping) {
    }

    private boolean mapConventionOn(SupportsConvention target, MappedPropertyImpl mapping) {
-        if (target instanceof Property) {
+        if (target instanceof DirectoryProperty || target instanceof RegularFileProperty) {


❌ use FileSystemLocationProperty.

Good point, fixed, thanks!

lptr · 2024-05-16T13:44:32Z

...ration/model-core/src/main/java/org/gradle/internal/extensibility/ConventionAwareHelper.java

-        if (target instanceof Property) {
+        if (target instanceof DirectoryProperty || target instanceof RegularFileProperty) {
+            ConventionMappingFileSystemLocationPropertyProxy proxy = Cast.uncheckedNonnullCast(target);
+            proxy.conventionFromAny(new DefaultProvider<>(() -> mapping.getValue(_convention, _source)));


❓ Why wrap this in a Provider? I think a Callable is perfectly fine, no?

This is beacuse convention on property doesn't support Callable (a missing feature?)

lptr · 2024-05-16T13:47:01Z

.../java/org/gradle/api/internal/provider/ConventionMappingFileSystemLocationPropertyProxy.java

+@NonNullApi
+public interface ConventionMappingFileSystemLocationPropertyProxy {
+    void conventionFromAny(Provider<Object> file);
+}


❌ I don't think we need this type. We can simply do what we need in ConventionAwareHelper.mapConventionOn() when we detect a FileSystemLocationProperty, no?

I added this type because we cannot create a Directory or a RegularFile inside ConventionAwareHelper, we can't access any factory there, or at least am not aware of a way to do that.

That is why we then pass File to a DirectoryProperty/RegularFileProperty via this method and there we can then do a conversion from a File to a Directory or a RegularFile

From what I noticed around, whenever we need implementations of an API type to support some internal protocol that we don't want to expose with API users, we create a SomeApiTypeInternal interface that (optionally) extends SomeApiType and is implemented by all implementations of SomeApiType.

For instance, ProjectInternal, PropertyInternal, ProviderInternal.

Would it make sense to follow that here, @asodja?

Good point. I introduced FileSystemLocationPropertyInternal extends FileSystemLocationProperty. This also makes more sense now and it's more consisten with our code style compared to previous ConventionMappingFileSystemLocationPropertyProxy.

abstratt · 2024-05-16T13:55:55Z

.../file-collections/src/main/java/org/gradle/api/internal/file/DefaultFilePropertyFactory.java

-    private static abstract class AbstractFileVar<T extends FileSystemLocation, THIS extends FileSystemLocationProperty<T>> extends DefaultProperty<T> implements FileSystemLocationProperty<T> {
+    private static abstract class AbstractFileVar<T extends FileSystemLocation, THIS extends FileSystemLocationProperty<T>> extends DefaultProperty<T> implements FileSystemLocationProperty<T>, ConventionMappingFileSystemLocationPropertyProxy {
+
+        private final Class<T> type;


Is this an optimization? (given that the type is available via DefaultProperty.getType())

Oh, it's not an optimization, I just didn't know that getType() exists. Thanks for the insight, I will use that!

…mapping

… FileSystemLocationPropertyInternal

asodja · 2024-05-17T09:56:27Z

@bot-gradle test this

bot-gradle · 2024-05-17T09:56:32Z

I've triggered the following builds for you. Click here to see all build failures.

PullRequestFeedback build

abstratt · 2024-05-17T11:52:36Z

.../core-api/src/main/java/org/gradle/api/internal/file/FileSystemLocationPropertyInternal.java

+ * Should be removed once ConventionMapping is removed.
+ */
+@NonNullApi
+public interface FileSystemLocationPropertyInternal<T extends FileSystemLocation> extends FileSystemLocationProperty<T> {


It seems the convention is to also annotate the public type with @HasInternalProtocol:

/** * Indicates that there is an internal complementary protocol to the public type that is annotated with this. * * This should only be used on a type that is always assumed to also implement the internal protocol by Gradle internals. * * This exists to help anyone reading the source code realise that there is an internal component to the type. */

This annotation is for public types to warn users to not implement them. This type is internal anyway, so there is no added value to add this annotation

Sorry, I meant to annotate the public type FileSystemLocationProperty, not this internal protocol itself.

This annotation is for public types to warn users to not implement them.

Are you sure, Anže? The javadoc seems to describe it is meant exactly to denote the case we have here, a public type (FileSystemLocationProperty) has an internal protocol (FileSystemLocationPropertyInternal) that must be implemented by all implementations of the public type (AbstractFileVar being the common ancestor to all of them):

This should only be used on a type that is always assumed to also implement the internal protocol by Gradle internals.

That makes sense yes, I though you meant adding it to the new type. My bad

abstratt

LGTM!

Fix convention mapping compatibility for DirectoryProperty and Regula…

36edec1

…rFileProperty

asodja self-assigned this May 16, 2024

asodja requested a review from a team May 16, 2024 12:29

gitstream-cm bot added the 5 min review label May 16, 2024

asodja marked this pull request as ready for review May 16, 2024 13:39

asodja requested a review from a team as a code owner May 16, 2024 13:39

asodja requested review from bamboo and abstratt and removed request for a team May 16, 2024 13:39

lptr reviewed May 16, 2024

View reviewed changes

abstratt reviewed May 16, 2024

View reviewed changes

Address review comments for FileSystemLocationProperty in convention …

a591014

…mapping

asodja requested review from lptr and abstratt May 16, 2024 16:36

Remove ConventionMappingFileSystemLocationPropertyProxy and introduce…

8023211

… FileSystemLocationPropertyInternal

asodja requested a review from a team as a code owner May 17, 2024 09:53

abstratt reviewed May 17, 2024

View reviewed changes

Annotate FileSystemLocationProperty with HasInternalProtocol

6ce4691

asodja requested a review from abstratt May 17, 2024 13:22

abstratt approved these changes May 17, 2024

View reviewed changes

asodja added this pull request to the merge queue May 17, 2024

bot-gradle added this to the 8.9 RC1 milestone May 17, 2024

Merged via the queue into master with commit 9f9e233 May 17, 2024
25 checks passed

asodja deleted the asodja/convention-mapping-files-fix branch May 17, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180

Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180

asodja commented May 16, 2024

asodja commented May 16, 2024

bot-gradle commented May 16, 2024

asodja commented May 16, 2024

lptr May 16, 2024

abstratt May 16, 2024

asodja May 16, 2024

abstratt May 16, 2024

asodja May 17, 2024 •

edited

lptr May 16, 2024

asodja May 16, 2024

lptr May 16, 2024

asodja May 16, 2024

lptr May 16, 2024

asodja May 16, 2024

abstratt May 16, 2024

asodja May 17, 2024

abstratt May 16, 2024

asodja May 16, 2024 •

edited

asodja commented May 17, 2024

bot-gradle commented May 17, 2024

abstratt May 17, 2024

asodja May 17, 2024 •

edited

abstratt May 17, 2024

asodja May 17, 2024

asodja May 17, 2024

abstratt left a comment

	convention(file.map(any -> {
	convention(provider.map(value -> {

Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180

Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180

Conversation

asodja commented May 16, 2024

asodja commented May 16, 2024

bot-gradle commented May 16, 2024

asodja commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asodja May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asodja May 16, 2024 • edited

Choose a reason for hiding this comment

asodja commented May 17, 2024

bot-gradle commented May 17, 2024

Choose a reason for hiding this comment

asodja May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abstratt left a comment

Choose a reason for hiding this comment

asodja May 17, 2024 •

edited

asodja May 16, 2024 •

edited

asodja May 17, 2024 •

edited