Preserve Unix file permissions when caching attachedOutputs (#392)

cowwoc · web-flow · commit f6c64851849d · 2025-10-18T23:42:38.000+02:00
diff --git a/pom.xml b/pom.xml
@@ -139,6 +139,11 @@ under the License.
       <artifactId>commons-io</artifactId>
       <version>2.20.0</version>
     </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-compress</artifactId>
+      <version>1.28.0</version>
+    </dependency>
     <dependency>
       <groupId>javax.annotation</groupId>
       <artifactId>javax.annotation-api</artifactId>
diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java
@@ -881,7 +881,7 @@ private boolean zipAndAttachArtifact(MavenProject project, Path dir, String clas
             throws IOException {
         Path temp = Files.createTempFile("maven-incremental-", project.getArtifactId());
         temp.toFile().deleteOnExit();
-        boolean hasFile = CacheUtils.zip(dir, temp, glob);
+        boolean hasFile = CacheUtils.zip(dir, temp, glob, cacheConfig.isPreservePermissions());
         if (hasFile) {
             projectHelper.attachArtifact(project, "zip", classifier, temp.toFile());
         }
@@ -896,7 +896,7 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M
         if (!Files.exists(outputDir)) {
             Files.createDirectories(outputDir);
         }
-        CacheUtils.unzip(artifactFilePath, outputDir);
+        CacheUtils.unzip(artifactFilePath, outputDir, cacheConfig.isPreservePermissions());
     }
 
     // TODO: move to config
diff --git a/src/main/java/org/apache/maven/buildcache/CacheUtils.java b/src/main/java/org/apache/maven/buildcache/CacheUtils.java
@@ -29,15 +29,18 @@
 import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
+import java.nio.file.attribute.PosixFilePermission;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.Set;
 import java.util.stream.Stream;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipInputStream;
-import java.util.zip.ZipOutputStream;
 
+import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
+import org.apache.commons.compress.archivers.zip.ZipArchiveInputStream;
+import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Strings;
 import org.apache.commons.lang3.mutable.MutableBoolean;
@@ -154,12 +157,23 @@ public static boolean isArchive(File file) {
      * @param dir directory to zip
      * @param zip zip to populate
      * @param glob glob to apply to filenames
+     * @param preservePermissions whether to preserve Unix file permissions in the zip.
+     *                           <p><b>Important:</b> When {@code true}, permissions are stored in ZIP entry headers,
+     *                           which means they become part of the ZIP file's binary content. As a result, hashing
+     *                           the ZIP file (e.g., for cache keys) will include permission information, ensuring
+     *                           cache invalidation when file permissions change. This behavior is similar to how Git
+     *                           includes file mode in tree hashes.</p>
      * @return true if at least one file has been included in the zip.
      * @throws IOException
      */
-    public static boolean zip(final Path dir, final Path zip, final String glob) throws IOException {
+    public static boolean zip(final Path dir, final Path zip, final String glob, boolean preservePermissions)
+            throws IOException {
         final MutableBoolean hasFiles = new MutableBoolean();
-        try (ZipOutputStream zipOutputStream = new ZipOutputStream(Files.newOutputStream(zip))) {
+        // Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
+        final boolean supportsPosix = preservePermissions
+                && dir.getFileSystem().supportedFileAttributeViews().contains("posix");
+
+        try (ZipArchiveOutputStream zipOutputStream = new ZipArchiveOutputStream(Files.newOutputStream(zip))) {
 
             PathMatcher matcher =
                     "*".equals(glob) ? null : FileSystems.getDefault().getPathMatcher("glob:" + glob);
@@ -170,12 +184,19 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
                         throws IOException {
 
                     if (matcher == null || matcher.matches(path.getFileName())) {
-                        final ZipEntry zipEntry =
-                                new ZipEntry(dir.relativize(path).toString());
-                        zipOutputStream.putNextEntry(zipEntry);
+                        final ZipArchiveEntry zipEntry =
+                                new ZipArchiveEntry(dir.relativize(path).toString());
+
+                        // Preserve Unix permissions if requested and filesystem supports it
+                        if (supportsPosix) {
+                            Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(path);
+                            zipEntry.setUnixMode(permissionsToMode(permissions));
+                        }
+
+                        zipOutputStream.putArchiveEntry(zipEntry);
                         Files.copy(path, zipOutputStream);
                         hasFiles.setTrue();
-                        zipOutputStream.closeEntry();
+                        zipOutputStream.closeArchiveEntry();
                     }
                     return FileVisitResult.CONTINUE;
                 }
@@ -184,9 +205,13 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
         return hasFiles.booleanValue();
     }
 
-    public static void unzip(Path zip, Path out) throws IOException {
-        try (ZipInputStream zis = new ZipInputStream(Files.newInputStream(zip))) {
-            ZipEntry entry = zis.getNextEntry();
+    public static void unzip(Path zip, Path out, boolean preservePermissions) throws IOException {
+        // Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
+        final boolean supportsPosix = preservePermissions
+                && out.getFileSystem().supportedFileAttributeViews().contains("posix");
+
+        try (ZipArchiveInputStream zis = new ZipArchiveInputStream(Files.newInputStream(zip))) {
+            ZipArchiveEntry entry = zis.getNextEntry();
             while (entry != null) {
                 Path file = out.resolve(entry.getName());
                 if (!file.normalize().startsWith(out.normalize())) {
@@ -200,6 +225,16 @@ public static void unzip(Path zip, Path out) throws IOException {
                     Files.copy(zis, file, StandardCopyOption.REPLACE_EXISTING);
                 }
                 Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
+
+                // Restore Unix permissions if requested and filesystem supports it
+                if (supportsPosix) {
+                    int unixMode = entry.getUnixMode();
+                    if (unixMode != 0) {
+                        Set<PosixFilePermission> permissions = modeToPermissions(unixMode);
+                        Files.setPosixFilePermissions(file, permissions);
+                    }
+                }
+
                 entry = zis.getNextEntry();
             }
         }
@@ -217,4 +252,64 @@ public static <T> void debugPrintCollection(
             }
         }
     }
+
+    /**
+     * Convert POSIX file permissions to Unix mode integer, following Git's approach of only
+     * preserving the owner executable bit.
+     *
+     * <p>Git stores file permissions as either {@code 100644} (non-executable) or {@code 100755}
+     * (executable). This simplified approach focuses on the functional aspect (executability)
+     * while ignoring platform-specific permission details that are generally irrelevant for
+     * cross-platform builds.</p>
+     *
+     * @param permissions POSIX file permissions
+     * @return Unix mode: {@code 0100755} if owner-executable, {@code 0100644} otherwise
+     */
+    private static int permissionsToMode(Set<PosixFilePermission> permissions) {
+        // Following Git's approach: preserve only the owner executable bit
+        // Git uses 100644 (rw-r--r--) for regular files and 100755 (rwxr-xr-x) for executables
+        if (permissions.contains(PosixFilePermission.OWNER_EXECUTE)) {
+            return 0100755; // Regular file, executable
+        } else {
+            return 0100644; // Regular file, non-executable
+        }
+    }
+
+    /**
+     * Convert Unix mode integer to POSIX file permissions, following Git's simplified approach.
+     *
+     * <p>This method interprets the two Git-standard modes:</p>
+     * <ul>
+     *   <li>{@code 0100755} - Executable file: sets owner+group+others read/execute, owner write</li>
+     *   <li>{@code 0100644} - Regular file: sets owner+group+others read, owner write</li>
+     * </ul>
+     *
+     * <p>The key distinction is the presence of the execute bit. Other permission variations
+     * are normalized to these two standard patterns for portability.</p>
+     *
+     * @param mode Unix mode (should be either {@code 0100755} or {@code 0100644})
+     * @return Set of POSIX file permissions
+     */
+    private static Set<PosixFilePermission> modeToPermissions(int mode) {
+        Set<PosixFilePermission> permissions = new HashSet<>();
+
+        // Check owner executable bit (following Git's approach)
+        if ((mode & 0100) != 0) {
+            // Mode 100755: rwxr-xr-x (executable file)
+            permissions.add(PosixFilePermission.OWNER_READ);
+            permissions.add(PosixFilePermission.OWNER_WRITE);
+            permissions.add(PosixFilePermission.OWNER_EXECUTE);
+            permissions.add(PosixFilePermission.GROUP_READ);
+            permissions.add(PosixFilePermission.GROUP_EXECUTE);
+            permissions.add(PosixFilePermission.OTHERS_READ);
+            permissions.add(PosixFilePermission.OTHERS_EXECUTE);
+        } else {
+            // Mode 100644: rw-r--r-- (regular file)
+            permissions.add(PosixFilePermission.OWNER_READ);
+            permissions.add(PosixFilePermission.OWNER_WRITE);
+            permissions.add(PosixFilePermission.GROUP_READ);
+            permissions.add(PosixFilePermission.OTHERS_READ);
+        }
+        return permissions;
+    }
 }
diff --git a/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java b/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java
@@ -108,6 +108,8 @@ public interface CacheConfig {
 
     List<DirName> getAttachedOutputs();
 
+    boolean isPreservePermissions();
+
     boolean adjustMetaInfVersion();
 
     boolean calculateProjectVersionChecksum();
diff --git a/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java b/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java
@@ -578,6 +578,13 @@ public List<DirName> getAttachedOutputs() {
         return attachedOutputs == null ? Collections.emptyList() : attachedOutputs.getDirNames();
     }
 
+    @Override
+    public boolean isPreservePermissions() {
+        checkInitializedState();
+        final AttachedOutputs attachedOutputs = getConfiguration().getAttachedOutputs();
+        return attachedOutputs == null || attachedOutputs.isPreservePermissions();
+    }
+
     @Override
     public boolean adjustMetaInfVersion() {
         if (isEnabled()) {
diff --git a/src/main/mdo/build-cache-config.mdo b/src/main/mdo/build-cache-config.mdo
@@ -376,6 +376,12 @@ under the License.
             <name>AttachedOutputs</name>
             <description>Section relative to outputs which are not artifacts but need to be saved/restored.</description>
             <fields>
+                <field>
+                    <name>preservePermissions</name>
+                    <type>boolean</type>
+                    <defaultValue>true</defaultValue>
+                    <description>Preserve Unix file permissions when saving/restoring attached outputs. When enabled, permissions are stored in ZIP entry headers and become part of the cache key, ensuring cache invalidation when permissions change. This is similar to how Git includes file mode in tree hashes. Disabling this may improve portability across different systems but will not preserve executable bits.</description>
+                </field>
                 <field>
                     <name>dirNames</name>
                     <association>
diff --git a/src/test/java/org/apache/maven/buildcache/CacheUtilsPermissionsTest.java b/src/test/java/org/apache/maven/buildcache/CacheUtilsPermissionsTest.java

Original file line number	Diff line number	Diff line change
`@@ -881,7 +881,7 @@ private boolean zipAndAttachArtifact(MavenProject project, Path dir, String clas`
`881`	`881`	`throws IOException {`
`882`	`882`	`Path temp = Files.createTempFile("maven-incremental-", project.getArtifactId());`
`883`	`883`	`temp.toFile().deleteOnExit();`
`884`		`- boolean hasFile = CacheUtils.zip(dir, temp, glob);`
	`884`	`+ boolean hasFile = CacheUtils.zip(dir, temp, glob, cacheConfig.isPreservePermissions());`
`885`	`885`	`if (hasFile) {`
`886`	`886`	`projectHelper.attachArtifact(project, "zip", classifier, temp.toFile());`
`887`	`887`	`}`
`@@ -896,7 +896,7 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M`
`896`	`896`	`if (!Files.exists(outputDir)) {`
`897`	`897`	`Files.createDirectories(outputDir);`
`898`	`898`	`}`
`899`		`- CacheUtils.unzip(artifactFilePath, outputDir);`
	`899`	`+ CacheUtils.unzip(artifactFilePath, outputDir, cacheConfig.isPreservePermissions());`
`900`	`900`	`}`
`901`	`901`
`902`	`902`	`// TODO: move to config`