Merge lp://staging/~steve-mcintyre/linaro-chromiumos/python-rewrite into lp://staging/linaro-chromiumos
Proposed by
Steve McIntyre
| Status: | Merged |
|---|---|
| Merged at revision: | 14 |
| Proposed branch: | lp://staging/~steve-mcintyre/linaro-chromiumos/python-rewrite |
| Merge into: | lp://staging/linaro-chromiumos |
| Diff against target: |
350 lines (+278/-53) 4 files modified
remote/run_build (+0/-14) remote/run_build.py (+59/-0) remote/setup_build (+0/-39) remote/setup_build.py (+219/-0) |
| To merge this branch: | bzr merge lp://staging/~steve-mcintyre/linaro-chromiumos/python-rewrite |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Loïc Minier | Pending | ||
|
Review via email:
|
|||
Description of the change
Rewrite in python.
Adding more features for better control of what we're building and where.
To post a comment you must log in.
Revision history for this message
| Loïc Minier (lool) wrote | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
I submitted a branch based on yours for you to merge, as I felt it was simpler to show the changes in commits rather than explain them, then ask you to do them, then review them etc. These were all very simple changes, and overall your code was really good, thanks! See https:/
There is one major issue with the new Python code: usage of os.system() is unchecked, which means we don't fail the run when any command fails. This really must be fixed; with the new run() wrapper I introduced, it should be simpler to add this in a single place.
Other things I had in mind:
- we should have a place to share code between the scripts
- we should move them out of remote and kill my linaro-
- (low priority) getopt doesn't integrate too nicely with Python in my experience, there are nicer ways to handle flags, but it works with getopt so we can keep it for now
- create_logdir() feels a bit specific; I think we should have an ensure_dir() thin wrapper around os.isdir() + os.makedirs(), and we should setup logdir early; perhaps it's easier to just expose it as an option; this would also allow merging logged_run() with run()
- I think we should use stderr consistently for our own output, but I'm open to discuss this
- the run() interface should take an array of commands rather than a command string; this is nicer to deal with when preparing commands before running them