On 09/01/2012, at 10:35 AM, Lars Hvile wrote: > 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?
Thinking about it a bit more, I think the best behaviour is to preserve permissions by default. It feels like this would be the behaviour most people would expect as a default. Given this, we don't need preserveModes, as you just set dirMode and fileMode if you don't want to preserve the permissions, and FileCopyDetails.setMode() if you want to do something on a per-file basis, such as preserving only the execute permission. > > >>> 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.. That's fine. > > >>> 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. I think FileCopySpecVisitor should honour the modes. It's a mostly backwards-compatible change. > > > -- > 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 > > -- Adam Murdoch Gradle Co-founder http://www.gradle.org VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting http://www.gradleware.com
