Status

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: aki, Assigned: aki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
While writing the MaemoMultiLocaleBuild class, I refactored quite a bit of mozharness.

This bug will track getting those changes back into the tree.
(Assignee)

Comment 1

8 years ago
Created attachment 506640 [details] [diff] [review]
full, single patch

This is a |hg diff -r a2a04b78ead5| from http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/file/780e984cc489 .  I will need a few changes on top to get maemo_multi_locale_build.py called from MaemoBuildFactory properly, but this is the meat of it.

Changelog:

http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness revision 780e984cc489

2011-01-24 Aki Sasaki <aki@mozilla.com>

* Move lib/ to mozharness/; change all 'import base.MODULE' statements to 'import mozharness.base.MODULE'

* ./*: Change method names to PEP 8 style (lowercase with words separated by underscores)
 
* TODO: deleted

* configs/: 4-space. Move hg repos to a list of dictionaries <-- may change later.  This makes it easy to add/remove/change repos in the config files without touching the scripts, but we lose the ability to specify revisions, repos, and directories in the command line (I want to fix this in 0.4+).

* configs/standalone_trunk_android.json: added

* configs/trunk_maemo_gtk.json: added

* mozharness/__init__.py: set version to 0.3

* mozharness/base/config.py (parse_config_file): add .py config file support

* mozharness/base/config.py (BaseConfig): fix all_actions, default_actions.  Remove automatic reading of localconfig.json

* mozharness/base/errors.py: update comments, add more Makefile errors.

* mozharness/base/log.py (BaseLogger.__del__): try to fix windows unit tests by calling logging.shutdown()

* mozharness/base/script.py (BaseScript.query_abs_dirs): add a default query_abs_dirs() to remove a lot of duplicate per-action code.  Also add the concept of an upload_dir.

* mozharness/base/script.py (BaseScript.run): add default run() method to a) allow for preflight- and postflight- methods to each action, and b) allow for a default workflow for all mozharness scripts.

* mozharness/base/script.py (BaseScript.dump_config): add a default dump_config() method.  Running into a clobber timing issue (logs + config inside upload_dir, which may be clobbered); will solve in 0.4+

* mozharness/base/script.py (BaseScript.mkdir_p): make slightly less verbose.

* mozharness/base/script.py (BaseScript.rmtree): fix for windows.

* mozharness/base/script.py (BaseScript.chmod): add

* mozharness/base/script.py (BaseScript.chown): add

* mozharness/base/script.py (BaseScript.query_env): add a generic way to build a working env from a partial env dictionary and os.environ, with magic strings that are replaced (currently only %(PATH)s )

* mozharness/base/script.py (BaseScript.get_output_from_command): deal with tempfile.NamedTemporaryFile differences between python 2.5 and 2.6

* mozharness/base/script.py (BaseScript): reorganize class methods by category.

* mozharness/base/script.py (MercurialMixin): remove hg_ from scm_* method arguments for potential future ease of changing source code systems

* mozharness/base/script.py (MercurialMixin.scm_checkout_repos): added to check out a list-of-dictionaries-of-hg-repos.  May change in the future.

* mozharness/base/script.py (MercurialScript.__init__): trying out super().__init__() rather than calling the __init__ methods separately.  I've waffled on this on various classes when I hit various bugs; not sure which way to go.

* mozharness/l10n/locales.py: pull query_locales(), parse_locales_file(), run_compare_locales() from the scripts and made generic.  override BaseScript.query_abs_dirs() to define the l10n directory paths required.

* mozharness/l10n/multi_locale_build.py: essentially scripts/multil10n.py, with the generic functions moved to locales.py (above), and MultiLocaleBuild made inheritable.  Add a _process_command() to help deal with scratchbox in MaemoMultiLocaleBuild.

* scripts/configtest.py: a new mozharness script that is a) small, b) self-contained, c) hopefully a good example for future simple mozharness scripts, and d) checks all config files in configs/ for well-formedness.  I'd like to expand on this later, to find well-formed-but-still-broken configs.

* scripts/maemo_multi_locale_build.py: inherit MultiLocaleBuild and add scratchbox-isms.  First examples of preflight_ACTION() methods in use.  Override _process_command() to massage the **kwargs to create a scratchbox command.

* scripts/multil10n.py: tear out most of this file; import MultiLocaleBuild instead.

* scripts/signdebs.py: modify to work with all the back-end changes, use LocalesMixin methods.

* test/test_log.py: some changes to try to get windows unit tests working (failed)

* test/test_script.py: stop downloading google.com; download mozilla.com instead (utf8 errors)

* test/* : modify to work with all the back-end changes.

* unit.sh: modify to work with the new directory layout. Try to suppress some of the known non-error messages.

Known Issues:
=============

* removed the ability to specify revision/repo/directory via commandline
* can't pass unit.sh on windows
* upload_dir clobber timing issue: logs and config are placed in upload_dir before we can clobber.
* incomplete multilocale scripts: no upload, no buildsymbols, no updates, no signing (currently handled by buildbot factories).

Wanted:
=======
* copy_to_upload_dir() with file/log rotation
* upload object/mixin
* config definitions, auditing
* revisit hg repos list-of-dictionaries
* no-op revisit
* logging revisited: per-action logs?
* more error checking
* more docs, more tests
I love the first few entries in the changelog, and from 15 minutes of browsing http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/file/780e984cc489, I like what I see there.
(Assignee)

Comment 3

8 years ago
Created attachment 512112 [details] [diff] [review]
additional changes to get maemo multilocale working in buildbot

This patch is the diff between the previous patch and mozharness revision 630a177f05ad.  The changes were to fix a couple bugs and a test, and get mozharness usable inside MaemoBuildFactory (bug 525327)
(Assignee)

Comment 4

8 years ago
Created attachment 512301 [details] [diff] [review]
full diff, take 2

Same as the previous full single patch, but with some bugfixes + tweaking to work inside of buildbot. (See attachment 512112 [details] [diff] [review] for the diff between the two patches).
Attachment #506640 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Hmm. Just thought of this:
I'm going to negatively affect Android release builds due to moving the compare-locales checkout out of the pull-locale-source step.

We should still be able to do code review, but I can't land until I deal with that.
(Assignee)

Comment 6

8 years ago
Created attachment 512921 [details] [diff] [review]
add compare-locales checkout to 0.7 factory.py

This, along with http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/725c6f781702, fix 0.7 Android release builds.
(Assignee)

Comment 7

8 years ago
error.py
 - regular expressions -> r'''REGEX'''

config.py
 - rename MozOptionParser?
 - search_path may mean it's not clear which config it's using.
 - log which config file
   config['orig_config_file_path'] = path
 - pprint the config
 - writing json: utf-8 will bork on windows. codex.open

log.py
 - sys.exit to exception ? config.py too

script.py
 - noop needs to be revisited
 - download_file needs same codex.open. needs to find mime type.
   - open as binary?
   - wget or curl would be preferable but not good to depend on command line.
 - exception in log() again
 - getOutputFromCommand should be able to just get output from subprocess.

bhearsum thinks he has enough to look at the rest of the scripts.

Let me know if you want another meeting about the rest of the code; I'd be happy to do that.  Anything that results in either an r+ or a specific list of fixes that block an r+ :)
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> error.py
>  - regular expressions -> r'''REGEX'''

http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/00c1fa703edf
(Assignee)

Comment 9

8 years ago
(In reply to comment #7)
> config.py
>  - search_path may mean it's not clear which config it's using.
>  - log which config file
>    config['orig_config_file_path'] = path

Hm, I'll get to this.

>  - rename MozOptionParser?
>  - pprint the config
>  - writing json: utf-8 will bork on windows. codex.open

http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/f684a8b67cb0

I seem to be going backwards in terms of test coverage.
I'll revisit.
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/665b46b7d9ad fixes the previous checkin's bustage + deals with some of the parse_config_file fixes.

http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/613e3e811410 changes sys.exit's to raise SystemExit(exit_code).  This is pretty much the same thing, but is an explicit raise that someone can try/except.

> - getOutputFromCommand should be able to just get output from subprocess.

Bear: I now remember. John Ford was correctly pointing out that we could potentially be getting output from a command that outputs >1gb of data on a memory-constrained machine.  In this case, it's better to write to a tempfile and return the filepath rather than try to suck all the output to memory.

However, to do that properly, we probably should be passing a filepath to get_output_from_command rather than depending on TempFile working properly.

I'm inclined to leave it as is for now; let me know if it needs fixing before I can get an r+.


I think I've dealt [at least mostly] with the items that came up in the review meeting; should I set r? or feedback? flags? Or is there more feedback coming?

Comment 11

8 years ago
(In reply to comment #10)
> http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/665b46b7d9ad
> fixes the previous checkin's bustage + deals with some of the parse_config_file
> fixes.
> 
> http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/rev/613e3e811410
> changes sys.exit's to raise SystemExit(exit_code).  This is pretty much the
> same thing, but is an explicit raise that someone can try/except.
> 
> > - getOutputFromCommand should be able to just get output from subprocess.
> 
> Bear: I now remember. John Ford was correctly pointing out that we could
> potentially be getting output from a command that outputs >1gb of data on a
> memory-constrained machine.  In this case, it's better to write to a tempfile
> and return the filepath rather than try to suck all the output to memory.
> 
> However, to do that properly, we probably should be passing a filepath to
> get_output_from_command rather than depending on TempFile working properly.

ah - that makes a lot of sense.  passing of a filepath should be an option, but the routine would need to have tempfile code no matter what.

> 
> I'm inclined to leave it as is for now; let me know if it needs fixing before I
> can get an r+.

r+ as is for now

> 
> I think I've dealt [at least mostly] with the items that came up in the review
> meeting; should I set r? or feedback? flags? Or is there more feedback coming?

for me it's an r+ with the changes you (will) have made
(Assignee)

Comment 12

8 years ago
Comment on attachment 512921 [details] [diff] [review]
add compare-locales checkout to 0.7 factory.py

We need this because I moved the compare-locales checkout to the pull-build-source action, which doesn't happen in the buildbot factory.

(Either that, or I need to move that back to the pull-locale-source action.)
Attachment #512921 - Flags: review?(bhearsum)
These comments are based on my reading of rev 613e3e811410 of your mozharness user repo.

General:
- There's a lot of duplication between this and the Python libraries in build/tools. That's not surprising given their parallel development, but we should think about how we want to deal with that in the future. This isn't an immediate blocker.

mozharness/base/script.py:
- BaseScript is pretty monolithic, and could do with some splitting up. A logical separation might be to follow Python libraries. Eg, OsMixin, ShutilMixin, et. al. This isn't a blocker, but it would be really nice to have from the get-go.

test/:
- Please split the monolithic tests into individual ones. Having so many asserts in one test (eg, test_actions in test_config.py) makes it hard to get an overview of all test failures. Wrapping common logic into a helper method is fine, but it's better to keep each assert in its own test method. http://hg.mozilla.org/users/bhearsum_mozilla.com/tools/file/a486dd5ecebe/lib/python/buildtools/test/test_build_versions.py is a good example of this.
- setUp() and tearDown() methods are called before and after each test, respectively. Use those instead of defining your own (eg, cleanup() and clean_log_dir())
Attachment #512921 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 14

8 years ago
(In reply to comment #13)
> General:
> - There's a lot of duplication between this and the Python libraries in
> build/tools. That's not surprising given their parallel development, but we
> should think about how we want to deal with that in the future. This isn't an
> immediate blocker.

Agreed.

tools/lib/python has a lot of more advanced and mature libs and scripts.  For instance, hgtool blows mozharness.base.script.MercurialMixin completely out of the water.

However, I think mozharness' configuration, logging, and log parsing are very strong. If others agree with me and want to keep those, it would make sense to port things like hgtool in as a new MercurialMixin.

> mozharness/base/script.py:
> - BaseScript is pretty monolithic, and could do with some splitting up. A
> logical separation might be to follow Python libraries. Eg, OsMixin,
> ShutilMixin, et. al. This isn't a blocker, but it would be really nice to have
> from the get-go.

Ok, I gave this a serious try and actually split out OSMixin, ShutilMixin, and TransferMixin (this only had download_file() as of now).

However, it turned out every single script needed OSMixin because BaseScript.dump_config() calls self.mkdir_p().  I thought about it, and I eventually want a copy_to_upload_dir() function that will allow for just that which would require access to copyfile().  Also, a default clobber() function would require the ShutilMixin.rmtree() function.

I do see the monolithic nature of BaseScript as a potential issue.

However, I want to strongly encourage, or even force people to use the logging mozharness functions instead of using the non-logging python functions.  Having those functions readily available helps that cause.  I see this as more important than whittling down the current size of BaseScript.

If I took all the functions that I wasn't going to use in copy_to_upload_dir() and clobber() I'd be left with chmod(), chown(), move(), and download_file() I think -- not a huge savings.

If we come across a type of script where we don't need any local filesystem access whatsoever, even for logging (network logging object in log.py?) I'd be much more open to moving those functions out.  As of now local filesystem access is assumed.

> test/:
> - Please split the monolithic tests into individual ones.

Done.

There is still a fairly large helper function tester since it makes sense to me -- I download a file, then verify its contents, then test moving, copying, removing that file.

Since the assertions have specific error messages attached, and the function has comments as to what it's doing now, I'm hoping that helps.

If it's still bad form to have those in the one function, I can download the file, cleanup, create a new file and verify its contents, clean up, create a new file and move it, clean up, etc.  It just seemed to make sense to do those tests in one.
(In reply to comment #14)
> (In reply to comment #13)
> > test/:
> > - Please split the monolithic tests into individual ones.
> 
> Done.
> 
> There is still a fairly large helper function tester since it makes sense to me
> -- I download a file, then verify its contents, then test moving, copying,
> removing that file.
>
> Since the assertions have specific error messages attached, and the function
> has comments as to what it's doing now, I'm hoping that helps.
> 
> If it's still bad form to have those in the one function, I can download the
> file, cleanup, create a new file and verify its contents, clean up, create a
> new file and move it, clean up, etc.  It just seemed to make sense to do those
> tests in one.

If you could, that would be great. When making changes that could break things that this code tests it can be really annoying that you don't get a summary of _all_ the failures right away -- you can quickly get into a situation where you fix one but break others, and not know it because earlier asserts are failing. Makes it much more annoying to debug.
Yup!
(Assignee)

Comment 18

8 years ago
Comment on attachment 512921 [details] [diff] [review]
add compare-locales checkout to 0.7 factory.py

http://hg.mozilla.org/build/buildbotcustom/rev/db8ec0bc51a7
Attachment #512921 - Flags: checked-in+
(Assignee)

Comment 19

8 years ago
I've landed my mozharness user repo changes to build/mozharness.
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: Other → Mozharness
You need to log in before you can comment on or make changes to this bug.