620772 - Windows/Mac Integration script for Jetpack SDK

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Ayan Shah[:ashah]]

I made some small changes in the lieu of bug 594076 and some papercut changes mentioned in bug 588222.

Warner - can you please do a review for this? This script is just an extension of the script in 588222 with some minor changes.

Comment 1

[Ayan Shah[:ashah]]

Attachment: Integration script

Comment 2

[Brian Warner [:warner :bwarner]]

sorry, is this a new script or a patch to an existing one? Should I be diffing this against something, or doing a review of the whole thing?

Comment 3

[Ayan Shah[:ashah]]

warner - its a patch to the existing one. I had to make some changes due to bug 594076. You can diff this against - 
https://github.com/mozilla/addon-sdk/commit/2d6e95c66c20a52fa6815ab251b123587625b218

Comment 4

[Brian Warner [:warner :bwarner]]

Comment on attachment 499139 [details]
Integration script

in the future, it'd be easier to work with a diff against the existing file, rather than a brand new copy, especially when the filename isn't obvious (the actual file in the tree is named bin/integration-scripts/integration-check, without the .py, so my 'find' command didn't find it). You might want to try grabbing a git checkout, make your changes, then use 'git diff' to generate the patch for attachment.

In the subprocess.Popen() call on line 265:

 p = subprocess.Popen('bin\\activate && cfx run --pkg examples\\reading-data --static-args="{\"quitWhenDone\":true}" -b \"" + self.bin + "\"' , stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)

Is that "--pkg" argument supposed to be "--pkgdir"? It might be better to use the full argument name, rather than an abbreviation, so that anyone reading the script can quickly figure out what the argument is supposed to do. Using just "--pkg" appears to work, but I think that's depending upon a surprising (to me at least) abbreviation-tolerant behavior in the argument-parsing code.

It'd also be easier to read if you delete the commented-out lines entirely, like the ones left on lines 264 and 269. I usually only leave in commented-out lines when they're retaining some important historical information, or demonstrating what the code ought to do in the future once some other feature or bugfix has been added. In this case, it's just kinda confusing :)

Overall it looks good: from what I can tell, the patch does this:
 - move '-p' argument from 'cfx -p DIR run' to 'cfx run -p DIR'
 - add --static-args to 'cfx run' calls
 - remove '-a firefox' from 'cfx run' calls
 - factor out no-spaces-in-addon-name checks
 - remove some unused imports
 - remove redundant p.communicate() call from common path after subprocess.Popen calls
 - fix directory-joining in result_example()
 - change failure detection: look for "FAIL" on a line by itself, instead of looking for "FAIL" in the middle of a line. Don't try to parse number-of-tests-passed.
  - question: why do the reversed() in line 330? It looks like that just increases memory usage without affecting the behavior. Same for 348.

If it looks like I've misinterpreted the changes, or missed something important, please let me know.

So let me ask you to make the following changes:
 - use "--pkgdir" instead of "--pkg"
 - remove commented-out lines of code
 - don't reversed() loglines, just use 'for line in open(log_path).readlines())'
 - add a newline at the end of the file
 - attach a diff, made with 'hg diff' or 'git diff'

With those changes, I think this can go in quickly!

Comment 5

[Ayan Shah[:ashah]]

Attachment: Changes according to warners review

Comment 6

[Ayan Shah[:ashah]]

Attachment: Patch to review

warner - please ignore the second attachment. Review the third one.

Comment 7

[Brian Warner [:warner :bwarner]]

Comment on attachment 505236 [details] [diff] [review]
Patch to review

Looks good. I'll r+ it, but there are two issues I'd like to see fixed sooner or later:

 - it still has the redundant reversed() calls. I suspect you want to change the condition inside the loop somehow.
 - I think the default --url value is wrong: I don't see a .zip file at https://ftp.mozilla.org/pub/mozilla.org/labs/jetpack/addon-sdk-latest.zip .

I've added this patch to a branch in my github repo: https://github.com/warner/addon-sdk/tree/ashah-620772-intscript . If you tell me that the --url value is correct, then I'll land the patch as soon as the tree reopens for 1.0b3 .

Comment 8

[Ayan Shah[:ashah]]

The --url value is correct. After my conversation with Myk, he removed the name "jetpack-sdk-latest.zip" and mentioned that writing jetpack-sdk-latest.zip will always point to the latest release. So even if you dont see the zip in there, if you copy-paste that link(https://ftp.mozilla.org/pub/mozilla.org/labs/jetpack/addon-sdk-latest.zip) in your browser, you can download the latest release.

you can land the patch now as soon as tree opens. Thanks warner.!

Comment 9

[Brian Warner [:warner :bwarner]]

landed, in https://github.com/mozilla/addon-sdk/commit/165412f29b8f988d37f32411c0a5e75e05bf7907

--url value works for me now, there's some clever redirect happening that I must have missed before.