On 09/01/2012, at 4:41 AM, Lars Hvile wrote: > I stumbled onto some known issues regarding file permissions in my current > project, and decided to try to fix them. I've never worked with the Gradle > source before, so I'm interested in some feedback before going any further > with this. > > I've committed a simple, first draft. No tests other than manual verification > yet.. > https://github.com/larshvile/gradle/commit/2025b546536177c6816bad633fa83ba36fb76a8a > > Basically I've added FileTreeElement.getMode() and > CopyProcessingSpec.setPreserveModes(). I'm assuming that backwards > compatibility is important here (?). The current solution should hopefully > behave 100% like the old one, unless the new 'preserveModes' option is used. > However, preserving the original modes seems like a reasonable default to me..
It does. And it's more or less backwards compatible. One downside of using the original mode as a default, is that the default is then dependent on the user's umask setting, and we prefer to have the build output be as independent of the environment as possible. Another option is to preserve only the executable permission by default. > > Could someone please verify that this looks sane =) ? It looks excellent. I think FileTreeElement.getMode() should return the mode that will actually be used by the copy when you are using CopySpec.eachFile(). This would mean moving EmptyCopySpecVisitor.getFileMode() to MappingCopySpecVisitor, and having the file, tar and zip visitors simply using FileTreeElement.getMode() to set the mode. It might also be good to add FileCopyDetails.setMode(), too, so that I can write custom logic to specify the permissions for a particular file. Implementation-wise (if you feel like doing this), we have been moving away from using PosixUtil directly, and changing things do file-related stuff using FileSystem. So, it would be nice to add chmod() and getUnixMode() methods to FileSystem. Also, it might be nice to fall back to using File.setReadable(), setWritable(), setExecutable() or Java 7's PosixFileAttributeView or 'chmod' when PosixUtil is not available (eg on AIX, or if the JNA lib has already been loaded by another classloader). But let's leave that until we have the basic implementation merged (as long as it doesn't blow up on these platforms, but we have some CI builds for this). > And if it is, what's the best practice for testing this kind of stuff? > Creating integration tests within subprojects/integ-test? Right. There's already CopyTaskIntegrationTest and ArchiveIntegrationTest, so you might add some more test cases to these classes. They could do with some splitting up, so you could also start some new integration test classes, and we'll shuffle around the test cases later. New test cases should extend AbstractIntegrationSpec. -- Adam Murdoch Gradle Co-founder http://www.gradle.org VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting http://www.gradleware.com
