778329 - devicemanagerSUT - invalid argument in unpackFile

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

rumCmds takes a list for dicts, here we are only passing a list of strings.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix the bug

369 is the line in cause.
Rest is just whitespace fixes.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

My bad, line 1107 in the diff.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 646754 [details] [diff] [review]
fix the bug

Giving this a r- because the real problem here is that there are two versions of unpackFile(), for some reason, and the second one is the broken one, which overrides the first (working) one. Please just delete the second one.

However while you are at it, the 'cd' in that command doesn't make any sense. The agent's unzp command takes an optional destination directory, and it makes a lot more sense to use that than to change the working directory.

So please make those changes, and ensure that there are appropriate tests for the command.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Add dest_dir param

Also created a test, adding it to bug #778366

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 647416 [details] [diff] [review]
Add dest_dir param

Looks good, thanks! I think some of the whitespace edits are unnecessary--it's okay to indent blank lines so long as they line up with the next line--but it's okay.

I only have one little nit:

>-    dir = ''
>-    parts = filename.split('/')
>-    if (len(parts) > 1):
>-      if self.fileExists(filename):
>-        dir = '/'.join(parts[:-1])
>-    elif self.fileExists('/' + filename):
>-      dir = '/' + filename
>-    elif self.fileExists(devroot + '/' + filename):
>-      dir = devroot + '/' + filename
>-    else:
>-      return None
>+    # if no dest_dir is passed in just set it to file_path's folder
>+    if not dest_dir:
>+      parts = file_path.split('/')
>+      dest_dir = '/'.join(parts[:-1])
>+
>+    if dest_dir[-1] != '/':
>+      dest_dir += '/'

Firstly it doesn't appear necessary to add a trailing slash to dest_dir. I just tried it without one, and it worked fine.

Secondly, you can simplify the dest_dir assignment to just

  dest_dir = posixpath.dirname(file_path)

Don't forget to import posixpath. We should be using this more in DeviceManager, since it's a lot clearer than splitting paths into parts and recombining them.

And finally, it's probably not a bad idea to submit this to try, just to be absolutely sure it doesn't bust anything.

Comment 6

[Mark Côté [:mcote]]

If you could just fix that one nit and submit another patch, I will push it to try to make sure it doesn't break anything, then we'll commit it. We are trying to be extra careful with things like DeviceManager. :)

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: updated according to feedback

I kept the check for the ending '/' because for me it doesn't work. If I don't do that check it will create "infratestpush2" instead of "infratest/push2".

Comment 8

[Mark Côté [:mcote]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=64b767ce51ff

Comment 9

[Mark Côté [:mcote]]

Try looked fine. Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/4f5a56859501

Comment 10

[Jeff Hammel]

This should be landed in the mozbase repo as well

Comment 11

[Mark Côté [:mcote]]

Yup, figured I'd wait until it landed in m-c first.

Comment 12

[Jeff Hammel]

(In reply to Mark Côté ( :mcote ) from comment #11)
> Yup, figured I'd wait until it landed in m-c first.

Sure, just figured I'd note for posterity

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f5a56859501

Comment 14

[Mark Côté [:mcote]]

Comment on attachment 647621 [details] [diff] [review]
updated according to feedback

Just realized I never officially reviewed the last patch. r+ :)