Closed Bug 868574 Opened 9 years ago Closed 8 years ago

devicemanager.mkDirs does not work on root's children

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(4 files, 3 obsolete files)

calling mkDirs with something like '/foo' will fail because of the split call.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → mihneadb
Attachment #745321 - Flags: review?(mcote)
Blocks: 868505
Again it would be nice to switch to posixpath functions here.
Attached patch use posixpath (obsolete) — Splinter Review
Attachment #745321 - Attachment is obsolete: true
Attachment #745321 - Flags: review?(mcote)
Attachment #745805 - Flags: review?(mcote)
Comment on attachment 745805 [details] [diff] [review]
use posixpath

Review of attachment 745805 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple little tweaks.

::: mozdevice/mozdevice/devicemanager.py
@@ +235,4 @@
>          WARNING: does not create last part of the path. For example, if asked to
>          create `/mnt/sdcard/foo/bar/baz`, it will only create `/mnt/sdcard/foo/bar`
>          """
> +        (containing, _) = posixpath.split(filename)

If you only care about the first part, you can just use posixpath.dirname().

@@ +240,1 @@
>              parts = filename.split('/')

So this could use posixpath, although admittedly there is no function to split a path into all its components.  I'm okay with keeping split("/") in this case as long as the path goes through normpath() first.
Attachment #745805 - Flags: review?(mcote) → review+
I don't want to change that split because (as you said) there's no equivalent in posixpath and the function would grow unnecessarily.
Attachment #745805 - Attachment is obsolete: true
Attachment #748179 - Flags: review?(mcote)
Attached patch improve dmsut.pushdir logic (obsolete) — Splinter Review
Attachment #748181 - Flags: review?(mcote)
Comment on attachment 748179 [details] [diff] [review]
normalize path in mkdirs

Review of attachment 748179 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  The only thing this is missing is a test.  Please modify sut_mkdir.py to test the creation of a directory off of root (make sure the test fails without this patch and passes with it).
Attachment #748179 - Flags: review?(mcote) → review+
I should say, please do the test as a separate patch and r? me.  I don't think we can verify this on a real device (via sut_tests) because I believe / is often mounted read-only.
Comment on attachment 748181 [details] [diff] [review]
improve dmsut.pushdir logic

Review of attachment 748181 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +375,2 @@
>  
> +                if (basename == ""):

This is not quite right.  parts[1] is actually the root directory, e.g. for /a/b/c it would be 'a'.  In fact, this breaks sut_tests/test_push2.py (please run *all* tests before submitting patches!).

I'm not sure of the best way to fix this; perhaps this whole loop needs to be rethought.
Attachment #748181 - Flags: review?(mcote) → review-
I reverted to the split method, as this works, but I kept the cleaner join() method.
All tests pass now.
Attachment #748181 - Attachment is obsolete: true
Attachment #757109 - Flags: review?
Attachment #757109 - Flags: review? → review?(mcote)
Oh and I had to take off the starting slash because posixpath.join("/a/b", "/c") == "/c" :).
Fails without the patches, passes with.
Attachment #757131 - Flags: review?(mcote)
Comment on attachment 757109 [details] [diff] [review]
use posixpath in dmsut.pushdir

Review of attachment 757109 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this makes the function clearer and it appears to work fine.  Just a few suggestions to make it even more clear and Pythonic. :) No need for rereview if you just make those changes.

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +369,5 @@
>          existentDirectories = []
>          for root, dirs, files in os.walk(localDir, followlinks=True):
> +            _, subpath = root.split(localDir)
> +            if subpath and subpath[0] == '/':
> +                subpath = subpath[1:]

You can replace both those lines with

subpath = subpath.lstrip('/')

@@ +374,2 @@
>              for f in files:
> +                remoteRoot = posixpath.join(remoteDir, subpath)

This doesn't actually have to be done on each iteration; might as well move it above the for loop.

@@ +377,2 @@
>  
> +                if (subpath == ""):

Since you're editing this line anyway, take out the extraneous parentheses.
Attachment #757109 - Flags: review?(mcote) → review+
Comment on attachment 748182 [details] [diff] [review]
update sut_push test to use proper paths

Review of attachment 748182 [details] [diff] [review]:
-----------------------------------------------------------------

This works fine now with your latest fix to pushDir().
Attachment #748182 - Flags: review?(mcote) → review+
Comment on attachment 757131 [details] [diff] [review]
test mkdirs for /foo

Review of attachment 757131 [details] [diff] [review]:
-----------------------------------------------------------------

Heh I keep forgetting that mkDirs() doesn't create the last path element, so mkDirs("/foo") is kind of pointless, but at least it doesn't error out now. :) Thanks!
Attachment #757131 - Flags: review?(mcote) → review+
(In reply to Mark Côté ( :mcote ) from comment #16)
> Comment on attachment 757131 [details] [diff] [review]
> test mkdirs for /foo
> 
> Review of attachment 757131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heh I keep forgetting that mkDirs() doesn't create the last path element, so
> mkDirs("/foo") is kind of pointless, but at least it doesn't error out now.
> :) Thanks!

Haha me too, I actually spent time "debugging" this!
https://github.com/mozilla/mozbase/commit/52cd9c8a98de666e6c5a4aef3bcc00ecf863b776
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.