Thanks for the feedback =) So you're suggesting something like - FileCopyDetails.setMode() can be used on individual files - dirMode/fileMode can be used as 'global' overrides - preserveModes can be used to preserve all modes - by default, use standard dir/file permissions, but preserve the executable bit
Some thoughts: - The order above would be the actual precedence, sounds right? - dirMode+fileMode & preserveModes are mutually exclusive, should this be validated somewhere? CopySpecImpl? - preserveModes might not be necessary? In my case it's the executable bit that must be preserved. I'm guessing that's the most common scenario. - .. also, if the executable bit is preserved by default, do you need some way of suppressing this behavior? >> So, it would be nice to add chmod() and getUnixMode() methods to FileSystem. I could do that, but would it be ok to just implement them in terms of PosixUtil? At least you can easily change it to something else later on.. >> I think FileTreeElement.getMode() should return the mode that will actually >> be used by the copy when you are using CopySpec.eachFile(). Sounds right... >> This would mean moving EmptyCopySpecVisitor.getFileMode() to >> MappingCopySpecVisitor, and having the file, tar and zip visitors simply >> using FileTreeElement.getMode() to set the mode. EmptyCopySpecVisitor was just a temporary location for the new file-mode calculation. I think I'll have to pull out the debugger an actually step through a copy, not sure how MappingCopySpecVisitor is used. Btw, FileCopySpecVisitor didn't seem to support dirMode/fileMode at all, so I deliberately only added support for the new preserveModes. I could change this as well if you're not worried about compatibility issues. -- Lars Hvile On Sun, 2012-01-08 at 22:02 +0100, Adam Murdoch wrote: > > 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 > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
