Tracking bug for Jetpack automation integration script

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ashah, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Comment 1

8 years ago
Created attachment 462886 [details]
Integration script v2
(Reporter)

Comment 2

8 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

8 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.
Attachment #462886 - Attachment mime type: text/x-python-script → text/plain
Attachment #462886 - Flags: review?(warner-bugzilla)
(Reporter)

Updated

8 years ago
Depends on: 588222
No longer depends on: 578770
(Reporter)

Updated

8 years ago
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
(Reporter)

Updated

8 years ago
Depends on: 620772
(Reporter)

Updated

8 years ago
No longer depends on: 620772
(Reporter)

Updated

8 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [triage:followup]
You need to log in before you can comment on or make changes to this bug.