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
