588222 - Integration script for windows

Status: RESOLVED FIXED

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

Description

[Ayan Shah[:ashah]]

Attachment: Integration windows script

Attached here is the Jetpack integration script for windows platform.

Note: This script requires a library "win32api.pyd". I have used it to kill process in windows. But this file was not included by default in my python lib directory(Python 2.6).
So I downloaded the "pywin32-214.win32-py2.6.exe" from http://sourceforge.net/projects/pywin32/files/  and installed it. This will put a bunch of windows apis into the python's lib directory.

So anyone trying this script has to make sure the win32api.py is present in the library or install it from the link above. For the end uers, we can bundle the library with the script. If we are able to do that, this single script can be run on Mac as well as windows. We wont need two different scripts for it.

Comment 1

[Ayan Shah[:ashah]]

Attachment: Updated patch for windows script.

The updated patch contains some minor changes for windows as well as some changes from the comment #4 in bug 584479.

Comment 2

[Ayan Shah[:ashah]]

Waner,

Dont review the first attachment. Review the second one instead. It is the updated(and much better) version of the first one.

Comment 3

[Brian Warner [:warner :bwarner]]

Comment on attachment 472840 [details]
Updated patch for windows script.

I think the script will do the job as it stands, but if you'd like to clean
it up some more, here are some ideas:

general notes:
 - indentation is better, still a few places to make consistent: comments
   that precede methods should start on the same column as the "def' (lines
   86, 101, 178). Otherwise the reader will assume that the class has been
   ended. The lines in your __main__ clause (lines 374-378) should be at
   column 4, not column 8.
 - The globals defined at line 14 can go away: it looks like you only refer to
   "self.zip" and "self.gz"
 - look at the docs for os.path.join(), os.path.sep, and os.path.split() .
   You can probably use them to replace a lot of the following with cleaner
   and more robust versions:
     self.base_path = home + str(options.path).strip() + "\\"
     self.new_dir = home_path + folder_name
     log_path = home_path + 'tests.log'
     exlog_path = sdk_dir + '/test-example.log'
   Your code is exactly correct to split/join zipfile pathnames with "/"
   independent of the host OS's separator character, since those are always
   forward-slashes.

- It's a bit weird to invoke a method on an object and pass it one of its own
  attributes, as in "obj.download(obj.link, obj.fpath, obj.fname)". If you're
  thinking of making this file be available as a library that others can use
  in a different way, then it could make sense, but in general it's better to
  let the object find its own attributes (via self.link, self.fpath, etc).

There are an awful lot of switches to do things one way under windows and a
different way on other OSes. Your script will be more robust and easier to
maintain if it does the same thing on all platforms. Could you use e.g.
zipfile.extractall on all platforms, instead of shelling out to "unzip
"+extfile on non-windows? Same for tarfile.extractall().

Using string interpolation of self.bin into the "bin/activate" command means
that certain characters in self.bin will cause problems. I see that you've
put double quotes around the self.bin argument in the windows case, but I
don't see any code to guard against a double quote in self.bin or a space
(which will cause the command to be misinterpreted on non-windows). I'd
recommend an assert statement like you have for self.base_path, and make sure
that there are no spaces in it either. (guarding against a double-quote would
be nice, but if the code does that then it really ought to guard against all
shell metacharacters like '&' and ';' and '>', and that's not very convenient
to implement, so personally I might not bother).

The "You cannot have a space in your home path" assertion should be factored
out: use one assert statement after the two sets of "if" clauses around line
40, rather than four copies of the same assert statement (one in each
branch).

Your "re.search('zip',self.fname,re.I)" would be better expressed as "if
self.fname.lower().endswith('.zip')", because the regexp-based approach will
incorrectly match on URLs like http://example.com/~zippy/file.tar .

I'm not sure if the watchdog you're using in run_testall() will actually
work: from the subprocess.Popen() does, it looks like p.communicate() will
block until the process has terminated. Make sure to actually test this
(probably by replacing the 'cfx testall' with a 'sleep 10000' and seeing if
the watchdog fires). You might have to start the timer before the call to
p.communicate().

"re.search('FAIL',f.read())" at line 335 is more commonly expressed as "if
'FAIL' in f.read()". To do the same with re.I (that's case-insensitive,
right?), use "if 'ok' in line.lower()".


Thanks for putting up with all of my picky suggestions! :)

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Brian: can you land this change?

Comment 5

[Drew Willcoxon :adw]

I could be wrong, but I think this needs to be updated in light of bug 594076?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Circling back around to this...

Drew: can the changes to be made in light of bug 594076 happen in a separate bug?

Ayan: is it possible to check in the patch Brian reviewed in its current state, or are any additional changes needed at this point?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 8

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla/addon-sdk/commit/2d6e95c66c20a52fa6815ab251b123587625b218