Ticket #3124 (closed enhancement: fixed )

Opened 1 year ago

Last modified 1 year ago

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

Reported by: cyli Assigned to: therve
Type: enhancement Priority: high
Milestone: Component: core
Keywords: Cc: therve, exarkun
Branch: branches/fp-chmod-3124 Author: therve
Launchpad Bug:

Description (last modified by therve)

Enable chmodding.

Attachments

filepath.chown.chmod.patch (2.7 kB) - added by cyli 1 year ago.
filepath.chown.chmod.2.patch (4.5 kB) - added by cyli 1 year ago.
filepath.chown.chmod.3.patch (3.5 kB) - added by cyli 1 year ago.
filepath.chown.chmod.4.patch (1.6 kB) - added by cyli 1 year ago.

Change History

  2008-03-27 18:59:04+00:00 changed by 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.

  2008-03-27 20:34:03+00:00 changed by cyli

  • keywords set to review
  • owner deleted
  • priority changed from normal to highest

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

  2008-03-27 20:34:19+00:00 changed by cyli

  • attachment filepath.chown.chmod.patch added

follow-up: ↓ 4   2008-03-28 15:51:59+00:00 changed by therve

  • cc set to therve
  • keywords deleted
  • owner set to cyli
  • priority changed from highest to high
  • 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!

in reply to: ↑ 3   2008-03-28 23:27:52+00:00 changed by 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?

  2008-03-28 23:29:17+00:00 changed by 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."

  2008-04-03 01:17:06+00:00 changed by cyli

  • attachment filepath.chown.chmod.2.patch added

  2008-04-03 01:17:47+00:00 changed by cyli

  • keywords set to review
  • owner deleted
  • priority changed from high to highest

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.

  2008-04-03 18:53:07+00:00 changed by therve

  • keywords deleted
  • owner set to cyli
  • 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!

  2008-04-03 19:42:47+00:00 changed by cyli

  • attachment filepath.chown.chmod.3.patch added

  2008-04-03 19:43:40+00:00 changed by cyli

  • keywords set to review
  • owner deleted

Changes made, thanks. :)

  2008-04-04 08:50:11+00:00 changed by therve

  • branch set to branches/fp-chown-chmod-3124
  • author set to therve

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

  2008-04-04 09:02:06+00:00 changed by therve

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

Refs #3124

follow-up: ↓ 12   2008-04-04 15:43:20+00:00 changed by exarkun

  • cc changed from therve to therve, exarkun
  • keywords deleted
  • owner set to therve

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 :).

in reply to: ↑ 11   2008-04-05 07:04:31+00:00 changed by cyli

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

  2008-04-05 09:39:52+00:00 changed by therve

  • owner changed from therve to cyli
  • priority changed from highest to high

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

  2008-04-20 20:52:23+00:00 changed by cyli

  • attachment filepath.chown.chmod.4.patch added

  2008-04-20 20:53:27+00:00 changed by cyli

  • keywords set to review
  • owner deleted

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

follow-up: ↓ 16   2008-04-21 08:33:57+00:00 changed by therve

Could you quickly explain why?

in reply to: ↑ 15   2008-04-21 21:16:27+00:00 changed by 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.

  2008-04-23 08:56:11+00:00 changed by therve

  • keywords deleted
  • owner set to therve
  • description deleted
  • summary changed from Enable filepath.FilePath to change permissions and ownership of a file or directory to Enable filepath.FilePath to change permissions of a file or directory

Previous description was:

Enable chmodding and chowning.

  2008-04-23 08:58:18+00:00 changed by therve

  • branch changed from branches/fp-chown-chmod-3124 to branches/fp-chmod-3124

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

  2008-04-23 09:17:05+00:00 changed by therve

  • status changed from new to closed
  • resolution set to fixed

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

Author: cyli Reviewers: therve, exarkun Fixes #3124

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

Note: See TracTickets for help on using tickets.