Cool stuff =) I did however notice that the new tests failed on one of the builds, Java 1.6 Commit Build on unknown OS..
I've tested the changes on linux and windows (posix permissions don't work too well on windows, so the windows chmod() implementation is just a NOP, while getUnixMode() simply returns some sane defaults). -- Lars Hvile On Wed, 2012-01-18 at 22:19 +0100, Adam Murdoch wrote: > > > This is pushed now. Thanks for the patch. I'm looking forward to the > next one. > > On 18/01/2012, at 9:27 AM, Lars Hvile wrote: > > > Hey.. Could someone w/commit access take a look at the first pull > > request for this issue? It's just some additions to FileSystem. It > > would > > be cool to get these changes in before I fix the remaining copy-task > > permission issues.. > > > > Thanks.. > > > > -- > > 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 > > > > > > > > > -- > 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
