Closed Bug 584479 Opened 9 years ago Closed 9 years ago

Tracking bug for Jetpack automation integration script

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ashah, Unassigned)

References

Details

Attachments

(1 file)

No description provided.
Attached file Integration script v2
This updated script contains all changes from v1(except windows). Also the guidelines given by warner in bug 578770 are implemented. Ready for review.
This updated script contains all changes from v1(except windows). Also the guidelines given by warner in bug 578770 are implemented. Ready for review.
Attachment #462886 - Attachment mime type: text/x-python-script → text/plain
Attachment #462886 - Flags: review?(warner-bugzilla)
Depends on: 588222
No longer depends on: 578770
Depends on: 578770
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-
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
Depends on: 620772
No longer depends on: 620772
Depends on: 620772
Ayan: is this still relevant, or has it been superseded by later work?
Whiteboard: [triage:followup]
Closing, as it's a tracking bug whose dependencies have been resolved, but please reopen if that's incorrect!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [triage:followup]
You need to log in before you can comment on or make changes to this bug.