Merge pull request gradle#27011 Fix NormalizingCopyActionDecorator to…

… respect directory permissions NormalizingCopyActionDecorator ignores file permissions set from dirPermissions. It's hard to eliminate `ParentDirectoryStub` completely as it might require bigger refactoring of `AbstractFileTreeElement` and related file APIs. ### Context The problem arose during implementation of gradle#25264 ### Reviewing cheatsheet Before merging the PR, comments starting with - ❌ ❓**must** be fixed - 🤔 💅 **should** be fixed - 💭 **may** be fixed - 🎉 celebrate happy things Co-authored-by: Vlad Chesnokov <[email protected]>
ljacomet · Nov 22, 2023 · 4486781 · 4486781
2 parents 341c72f + e787af5
commit 4486781
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 71 deletions.
diff --git a/.../file-collections/src/main/java/org/gradle/api/internal/file/AbstractFileTreeElement.java b/.../file-collections/src/main/java/org/gradle/api/internal/file/AbstractFileTreeElement.java
@@ -18,8 +18,8 @@
 import org.apache.commons.io.IOUtils;
 import org.gradle.api.GradleException;
 import org.gradle.api.UncheckedIOException;
-import org.gradle.api.file.FileTreeElement;
 import org.gradle.api.file.FilePermissions;
+import org.gradle.api.file.FileTreeElement;
 import org.gradle.internal.exceptions.Contextual;
 import org.gradle.internal.file.Chmod;
 import org.gradle.util.internal.GFileUtils;
@@ -117,8 +117,8 @@ public FilePermissions getPermissions() {
     }
 
     @Contextual
-    private static class CopyFileElementException extends GradleException {
-        CopyFileElementException(String message, Throwable cause) {
+    protected static class CopyFileElementException extends GradleException {
+        public CopyFileElementException(String message, Throwable cause) {
             super(message, cause);
         }
     }

diff --git a/...ects/core/src/integTest/groovy/org/gradle/api/tasks/CopyPermissionsIntegrationTest.groovy b/...ects/core/src/integTest/groovy/org/gradle/api/tasks/CopyPermissionsIntegrationTest.groovy
@@ -495,14 +495,40 @@ class CopyPermissionsIntegrationTest extends AbstractIntegrationSpec implements
         assertDestinationFilePermissions("r-xr-xrw-")
 
         where:
-        description         | setting
-        "permissions"       | """
+        description        | setting
+        "permissions"      | """
                                 eachFile {
                                     permissions = p
                                 }
                               """
-        "file mode"         | "fileMode = p.toUnixNumeric()"
-        "file permissions"  | "filePermissions.set(p)"
+        "file mode"        | "fileMode = p.toUnixNumeric()"
+        "file permissions" | "filePermissions.set(p)"
+    }
+
+    @Requires(UnitTestPreconditions.FilePermissions)
+    def "permissions are set correctly for intermediate directories"() {
+        given:
+        withSourceFiles("r--------")
+
+        buildFile.delete()
+        buildKotlinFile.text = '''
+            tasks.register<Copy>("copy") {
+               into("dest")
+               into("prefix1/prefix2") {
+                 from("files")
+               }
+               dirPermissions {
+                  unix("rwxr-x---")
+               }
+            }
+        '''.stripIndent()
+
+        when:
+        run 'copy'
+
+        then:
+        file("dest/prefix1").permissions == "rwxr-x---"
+        file("dest/prefix1/prefix2").permissions == "rwxr-x---"
     }
 
     private def withSourceFiles(String permissions) {

diff --git a/subprojects/core/src/main/java/org/gradle/api/internal/file/copy/DefaultFileCopyDetails.java b/subprojects/core/src/main/java/org/gradle/api/internal/file/copy/DefaultFileCopyDetails.java
@@ -23,8 +23,8 @@
 import org.gradle.api.file.ContentFilterable;
 import org.gradle.api.file.DuplicatesStrategy;
 import org.gradle.api.file.ExpandDetails;
-import org.gradle.api.file.FileVisitDetails;
 import org.gradle.api.file.FilePermissions;
+import org.gradle.api.file.FileVisitDetails;
 import org.gradle.api.file.RelativePath;
 import org.gradle.api.internal.file.AbstractFileTreeElement;
 import org.gradle.api.internal.file.DefaultConfigurableFilePermissions;
@@ -64,11 +64,6 @@ public DefaultFileCopyDetails(FileVisitDetails fileDetails, CopySpecResolver spe
         this.defaultDuplicatesStrategy = specResolver.isDefaultDuplicateStrategy();
     }
 
-    @Override
-    public boolean isIncludeEmptyDirs() {
-        return specResolver.getIncludeEmptyDirs();
-    }
-
     @Override
     public String getDisplayName() {
         return fileDetails.toString();
@@ -268,6 +263,11 @@ public boolean isDefaultDuplicatesStrategy() {
         return defaultDuplicatesStrategy;
     }
 
+    @Override
+    public CopySpecResolver getSpecResolver() {
+        return specResolver;
+    }
+
     @Override
     public String getSourceName() {
         return this.fileDetails.getName();

diff --git a/...rojects/core/src/main/java/org/gradle/api/internal/file/copy/FileCopyDetailsInternal.java b/...rojects/core/src/main/java/org/gradle/api/internal/file/copy/FileCopyDetailsInternal.java
@@ -20,7 +20,7 @@
 
 public interface FileCopyDetailsInternal extends FileCopyDetails {
 
-    boolean isIncludeEmptyDirs();
-
     boolean isDefaultDuplicatesStrategy();
+
+    CopySpecResolver getSpecResolver();
 }
diff --git a/.../core/src/main/java/org/gradle/api/internal/file/copy/NormalizingCopyActionDecorator.java b/.../core/src/main/java/org/gradle/api/internal/file/copy/NormalizingCopyActionDecorator.java
@@ -20,17 +20,20 @@
 import groovy.lang.Closure;
 import org.gradle.api.Action;
 import org.gradle.api.Transformer;
+import org.gradle.api.file.ConfigurableFilePermissions;
 import org.gradle.api.file.ContentFilterable;
 import org.gradle.api.file.DuplicatesStrategy;
 import org.gradle.api.file.ExpandDetails;
-import org.gradle.api.file.ConfigurableFilePermissions;
 import org.gradle.api.file.FilePermissions;
 import org.gradle.api.file.RelativePath;
 import org.gradle.api.internal.file.AbstractFileTreeElement;
 import org.gradle.api.internal.file.CopyActionProcessingStreamAction;
+import org.gradle.api.provider.Provider;
 import org.gradle.api.tasks.WorkResult;
 import org.gradle.internal.file.Chmod;
+import org.gradle.util.internal.GFileUtils;
 
+import javax.annotation.Nullable;
 import java.io.File;
 import java.io.FilterReader;
 import java.io.InputStream;
@@ -68,16 +71,16 @@ public WorkResult execute(final CopyActionProcessingStream stream) {
                         pendingDirs.put(path, details);
                     }
                 } else {
-                    maybeVisit(details.getRelativePath().getParent(), details.isIncludeEmptyDirs(), action, visitedDirs, pendingDirs);
+                    maybeVisit(details.getRelativePath().getParent(), details.getSpecResolver(), action, visitedDirs, pendingDirs);
                     action.processFile(details);
                 }
             });
 
             for (RelativePath path : new LinkedHashSet<>(pendingDirs.keySet())) {
                 List<FileCopyDetailsInternal> detailsList = new ArrayList<>(pendingDirs.get(path));
                 for (FileCopyDetailsInternal details : detailsList) {
-                    if (details.isIncludeEmptyDirs()) {
-                        maybeVisit(path, details.isIncludeEmptyDirs(), action, visitedDirs, pendingDirs);
+                    if (details.getSpecResolver().getIncludeEmptyDirs()) {
+                        maybeVisit(path, details.getSpecResolver(), action, visitedDirs, pendingDirs);
                     }
                 }
             }
@@ -87,37 +90,39 @@ public WorkResult execute(final CopyActionProcessingStream stream) {
         });
     }
 
-    private void maybeVisit(RelativePath path, boolean includeEmptyDirs, CopyActionProcessingStreamAction delegateAction, Set<RelativePath> visitedDirs, ListMultimap<RelativePath, FileCopyDetailsInternal> pendingDirs) {
+    private void maybeVisit(@Nullable RelativePath path, CopySpecResolver specResolver, CopyActionProcessingStreamAction delegateAction, Set<RelativePath> visitedDirs, ListMultimap<RelativePath, FileCopyDetailsInternal> pendingDirs) {
         if (path == null || path.getParent() == null || !visitedDirs.add(path)) {
             return;
         }
-        maybeVisit(path.getParent(), includeEmptyDirs, delegateAction, visitedDirs, pendingDirs);
-        List<FileCopyDetailsInternal> detailsForPath = pendingDirs.removeAll(path);
 
+        List<FileCopyDetailsInternal> detailsForPath = pendingDirs.removeAll(path);
         FileCopyDetailsInternal dir;
         if (detailsForPath.isEmpty()) {
             // TODO - this is pretty nasty, look at avoiding using a time bomb stub here
-            dir = new StubbedFileCopyDetails(path, includeEmptyDirs, chmod);
+            dir = new ParentDirectoryStub(specResolver, path, chmod);
         } else {
             dir = detailsForPath.get(0);
         }
+        maybeVisit(path.getParent(), dir.getSpecResolver(), delegateAction, visitedDirs, pendingDirs);
+
         delegateAction.processFile(dir);
     }
 
-    private static class StubbedFileCopyDetails extends AbstractFileTreeElement implements FileCopyDetailsInternal {
+    private static class ParentDirectoryStub extends AbstractFileTreeElement implements FileCopyDetailsInternal {
         private final RelativePath path;
-        private final boolean includeEmptyDirs;
-        private long lastModified = System.currentTimeMillis();
 
-        private StubbedFileCopyDetails(RelativePath path, boolean includeEmptyDirs, Chmod chmod) {
+        private final long lastModified = System.currentTimeMillis();
+
+        private final CopySpecResolver specResolver;
+
+        private ParentDirectoryStub(CopySpecResolver specResolver, RelativePath path, Chmod chmod) {
             super(chmod);
             this.path = path;
-            this.includeEmptyDirs = includeEmptyDirs;
+            this.specResolver = specResolver;
         }
 
-        @Override
-        public boolean isIncludeEmptyDirs() {
-            return includeEmptyDirs;
+        private static UnsupportedOperationException unsupported() {
+            return new UnsupportedOperationException("It's a synthetic FileCopyDetails just to create parent directories");
         }
 
         @Override
@@ -127,12 +132,12 @@ public String getDisplayName() {
 
         @Override
         public File getFile() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public boolean isDirectory() {
-            return !path.isFile();
+            return true;
         }
 
         @Override
@@ -142,112 +147,138 @@ public long getLastModified() {
 
         @Override
         public long getSize() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public InputStream open() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public RelativePath getRelativePath() {
             return path;
         }
 
+        @Override
+        public boolean copyTo(File target) {
+            try {
+                GFileUtils.mkdirs(target);
+                getChmod().chmod(target, getPermissions().toUnixNumeric());
+                return true;
+            } catch (Exception e) {
+                throw new CopyFileElementException(String.format("Could not copy %s to '%s'.", getDisplayName(), target), e);
+            }
+        }
+
+        @Override
+        public FilePermissions getPermissions() {
+            Provider<FilePermissions> specMode = specResolver.getImmutableDirPermissions();
+            if (specMode.isPresent()) {
+                return specMode.get();
+            }
+
+            return super.getPermissions();
+        }
+
         @Override
         public void exclude() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setName(String name) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setPath(String path) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setRelativePath(RelativePath path) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setMode(int mode) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void permissions(Action<? super ConfigurableFilePermissions> configureAction) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setPermissions(FilePermissions permissions) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public void setDuplicatesStrategy(DuplicatesStrategy strategy) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public DuplicatesStrategy getDuplicatesStrategy() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public boolean isDefaultDuplicatesStrategy() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public String getSourceName() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public String getSourcePath() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public RelativePath getRelativeSourcePath() {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable filter(Map<String, ?> properties, Class<? extends FilterReader> filterType) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable filter(Class<? extends FilterReader> filterType) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable filter(Closure closure) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable filter(Transformer<String, String> transformer) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable expand(Map<String, ?> properties) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
         }
 
         @Override
         public ContentFilterable expand(Map<String, ?> properties, Action<? super ExpandDetails> action) {
-            throw new UnsupportedOperationException();
+            throw unsupported();
+        }
+
+        @Override
+        public CopySpecResolver getSpecResolver() {
+            return specResolver;
         }
     }
 }