Enable filepath.FilePath to change permissions of a file or directory

[cyli]

Enable chmodding.

[cyli]

Hmm... I'm having problems trying to figure out how to test chown. All I'm doing is calling os.chown, but I can't figure out how to write a test that would work on any machine. If anyone has any ideas, please let me know.

[cyli]

Ok, just mocked the os.chown and os.chmod functions for testing. Here we go.

[therve]

• TestCase instances have a patch method that would be bit cleaner than your Verification class. See http://twistedmatrix.com/documents/current/api/twisted.trial.unittest.TestCase.html#patch
• the two tests are mostly good, but I wonder if tests that actually call the underlying functions would not be useful too. Maybe calling chown with the current process id. You'd just have to skip it under windows.
• maybe chown could do something a bit smarter, like taking string representing id. I'm thinking about that because of #3127: mktap.getid basically does this, but it should be moved somewhere (and probably tested). Again, it's not blocking this ticket.

Thanks!

[cyli]

Replying to therve:

• the two tests are mostly good, but I wonder if tests that actually call the underlying functions would not be useful too. Maybe calling chown with the current process id. You'd just have to skip it under windows. This is what I was originally going to do with FilePath.chmod, but I wasn't sure it would work in windows. I also do not really understand your suggestion for chown - if there is no user whose id corresponds with the process id, it would fail. Do you mean, call it and make sure it fails? Do you mean calling it with the current process's user id? If so, how can you tell if it worked? (presumably files were originally created with the owner and group being the current process's user id).
• maybe chown could do something a bit smarter, like taking string representing id. I'm thinking about that because of #3127: mktap.getid basically does this, but it should be moved somewhere (and probably tested). Again, it's not blocking this ticket. Looking at the documentation (http://twistedmatrix.com/documents/current/api/twisted.scripts.mktap.html#getid) and code (http://twistedmatrix.com/trac/browser/trunk/twisted/scripts/mktap.py#L26) for mktap.getid, it seems to take a uid and gid and probably return the string representation (username.groupname ?). You seem to be suggesting that FilePath.chown does what mktap.getid does in reverse (take username.groupname and break it down to uid and gid, and then calling os.chown with the uid and gid). I wouldn't mind doing so, and it wouldn't be difficult, but should the getid and resolveid (my made-up name for the reverse of getid) methods don't seem like they belong in FilePath.

Also, should chown just raise an error in Windows?

[cyli]

oops, that should read: "I wouldn't mind doing so, and it wouldn't be difficult, getid and resolveid (my made-up name for the reverse of getid) methods don't seem like they belong in FilePath."

[cyli]

Alrighty, I added two additional tests: one that calls chmod, and one that calls chown. I also had chown accept either uid/gid's or usernames/group names, and added a test for this as well.

[therve]

• test_chmod should actually uses 0555 and 0777 to work under windows (my first guess was wrong)
• pwd and grp aren't available anywhere. I guess we should postpone this feature until we know where getid lands. Please remove this, I'll open a ticket eventually
• you don't have to call restore manually after a patch, it will done automatically
• test_chown_mockup should be named test_chownMockup (same for test_chmod_mockup)
• test_chown_mockup can't work on windows, because of some limitations of the patch system. On the other hand, the feature will never work on Windows, so we'd better just skip this test too under windows.
• test_chown docstring doesn't seem to be finished

Thanks!

[cyli]

Changes made, thanks. :)

[therve]

(In [23195]) Branching to 'fp-chown-chmod-3124'

[therve]

(In [23196]) Apply patch with some small changes.

Refs #3124

[exarkun]

test_chmodMockup seems pretty redundant with test_chmod. I'd just keep test_chmod, since it really tests what the API is supposed to do. test_chmodMockup is a whitebox test that doesn't seem to add any additional coverage.

Almost contrariwise, test_chown is nearly redundant with test_chownMockup. test_chown would succeed with a no-op implementation. Of course, test_chownMockup would fail, so together they may demonstrate that the implementation is sound. test_chownMockup is a bit whitebox, but since chown is not going to succeed if we try passing interesting arguments to it, that's probably a necessity. It'd be nice if we had a more structured way of having verified fakes (which is basically what these two tests in combination are achieving), but maybe that's out of scope. In any case, I don't see why test_chownMockup needs to be skipped on Windows. It shouldn't fail there, should it? :)

It might be nice to have something in the FilePath.chown docstring indicating that it isn't going to work on Windows. The -1 arguments are really stupid too but I guess that's Python's fault. I'm tempted to suggest we should use None here instead, but maybe diverging from the Python API is needless complexity (although I'm also tempted to suggest changeMode and changeOwner as method names too :).

[cyli]

Okay... do you want me to make these changes? Or therve, since he's already made the branch?

[therve]

Don't hesitate, I'll have no problem finding another ticket :).

[cyli]

As per discussion with exarkun, took out all chown stuff.

[therve]

Could you quickly explain why?

[cyli]

Could you quickly explain why?

Sure. There was some debate as to what chown's behavior should be in Windows, whether it should return a separate AttributeError? to disambiguate between os.chown's error, and whether it should be in FilePath? at all or whether FilePath? should be split/subclassed/wrapped to provide Unix-specific functionality and Windows specific functionality. At the end of the huge discussion, since I said I didn't actually need chown and I was just adding it to b complete/symmetric (because I needed chmod), everyone (the April 20th sprint) said that I should just take out the chown functionality until they can figure out what they want for FilePath?. It could always be reinstated in a future patch, but once it's in the API'd have to be supported forever (or for a very long time, at least).

Since chmod works in both Windows and Unix, it was fine to leave in.

[therve]

Previous description was:

Enable chmodding and chowning.

[therve]

(In [23397]) Branching to 'fp-chmod-3124'

[therve]

(In [23400]) Merge fp-chmod-3124

Author: cyli Reviewers: therve, exarkun Fixes #3124

Add the chmod method to FIlePath objects, allowing to change permissions.