Opened a pull request on the changes to FileSystem now.. -- Lars Hvile
On Mon, 2012-01-09 at 06:03 +0100, Adam Murdoch wrote: > > 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 > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
