Closed
Bug 584479
Opened 14 years ago
Closed 13 years ago
Tracking bug for Jetpack automation integration script
Categories
(Add-on SDK Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ashah, Unassigned)
References
Details
Attachments
(1 file)
No description provided.
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
This updated script contains all changes from v1(except windows). Also the guidelines given by warner in bug 578770 are implemented. Ready for review.
Reporter | ||
Comment 3•14 years ago
|
||
This updated script contains all changes from v1(except windows). Also the guidelines given by warner in bug 578770 are implemented. Ready for review.
Updated•14 years ago
|
Attachment #462886 -
Attachment mime type: text/x-python-script → text/plain
Attachment #462886 -
Flags: review?(warner-bugzilla)
Reporter | ||
Updated•14 years ago
|
Comment 4•14 years ago
|
||
Comment on attachment 462886 [details]
Integration script v2
Looking better: there are still a lot of changes I'd recommend:
* use consistent indentation: 4 or 8 but not both. "def __init__" is indented
by 4, but the following "# Take the current working directory" is indented
8 more than that.
* use "class SDK", not "class sdk": the python convention is that classnames
are capitalized (and all-uppercase in the case of acronyms)
* run the "pyflakes" tool against the script: it spots the following bugs:
integration_v2.py:192: undefined name 'base_path'
integration_v2.py:239: undefined name 'base_path'
integration_v2.py:242: undefined name 'log_path'
integration_v2.py:275: undefined name 'base_path'
integration_v2.py:295: undefined name 'base_path'
integration_v2.py:322: undefined name 'base_path'
integration_v2.py:323: undefined name 'base_path'
* don't put semicolons at the end of lines
* many variables are used in inconsistent ways: sometimes as globals,
sometimes as attributes on the "SDK" object, sometimes as locals. Search
for places where you assign to e.g. "self.home" but never reference it on
self: these could be turned into locals. Similarly with "zipfilepath".
* have the subfunctions return values (like base_path) rather than storing
them in globals
* silencing the error at the end of the script is going to cause problems. I
recommend not using a try/except block there and just let the exception be
raised: it will print useful debugging info and the script will exit with a
non-zero exit code. It's important to signal an error to the caller,
especially if this is being run in a buildbot step, because otherwise the
buildbot will think the program has succeeded when it has in fact failed.
* don't do "sys.exit(0)" followed by "raise" (at line 260): the program will
exit with an apparent success code (use sys.exit(1) to signal an error),
the raise will never be executed, and the bare raise is inappropriate
outside of an except: block (the bare raise means "re-raise the exception
I'm currently handling"; you may want to use something like 'raise
RuntimeError("oops")' instead).
* the subprocess.Popen() calls use stdout=PIPE, yet include a ">" shell
redirect that means there will be no stdout. One of these two is
unnecessary.
* the use of shell interpolation for "log_path" will cause problems on
systems in which the home directory or base_path contains spaces (like a
lot of OS-X systems). This is non-trivial to deal with: you either need to
log to a fixed path (that doesn't use the value of base_path), or use
stdout=PIPE (and stderr=PIPE) and write the data to a logfile yourself, or
perhaps the simplest is to assert that base_path does not contain spaces at
soon as it is defined (the python statement is: 'assert " " not in
base_path')
Most of these are pretty minor, but the spaces-in-base_path, the pyflakes errors, and the silencing-errors ones are worth changing before committing this.
cheers,
-Brian
Attachment #462886 -
Flags: review?(warner-bugzilla) → review-
Comment 5•14 years ago
|
||
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Comment 6•13 years ago
|
||
Ayan: is this still relevant, or has it been superseded by later work?
Whiteboard: [triage:followup]
Comment 7•13 years ago
|
||
Closing, as it's a tracking bug whose dependencies have been resolved, but please reopen if that's incorrect!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [triage:followup]
You need to log in
before you can comment on or make changes to this bug.
Description
•