"Firefox l10n. I'd like to make these similar to the Fennec l10n solution, and help users create repacks locally.. " [1] We currently run L10n repacks in the mobile world with mozharness [2]. We should have the same solution for desktop L10n repacks. For releases we use split repacks and a script from the tools repo [3]: "bash scripts/scripts/l10n/release_repacks.sh macosx64 mozilla/production_config.py 6 1 generatePartials" For nightly and dependent repacks we use one single builder (NightlyRepackFactory from factory.py) [4] which contains many factory steps. This is the project I would like Jordan to start with. Since mozharness has such high priority I would like to leave it for later in his internship switching release repacks to use mozharness. [1] https://wiki.mozilla.org/ReleaseEngineering/Mozharness/28-Mar-2012#On_the_Horizon [2] http://hg.mozilla.org/build/mozharness/file/7f256b5c8730/scripts/mobile_l10n.py [3] http://hg.mozilla.org/build/tools/file/366b0228b576/scripts/l10n [4] http://hg.mozilla.org/build/buildbotcustom/file/default/process/factory.py#l3566

Possible feature creep that would be useful: repack with l10n-mozconfigs, as we already do in Fennec land.

"""Currently running:""" $ scripts/mobile_l10n.py --cfg single_locale/mozilla-central_android.py --total-chunks 5 --this-chunk 1 """and catching this fatal error in the logs:""" FATAL - Can't run command ['make'] in non-existent directory /home/jlund/mozilla/mozharness/build/mozilla-central/obj-l10n/config! FATAL - Exiting -1 scripts/mobile_l10n.py --cfg single_locale/mozilla-central_android.py 5 1 186.75s user 28.32s system 10% cpu 35:23.80 total #note: the dir 'obj-l10n' is never created I will keep investigating but if something immediately jumps out at someone, I would appretiate any input. Thx :)

Also, aki: Do you have any advice for someone who is trying to muster up some global understanding of Mozharness and how it works (at a real noob low level). Your blog posts are well documented but I would be really keen on putting all the pieces together in my head. :) whenever you have time.

(In reply to Jordan Lund (:jlund) from comment #3) > Also, aki: > > Do you have any advice for someone who is trying to muster up some global > understanding of Mozharness and how it works (at a real noob low level). > Your blog posts are well documented but I would be really keen on putting > all the pieces together in my head. :) whenever you have time. I'm not entirely sure what isn't clear to people, since it's intuitive to me. So I'm hoping that when I explain it to other people, they'll document or blog in a way that would help other people with the same confusion. I'm happy to help get you that understanding, though. What would work best? A walkthrough? Q&A about general overview? Q&A about specific problems & solutions?

(In reply to Aki Sasaki [:aki] from comment #5) > (In reply to Jordan Lund (:jlund) from comment #3) > > Also, aki: > > > > Do you have any advice for someone who is trying to muster up some global > > understanding of Mozharness and how it works (at a real noob low level). > > Your blog posts are well documented but I would be really keen on putting > > all the pieces together in my head. :) whenever you have time. > > I'm not entirely sure what isn't clear to people, since it's intuitive to me. > So I'm hoping that when I explain it to other people, they'll document or > blog in a way that would help other people with the same confusion. > > I'm happy to help get you that understanding, though. What would work best? > A walkthrough? Q&A about general overview? Q&A about specific problems & > solutions? for sure, I will blog when I find appropriate. Would you be ok with giving a general overview(as I do not have specific Q's yet). Maybe sometime next week so that I have some time to poke around more?

currently hitting this problem when mobile_l10n.py is trying to run this command: → ./tools/release/signing/verify-android-signature.sh --tools-dir=tools/ --nightly --apk=/home/jlund/mozilla/mozharness/build/mozilla-central/obj-l10n/dist/fennec-14.0a1.es-ES.android-arm.apk TOOLS DIR: tools/ Archive: /home/jlund/mozilla/mozharness/build/mozilla-central/obj-l10n/dist/fennec-14.0a1.es-ES.android-arm.apk caution: filename not matched: META-INF/NIGHTLY.DSA ERROR: Could not unzip /home/jlund/mozilla/mozharness/build/mozilla-central/obj-l10n/dist/fennec-14.0a1.es-ES.android-arm.apk Are you sure this is a nightly package?

This is the diff between configs/single_locale/mozilla-central_android.py and configs/single_locale/mozilla-central_desktop.py in revision https://github.com/lundjordan/mozharness/commit/78a2b0f2c9249b0efd353f7b0aca5fbd73b2fc62 of https://github.com/lundjordan/mozharness/tree/desktop-l10n .

This is the diff between mobile_l10n.py and desktop_l10n.py. This looks good! I imagine we have to deal with desktop signing and partial updates. However, this is close enough that I think we'd be able to deal with this in a single script.

Jordan: I'm taking this off your plate so you can concentrate on unit tests. It looks like you were able to learn from this and give us a good head start on getting desktop repacks in mozharness, thanks! I'm certain we'll have more work for you when you finish your unit test script.

If we don't fix win32-l10n-on-win64 before this lands, we should land that portion of this patch: https://bugzilla.mozilla.org/attachment.cgi?id=627535

(In reply to Aki Sasaki [:aki] from comment #9) > I imagine we have to deal with desktop signing and partial updates. > However, this is close enough that I think we'd be able to deal with this in > a single script. Axel also has requirements for tinderbox scraping that we will want to account for when porting this.

This has been on my "tackle soon" list for a while now, but I'm not actively working on it.

One random note: when doing Windows development/testing, it should be done against 64-bit Windows slaves. 32-bit ones are deprecated.

Hi Aki, This is what I have implemented for desktop repacks so far. I am currently working with mar files. As you can see this code is not complete but I'd like to have your opinion on this work in progress. About this patch: almost everything is in scripts/desktop_l10n.py except for some general purpose functions: helpers, and configuration files. about configuration: it's messy but i kept the same structure as the mobile repacks configuration for now. about helpers: I am not sure if mozarness/base/helpers is a good place but the idea is to move the generic functions away form the DesktopRepack class so they can be reused.

Removed from helpers some functions that are already present in ScriptMixin

Comment on attachment 798827 [details] [diff] [review] desktop-repacks.patch (In reply to Massimo Gervasini [:mgerva] from comment #16) > This is what I have implemented for desktop repacks so far. I am currently > working with mar files. As you can see this code is not complete but I'd > like to have your opinion on this work in progress. Ok. > about helpers: I am not sure if mozarness/base/helpers is a good place but > the idea is to move the generic functions away form the DesktopRepack class > so they can be reused. I'm not really sure about these, though it's good that you're thinking about moving generic functions out of the script. Anything Mozilla-specific should go under mozharness.mozilla instead of mozharness.base. Thanks for the patch! How's the mar work going, so far? >diff --git a/.hgignore b/.hgignore >index 738648e..db415cd 100644 >--- a/.hgignore >+++ b/.hgignore >@@ -13,3 +13,5 @@ htmlcov > venv > build/ > logs/ >+l10n/ >+properties/ This works... I tend to avoid running inside the mozharness directory so it's easier to keep clean. >diff --git a/configs/single_locale/mozilla-central_desktop.py b/configs/single_locale/mozilla-central_desktop.py I think mozilla-central_desktop.py is out of date, and you're using mozilla-central_PLATFORM-repacks.py instead? >+config = { >+ "mozilla_dir": MOZILLA_DIR, >+ "app_name": "browser", >+ "brand_name": "Minefield", I don't think you use app_name or brand_name anywhere? >+ "snippet_base_url": "http://example.com", TODO ? >+ "repos": [{ >+ "vcs": "hgtool", >+ "repo": "http://hg.mozilla.org/mozilla-central", >+ "revision": "default", >+ "dest": MOZILLA_DIR, >+ }, { >+ "vcs": "hgtool", >+ "repo": "http://hg.mozilla.org/build/buildbot-configs", >+ "revision": "default", >+ "dest": "buildbot-configs" I think I needed buildbot-configs for: * mozconfig (now in-tree) * release config (releases only, not needed for nightly) * and the locales_file (releases only, in-tree for nightly). We may not need to clone it for desktop nightly/depend repacks. >+ "l10n_dir": "../l10n", Looks like you have "l10n_dir": "l10n" for linux, linux64, and win32, and "l10n_dir": "../l10n" for osx. Is that intentional? If not, I think I'd prefer them the same. If so, maybe a comment as to why it's different? >diff --git a/mozharness/helpers/filesystem.py b/mozharness/helpers/filesystem.py >+def find_file(root_dir, filename): >+ """ find <root_dir> -type f -name <filename> >+ returns a single file, >+ """ >+ for dirpath, dirnames, filenames in os.walk(root_dir): >+ for f in filenames: >+ if f == filename: >+ return os.path.join(dirpath, f) I know we use this approach in the buildbotcustom code. I wonder if we really need to find it... If it's in a known location per platform, I'd like it in the config file rather than having to traverse the filesystem for it. If it's not in a known location, I'd be curious why it moves. I'm not going to block on that, though. >+def find_directory(root_dir, dirname): >+ """ find <root_dir> -type d -name <dirname> >+ returns the first occurence >+ """ >+ for dirpath, dirnames, filenames in os.walk(root_dir): >+ for d in dirnames: >+ if d == dirname: >+ return os.path.join(dirpath, d) Looks like this is unused? >diff --git a/mozharness/helpers/html_parse.py b/mozharness/helpers/html_parse.py <snip> >+def _extract_from_html(filename, left_delimiter, right_delimiter, ignoreHTTPErrors=[]): >+ """ a very simple html parser. It reads a file and >+ parses the page line by line, returning a list of elements >+ between left and right delimiters >+ """ >+ elements = [] >+ with open(filename, 'r') as f: >+ for line in f.read().split('\n'): >+ element = line.partition(left_delimiter)[2] >+ element = element.partition('/"')[0] >+ element = element.strip() >+ elements.append(element) >+ return elements This isn't really a generic html parser. This seems to be a Mozilla-specific, file-specific method. It looks like you use this to find the latest version(s) and build number(s) for a release? And you don't need that for a nightly/depend repack, correct? I'd much rather you got this information from buildbot-configs: http://hg.mozilla.org/build/buildbot-configs/file/5db20ab3d56b/mozilla/release-firefox-mozilla-beta.py releaseConfig['version'] and releaseConfig['buildNumber'] have what you need. I set a release_config_file here: http://hg.mozilla.org/build/mozharness/file/0d438d3dc9f6/configs/single_locale/release_mozilla-beta_android.py#l46 and get the release config via this method: http://hg.mozilla.org/build/mozharness/file/a07a3256cfd2/mozharness/mozilla/release.py#l21 You may have to add the partial update information in there as well. If these methods stick around, I have a slight preference of putting it in mozharness.mozilla.* so we have access to logging etc, but it's not a requirement. I do think we might be able to get rid of these methods with the release config file. >diff --git a/scripts/desktop_l10n.py b/scripts/desktop_l10n.py <snip> >+"""desktop_l10n.py >+ >+Firefox repacks >+""" I think we're hoping it'll work for Thunderbird as well, without too many changes, but that's out of scope! >+ def __init__(self, require_config_file=True): >+ LocalesMixin.__init__(self) >+ BaseScript.__init__( >+ self, >+ config_options=self.config_options, >+ all_actions=[ >+ "clobber", >+ "pull", >+ "list-locales", >+ "setup", >+ "generate_partials", >+ "repack", Hm, we generate partials before repacking? >+ def _query_make_ident_output(self): >+ """Get |make ident| output from the objdir. >+ Only valid after setup is run. >+ """ >+ if self.make_ident_output: >+ return self.make_ident_output >+ env = self.query_repack_env() >+ dirs = self.query_abs_dirs() >+ make = self.query_exe("make", return_type="list") >+ output = self.get_output_from_command(make + ["ident"], >+ cwd=dirs['abs_locales_dir'], >+ env=env, >+ silent=True, >+ halt_on_failure=True) >+ parser = OutputParser(config=self.config, log_obj=self.log_obj, >+ error_list=MakefileErrorList) >+ parser.add_lines(output) >+ self.make_ident_output = output >+ return output This seems like the same as mobile_l10n.py's method ? (as well as query_{buildid,revision}, ...) That's fine while you're working things out. When we land I'd like this to be in a shared method in mozharness.mozilla.*. I'm ok with having the mobile repacks also exclude the Pymake lines, since that'll be a noop. >+ def _touch_file(self, file_name, times=None): >+ """touch a file; If times is None, then the file's access and modified >+ times are set to the current time >+ """ >+ os.utime(file_name, times) Could you add a self.info("Touching %s" % file_name), possibly with the time if set? I don't like having things change silently... someone should be able to reproduce the whole workflow looking at the log. This method could go in ScriptMixin... >+ def create_nightly_snippets(self): >+ c = self.config >+ dirs = self.query_abs_dirs() >+ locales = self.query_locales() >+ base_package_name = self.query_base_package_name() >+ buildid = self.query_buildid() >+ version = self.query_version() >+ binary_dir = os.path.join(dirs['abs_objdir'], 'dist') >+ success_count = total_count = 0 >+ replace_dict = { >+ 'buildid': buildid, >+ 'build_target': c['build_target'], >+ } >+ for locale in locales: >+ total_count += 1 >+ replace_dict['locale'] = locale >+ aus_base_dir = c['aus_base_dir'] % replace_dict >+ aus_abs_dir = os.path.join(dirs['abs_work_dir'], 'update', >+ aus_base_dir) >+ binary_path = os.path.join(binary_dir, >+ base_package_name % {'locale': locale}) >+ # for win repacks >+ binary_path = binary_path.replace(os.sep, "/") >+ print "==========================" >+ print "binary_dir: >{0}<".format(binary_dir) >+ print "base_package_name: >{0}<".format(base_package_name) >+ print "base_package_name (2): >{0}<".format(base_package_name % {'locale': locale}) >+ print "binary_path: >{0}<".format(binary_path) >+ print "==========================" I'm guessing these prints are for debugging? >+ url = self.query_upload_url(locale) >+ if not url: >+ self.add_failure(locale, "Can't create a snippet for %s without an upload url." % locale) >+ continue >+ if not self.create_complete_snippet(binary_path, version, buildid, url, aus_abs_dir): >+ self.add_failure(locale, message="Errors creating snippet for %s! Removing snippet directory." % locale) >+ self.rmtree(aus_abs_dir) >+ continue >+ self.run_command(["touch", os.path.join(aus_abs_dir, "partial.txt")]) >+ success_count += 1 >+ self.summarize_success_count(success_count, total_count, >+ message="Created %d of %d snippets successfully.") So for mobile, we didn't create any complete MARs because Android updates use the installer (apk) for updates. For desktop we need a complete mar (and usually a partial mar) as well as the snippets. Looks like we have more work to do here. >+ def upload_nightly_snippets(self): >+ c = self.config >+ dirs = self.query_abs_dirs() >+ update_dir = os.path.join(dirs['abs_work_dir'], 'update') >+ if not os.path.exists(update_dir): >+ self.error("No such directory %s! Skipping..." % update_dir) >+ return >+ if self.rsync_upload_directory(update_dir, c['aus_ssh_key'], >+ c['aus_user'], c['aus_server'], >+ c['aus_upload_base_dir']): >+ self.return_code += 1 You may need to make this release-friendly, or add an upload_release_snippets(). >+ def generate_partials(self): >+ self.get_mar_tools() >+ self.get_previous_mar() >+ self.unpack_previous_mar() >+ p_build_id = self.get_buildid_form_ini(self.get_previous_application_ini_file()) >+ print p_build_id Rather than print, maybe self.info() or self.set_buildbot_property() ? Guessing this method is unfinished. >+ def local_mar_dir(self): >+ dirs = self.query_abs_dirs() >+ return os.path.join(dirs['abs_objdir'], 'dist', 'update') This could go into query_abs_dirs(). Sorry it's a pain to override, I'm considering reworking it in bug 898223. It could also possibly be a self.local_mar_dir, which would avoid re-os.path.join()ing every time this is called. >+ def local_mar_filename(self): >+ return os.path.join(self.local_mar_dir(), 'previous.mar') Same, this could be self.local_mar_filename. Not a blocker though. >+ def query_latest_version(self): >+ """ find latest available version from CANDIDATES_URL """ >+ c = self.config >+ update_env = self.query_env(partial_env=c.get("update_env")) >+ url = update_env['CANDIDATES_URL'] >+ temp_out = tempfile.NamedTemporaryFile(delete=False) >+ self.download_file(url, temp_out.name) >+ version = html_parse.get_last_version_number(temp_out.name) >+ os.remove(temp_out.name) self.rmtree() ? >+ return version >+ >+ def query_buildnumber(self, url): >+ temp_out = tempfile.NamedTemporaryFile(delete=False) >+ self.download_file(url, temp_out.name) >+ buildnum = html_parse.get_latest_build_number(temp_out.name) >+ os.remove(temp_out.name) >+ return buildnum As above, I think you can get these from the release config. I'm not sure if these are needed for nightly updates? >+ def previous_mar_url(self): >+ c = self.config >+ update_env = self.query_env(partial_env=c.get("update_env")) >+ #TODO nightly is hardcoded here... fix it!! >+ base_url = update_env['EN_US_BINARY_URL'] >+ platform = update_env['MOZ_PKG_PLATFORM'] >+ version = self.query_version() >+ remote_filename = "".join(("firefox-", version, ".en-US.", platform, ".complete.mar")) >+ return "/".join((base_url, remote_filename)) I think it would be best to unhardcode the remote_filename. At least have it config-overrideable, like remote_filename = c.get("complete_mar_filename", "".join("firefox-", ...)) % {'version': version, 'platform': platform} or something ? >+ from ConfigParser import SafeConfigParser Any reason you're not doing this import at the top? >+ def get_buildid_form_ini(self, ini_file): >+ return self.get_value_from_ini(ini_file, 'App', 'BuildID') s,form,from, ?

A way to plug-in mozharness based desktop repacks. * added enable_misc_py_short_circuit_hack to be annoying :) * use makeMHFactory everywhere * still have a couple of TODOs

Comment on attachment 802339 [details] [diff] [review] buildbotcustom Two things: * enable_misc_py_short_circuit_hack is an ugly name, but maybe that'll encourage us to port everything so we can get rid of it? :) * you're enabling mozharness dep l10n, which I think is good in theory, but has never been tested. I'm ok with this on Cedar, but am a bit worried that it's getting enabled on m-c and m-a as well (for Android). Then again, l10n isn't reported on tbpl, so maybe no one will notice... could be a waste of machine time if it's busted, though.

Hi all, I am still working on this bug: here is the current status: * updated code with Aki's suggestions (Thanks for the feedback!!) * generating complete mar under osx took me a lot of time because some issues with make and environment variables - now fixed * partial generation works on osx next steps: * more testing on linux and windows * code needs a clean up

updated configs, carry over f+aki

refresh, f+aki

syntax error fix, f+aki carry over

Hi Aki, This a very long patch and it's still a work in progress, as you can see it's not 100% complete. I've try to address all the issues you have highlighted on the previous patch and I have created a new class, Make that wraps all the make calls and a new module in mozharness/base/mar.py that manages the operations on mar files. I am also trying to move a part of the configuration into the mar.py file since it is common to all platform/branches. As it is now, this patch generates the partial mars files from the latest version of firefox (hardcoded to version 27, at the moment). I had to introduce a hack to set the --with-l10n-base to the correct value otherwise it does not work in buildbot (tested only for OSX). I don't know what it's the best way to address this problem, probably updating the mozconfig from the tree (but I can be totally wrong here). Next steps are: * test on linux and windows * remove the hardcoded values * complete the code for uploads * clean up configuration files

I haven't gotten to this yet, but it's at the top of the list for tomorrow. If you have any changes to make to the review request, now's a great time; otherwise, hopefully I'll get through this by ~EOD.

Comment on attachment 823475 [details] [diff] [review] work in progess on destktop repacks Phew! This is a lot of work, I can tell. Is there anything you're unsure of, in terms of what you should be doing? Pylint says: mozharness/base/mar.py:59: [E, MarTool.__init__] Bad first argument 'ScriptMixin' given to super class mozharness/base/mar.py:86: [E, MarFile.__init__] Bad first argument 'ScriptMixin' given to super class >diff --git a/mozharness/base/mar.py b/mozharness/base/mar.py >+"""desktop_l10n.py >+ >+Firefox repacks >+""" Docstring needs updating :) >+# load modules from parent dir >+sys.path.insert(1, os.path.dirname(sys.path[0])) You probably don't need this line, but if you do it'll probably be os.path.dirname(os.path.dirname(sys.path[0])) >+MAR_BINARIES = ('mar', 'mbsdiff') >+ >+CONFIG = { >+ "buildid_section": 'App', >+ "buildid_option": "BuildID", >+ "unpack_script": "unwrap_full_update.pl", >+ "incremental_update_script": "make_incremental_update.sh", >+ "update_packaging_dir": "tools/update-packaging", >+ "mar_tools_url": "https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/mar-tools/%(platform)s", >+ "complete_mar": "firefox-%(version)s.en-US.%(platform)s.complete.mar", >+ "localized_mar": "firefox-%(version)s.%(locale)s.%(platform)s.complete.mar", >+ "generated_mar": "%(platform)s/%(locale)s/firefox-%(version)s.complete.mar", >+ "partial_mar": "firefox-%(version)s.%(locale)s.partial.%(from_buildid)s-%(to_buildid)s.mar", >+} I would really rather that the above be overrideable via the mozharness config, but I'm not sure that should block. >+def tools_environment(base_dir): >+ """returns the env setting required to run mar and/or mbsdiff""" >+ env = {} >+ for binary in MAR_BINARIES: >+ env[binary.upper()] = os.path.join(base_dir, binary) >+ return env >+ >+ >+def buildid_form_ini(ini_file): >+ """reads an ini_file and returns the buildid""" >+ ini = ConfigParser.SafeConfigParser() >+ ini.read(ini_file) >+ return ini.get(CONFIG.get('buildid_section'), >+ CONFIG.get('buildid_option')) If we wanted to be able to override the buildid_* or the MAR_BINARIES, we could specify additional kwargs in these two methods. >+class MarTool(ScriptMixin, LogMixin, object): I don't think you need to specify object, since both Mixins inherit object... (I think MercurialVCS has this extraneous object too) >+ """manages the mar tools executables""" >+ def __init__(self, url, dst_dir, log_obj): >+ self.url = url >+ self.dst_dir = dst_dir >+ self.binaries = ('mar', 'mbsdiff') >+ self.log_obj = log_obj >+ self.config = CONFIG >+ super(ScriptMixin, self).__init__() This should probably be super(MarTool, ... You're setting log_obj from the script, but not self.config... any reason? >+# MarFile {{{1 >+class MarFile(ScriptMixin, LogMixin, object): >+ """manages the downlad/unpack and incremental updates of mar files""" >+ def __init__(self, mar_scripts, log_obj, filename=None): >+ self.filename = filename >+ super(ScriptMixin, self).__init__() >+ self.log_obj = log_obj >+ self.build_id = None >+ self.mar_scripts = mar_scripts >+ self.config = CONFIG Same comments as MarTool. >+ def unpack_mar(self, dst_dir): >+ """unpacks a mar file into dst_dir""" >+ self.download() >+ # downloading mar tools >+ cmd = ['perl', self._unpack_script(), self.filename] >+ mar_scripts = self.mar_scripts >+ tools_dir = mar_scripts.tools_dir >+ env = tools_environment(tools_dir) >+ env["MOZ_PKG_PRETTYNAMES"] = "1" Hm, I'd like this configurable as well. If it's working, we can make these things more overrideable/configurable later, but we may want to be able to not set this in some circumstances. >diff --git a/mozharness/base/script.py b/mozharness/base/script.py >+ def _touch_file(self, file_name, times=None): >+ """touch a file; If times is None, then the file's access and modified >+ times are set to the current time >+ """ >+ self.info("Touching: %s" % file_name) >+ try: >+ os.utime(file_name, times) >+ except OSError: >+ try: >+ open(file_name, 'w').close() >+ except IOError as e: >+ self.fatal("I/O error({0}): {1}".format(e.errno, e.strerror)) >+ >+ os.utime(file_name, times) >+ self.info("Touching %s" % file_name) I think the one self.info() at the beginning is fine; we can remove this second one. >diff --git a/scripts/desktop_l10n.py b/scripts/desktop_l10n.py >+ # STUPID HACK HERE >+ # should we update the mozconfig so it has the right value? >+ with open(src, 'r') as in_mozconfig: >+ with open(dst, 'w') as out_mozconfig: >+ for line in in_mozconfig: >+ if 'with-l10n-base' in line: >+ line = 'ac_add_options --with-l10n-base=../../l10n\n' >+ out_mozconfig.write(line) I think we should fix the mozconfig, but that might need to happen after this is live. >+ def delete_pgc_files(self): >+ """deletes pgc files""" >+ for directory in (self.previous_mar_dir(), >+ self.current_mar_dir()): >+ for pcg_file in self.pgc_files(directory): >+ self.info("removing %s" % pcg_file) >+ #os.remove(f) Typo? pgc_file, and self.rmtree() >+ def current_mar_filename(self): >+ """retruns the full path to complete.mar""" >+ c = self.config >+ version = self.query_version() >+ update_env = self.query_env(partial_env=c.get("update_env")) >+ platform = update_env['MOZ_PKG_PLATFORM'] >+ version = self.query_version() >+ filename = c["complete_mar"] % {'version': version, >+ 'platform': platform} >+ return os.path.join(self._abs_dist_dir(), filename) >+ >+ def query_latest_version(self): >+ """find latest available version from candidates_base_url""" >+ if self.version: >+ return self.version >+ c = self.config >+ url = c.get('candidates_base_url') >+ temp_dir = tempfile.mkdtemp() >+ temp_out = os.path.join(temp_dir, 'versions') >+ self.download_file(url, temp_out) >+ self.version = "27.0a1" # hardcoded... too bad Haha. Do you need a way to figure out the version? >+ def previous_mar_url(self): >+ """returns the url for previous mar""" >+ c = self.config >+ update_env = self.query_env(partial_env=c.get("update_env")) >+ # why from env? >+ base_url = update_env['EN_US_BINARY_URL'] >+ platform = update_env['MOZ_PKG_PLATFORM'] I'm not entirely sure if this needs to be in env, or if this is just how we set the buildbot factory variables. This is fine, though. >+# main {{{ >+if __name__ == '__main__': >+ single_locale = DesktopSingleLocale() >+ single_locale.run() This should be run_and_exit() now.

Hi Aki, here is another patch! This one adds windows compatibility: repacks for windows are working in our staging environment. It also addresses most the problem you have find in the previous version. next steps: * clean up configuration * extend the script so it can use Balrog

(In reply to Massimo Gervasini [:mgerva] from comment #29) > Created attachment 8341856 [details] [diff] [review] > work in progress on desktop repacks > > Hi Aki, > > here is another patch! > This one adds windows compatibility: repacks for windows are working in our > staging environment. It also addresses most the problem you have find in the > previous version. > > next steps: > * clean up configuration > * extend the script so it can use Balrog Awesome. * I'll try to finish this by sometime Thursday * Should we think about landing a mostly-finished patch at some point, if it doesn't break anything currently running? That way future feedback/review loops would be incremental.

I noticed one thing during a quick read: build_target": "Linux_x86-gcc3" is set the same in each of the platform configs. That looks like it should be changing, like update_platform in PLATFORM_VARS (buildbot-configs/mozilla/config.py).

Comment on attachment 8341856 [details] [diff] [review] work in progress on desktop repacks Thanks Massimo! And yes, can we start landing some of this? Splitting the patch between new files and altering existing files would be a start. >diff --git a/configs/single_locale/mozilla-central_desktop.py b/configs/single_locale/mozilla-central_desktop.py Ignoring this one, I think. >diff --git a/configs/single_locale/mozilla-central_linux.py b/configs/single_locale/mozilla-central_linux.py >+PLATFORM = "linux64" Should this be 'linux'? >+ # AUS >+ "build_target": "Linux_x86-gcc3", As Nick says :) This is fine for linux. >+ 'freetype-2.3.11-6.el6_1.8.x86_64', >+ 'freetype-devel-2.3.11-6.el6_1.8.x86_64', ], Hm, is this 64 bit or mixed? I bet at least some of this stuff falls under your "clean up configuration" todo. >diff --git a/mozharness/base/mar.py b/mozharness/base/mar.py I see some of my previous comments aren't addressed yet here, but they look to be mostly the less important ones. >+ def buildid(self): <snip> >+ for dirpath, dirnames, filenames in os.walk(temp_dir): >+ for f in filenames: >+ if f == 'application.ini': >+ self.log("found application.ini file: %s" % os.path.join(dirpath, f)) >+ with open(os.path.join(dirpath, f), 'r') as ini: >+ self.log(ini.read()) Since this inherits ScriptMixin, you can use the new self.opened() method, which avoids leaking filehandles: http://hg.mozilla.org/build/mozharness/file/2b787e4d30f6/mozharness/base/script.py#l374 >diff --git a/mozharness/base/script.py b/mozharness/base/script.py <snip> >+ def _touch_file(self, file_name, times=None): >+ """touch a file; If times is None, then the file's access and modified >+ times are set to the current time >+ """ >+ self.info("Touching: %s" % file_name) >+ try: >+ os.utime(file_name, times) >+ except OSError: >+ try: >+ open(file_name, 'w').close() >+ except IOError as e: >+ self.fatal("I/O error({0}): {1}".format(e.errno, e.strerror)) If you want to allow for this to be non-fatal, you could do def _touch_file(self, file_name, times=None, error_level=FATAL): ... self.log("I/O error...", level=error_level) return [some error value] and then error_level will be overrideable. >diff --git a/scripts/desktop_l10n.py b/scripts/desktop_l10n.py <snip> >+class DesktopSingleLocale(LocalesMixin, ReleaseMixin, MobileSigningMixin, <snip> >+ def query_repack_env(self): <snip> >+ if 'MOZ_SIGNING_SERVERS' in os.environ: >+ sign_cmd = self.query_moz_sign_cmd(formats=None) >+ sign_cmd = subprocess.list2cmdline(sign_cmd) >+ # windows fix >+ repack_env['MOZ_SIGN_CMD'] = sign_cmd.replace('\\', '\\\\\\\\') Haha :) >+ def query_base_package_name(self, locale, prettynames=True): >+ """Gets the package name from the objdir. >+ Only valid after setup is run. >+ """ >+ # optimization: >+ # replace locale with %(locale)s >+ # and store its values. >+ args = ['AB_CD=%s' % locale] >+ return self._query_make_variable("PACKAGE", make_args=args) Hm, you don't seem to use prettynames here? Also, it's defaulting to True as opposed to 0/1 elsewhere. >+ msg = "You either need to run --upload-repacks before " >+ msg += "--create-nightly-snippets, or specify " >+ msg += "the 'snippet_base_url' in self.config!" >+ self.error(msg) You can do a multiline string with """, but this is fine. msg = """line one line two line three""" >+ def add_failure(self, locale, message, **kwargs): >+ self.locales_property[locale] = "Failed" >+ prop_key = "%s_failure" % locale >+ prop_value = self.query_buildbot_property(prop_key) >+ if prop_value: >+ prop_value = "%s %s" % (prop_value, message) >+ else: >+ prop_value = message >+ self.set_buildbot_property(prop_key, prop_value, write_to_file=True) >+ BaseScript.add_failure(self, locale, message=message, **kwargs) Hm, maybe super()? Is there a reason you're calling BaseScript.add_failure() directly here? >+ # STUPID HACK HERE >+ # should we update the mozconfig so it has the right value? >+ with open(src, 'r') as in_mozconfig: >+ with open(dst, 'w') as out_mozconfig: >+ for line in in_mozconfig: >+ if 'with-l10n-base' in line: >+ line = 'ac_add_options --with-l10n-base=../../l10n\n' >+ self.l10n_dir = line.partition('=')[2].strip() >+ out_mozconfig.write(line) >+ # now log >+ with open(dst, 'r') as mozconfig: >+ for line in mozconfig: >+ self.info(line.strip()) We may be able to use self.opened() here as well. And yes, when we get all migrated to mozharness l10n, this looks like a good change to make in the in-tree mozconfigs.

Thanks Aki and Nick, I am ready to commit the previous patch as it does not affect anything already existing in mozharenss except for the changes highlighted this patch. If you are happy with the changes, I will commit the attachment 8341865 [details] (work in progress on desktop repacks) so I can start fixing all the open issues. thanks again!

Hi Aki, 99% of this patch is because pylint and configuration clean up. There is only one small fix introduced: now, in case of error during create_nightly_snippets(), summarize_success_count() gets the right message string. I hope next patch will be smaller.

Massimo: no update since last year. How's progress?

Hey coop, I am planning to submit soon another patch to address some problems found by aki and some minor fixes for the windows rev 2 machines.

Hi Aki, Work in progress on desktop repacks * removed a lot of obsolete code * different way to report success/failures. * balrog submitter for repacks (not tested) * added dev configuration files (config/single-locales/dev-*) TODO: * clean up production config * test/finish balrog submitter * ... the plan here is to enable desktop repacks soon on cedar.

Hi rail, I've updated your patch because it does not apply anymore :(

... and this is the buildbot-config one. Could this work? (I know that there are still some values pointing to my dev-master01 configuration - e.g. my mozharness repo)

Quick update: I have setup a staging environment on [0] with mozharness desktop repacks enabled on cedar. Enabled platforms are win32, linux, linux64, macosx64. When all the issues on devmaster-01 are sorted out, I'd like to have extend the same to a real production environment. Current status: * linux64, macosx64 desktop repacks fail because of the barlog submitter configuration is still pointing to local my local balrog (... 127.0.0.1) and the code is still incomplete. I'm working on this in the next days. * win32 has troubles running the desktop_l10n.py script: argv: ['scripts/scripts/desktop_l10n.py', '--config', 'single_locale/win32.py', '--total-chunks', '10', '--this-chunk', '1'] 'scripts' is not recognized as an internal or external command How can I invoke "python.exe scripts/scripts/..." instead? * linux has issues with mock: sh: /builds/slave/ced-lx-l10n_1-0000000000000000/build/mozilla-central/obj-l10n/dist/host/bin/mar: /lib/ld-linux.so.2: bad ELF interpreter: No such file or directory This happens when I download mar/mbsdiff from [1]. Is my local mock setup is broken or should I change the configuration to point to 64 bits mar-tools? While working on this issue, I have noticed that there is a x64 package in linux 32 mock section: freetype-2.3.11-6.el6_1.8.x86_64 (mozilla/config.py). Is this expected? ---- [0] http://dev-master1.srv.releng.scl3.mozilla.com:8914 (vpn required) [1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/mar-tools/linux/ [2] /builds/slave/ced-lx-l10n_1-0000000000000000/build/mozilla-central/obj-l10n/dist/l10n-stage/firefox.work/output.mar

Comment on attachment 8444950 [details] [diff] [review] [buildbot-configs] updated config for desktop repacks.patch Review of attachment 8444950 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/config.py @@ +132,5 @@ > + # list platforms with mozharness l10n repacks enabled. > + # mozharness repacks will be enabled per branch > + 'mozharness_desktop_l10n_platforms': [ > + 'linux', 'linux64', 'win32', 'win64', 'macosx64' > + ], instead of listing platforms in the global name space, I'd rather move this to the platform level. @@ +2360,5 @@ > + 'mozharness_desktop_l10n_platforms',): > + try: > + del branch[token] > + except KeyError: > + pass I'm not sure if I understand this... You are deleting some keys from all branches? We usually use the following pattern: for name, branch in BRANCHES.items(): if name in mozharness_desktop_repacks_branches: # do not delete anything set at platform level continue # for all other branches delete the keys you want for p in branch["platforms"]: if "mozharness_desktop_l10n" in p: del p["mozharness_desktop_l10n"] ....

Comment on attachment 8444948 [details] [diff] [review] [buildbotcustom] adding mozharness desktop repacks.patch Review of attachment 8444948 [details] [diff] [review]: ----------------------------------------------------------------- In overall it looks good. But to make this "landable" to production you also need to add code to disable regular repacks if mozharness repacks are enabled. We don't want to run them in parallel, race each other and consume resources. ::: misc.py @@ +1534,5 @@ > pf = config['platforms'][platform] > + stage_platform = pf.get('stage_platform', platform) > + > + if pf.get('desktop_mozharness_repacks_enabled'): > + print "l10n enabled: {0}, {1}".format(platform, name) I'd rather use log.msg() instead of print.

Comment on attachment 8444946 [details] [diff] [review] [mozharness] desktop repacks.patch My main concern here is the BalrogMixin change. >diff --git a/mozharness/base/mar.py b/mozharness/base/mar.py Not sure if I've mentioned this. MAR files are Mozilla-specific, so we probably want this in mozharness/mozilla/. >+ # log the content of application.ini >+ with self.opened(ini_file, 'r') as (ini, error): >+ if error: >+ self.fatal('cannot open {0}'.format(ini_file)) >+ self.debug(ini.read()) >+ # delete temp_dir > self.build_id = buildid_form_ini(ini_file) I think you mean buildid_from_ini() (sp?) Looks like it's misspelled in both the call and the method name. >diff --git a/mozharness/mozilla/summary.py b/mozharness/mozilla/summary.py >+def per_locale_summary(func): >+def time_for_step(func): It looks like these methods aren't in use yet? >diff --git a/mozharness/mozilla/updates/balrog.py b/mozharness/mozilla/updates/balrog.py <snip> >- def submit_balrog_updates(self, release_type="nightly"): >- c = self.config >+ def submit_balrog_updates(self, marfile, hash_type, appName, appVersion, >+ platform, branch, buildid, complete_mar_url, >+ release_type="nightly", ): This is going to break b2g_build.py. We either need to make this backwards compatible, or change b2g_build.py to accomodate this change. diff --git a/scripts/desktop_l10n.py b/scripts/desktop_l10n.py I like the look of this cleanup... looks like 'make upload' and balrog help remove a lot of complexity. Awesome.

Also, from pyflakes/pylint: scripts/desktop_l10n.py:639: local variable 'platform' is assigned to but never used scripts/desktop_l10n.py:640: local variable 'hashType' is assigned to but never used scripts/desktop_l10n.py:617: [E, DesktopSingleLocale._query_objdir] Access to member 'objdir' before its definition line 620 scripts/desktop_l10n.py:618: [E, DesktopSingleLocale._query_objdir] Access to member 'objdir' before its definition line 620 I don't know about the latter two.

Thanks aki and rail for the feedback. current status: fixed all the problems with mock environment and mar tools. I am starting working on balrog submitter for macosx64, linux, linux64. win32 requires some more efforts so I'm not going to work actively on this platform until the other platforms get to cedar.

First patch for desktop repacks with mozharness, this patch includes only configuration changes

Second patch: changes in mozharness/ directory

Last one... the desktop_l10n.py script

Updated patch including last changes made in Bug 1034925

small update because of Bug 1034925 changes

I'm sorry, Jordan. I have uploaded a wrong patch file. Here's the correct one.

Moved from cedar to ash

...and buildbotcustom changes

Comment on attachment 8459054 [details] [diff] [review] [buildbotcustom] Bug 740142 - enable mozharness desktop repacks.patch Let's try it. I think it would be better to land this after the merge day.

Comment on attachment 8458993 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - mozharness files.patch Review of attachment 8458993 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. Depending on the result of funsize service, I may have to scrap my partial mar gen logic out of BuildScript and use this MarMixin; it's much better :) If you can confirm we still need the PRETTYNAMES stuff and that we can use {0}.format on strings in MarMixin, I'm cool with landing this. the three methods I mention below would be cool to have implementations of in mar.py so we can use the mixin as is on new scripts, but I won't block on landing this for that. If you agree with me, maybe you could write a follow up patch that adds the defaults? :) ::: mozharness/base/script.py @@ +874,5 @@ > try: > open(file_name, 'w').close() > except IOError as e: > + msg = "I/O error(%s): %s" % (e.errno, e.strerror) > + self.log(msg, error_level=error_level) it's too bad we can't use {0}.format yet and bump mozharness requirement to 2.7 :) At any rate, good catch ::: mozharness/mozilla/mar.py @@ +42,5 @@ > + def download_mar_tools(self): > + """downloads mar tools executables (mar,mbsdiff) > + and stores them local_dir()""" > + self.info("getting mar tools") > + dst_dir = self._mar_tool_dir() It looks like _mar_tool_dir is only implemented in your base desktop_l10n.py class. I'm on the side of if we want to use a Mixin, it should have pretty much have all the methods we need to use it or else have it in another mixin/subclass. desktop_l10n.py seems like an end script that we will never inherit from. I think _mar_tool_dir simply just joins a path with keys from query_abs_dirs and self.config. This mixin uses query_abs_dirs and self.config already so having a default _mar_tool_dir here with that same logic makes sense to me. @@ +76,5 @@ > + def _unpack_mar(self, mar_file, dst_dir, prettynames): > + """unpacks a mar file into dst_dir""" > + cmd = ['perl', self._unpack_script(), mar_file] > + env = deepcopy(self.query_repack_env()) > + env["MOZ_PKG_PRETTYNAMES"] = str(prettynames) sanity check, do we still use MOZ_PKG_PRETTYNAMES here? I know for desktop builds, we removed it except for release branches and nonunified builds. @@ +91,5 @@ > + # Usage: make_incremental_update.sh [OPTIONS] ARCHIVE FROMDIR TODIR > + cmd = [self._incremental_update_script(), partial_filename, > + current_dir, previous_dir] > + env = self.query_repack_env() > + cwd = self._mar_dir('update_mar_dir') same thing here with _mar_dir and _incremental_update_script as I mentioned with _mar_tool_dir. Maybe we can just have default implementations for all three of these in this mixin. I think _incremental_update_script could even use CONFIG from the top of this mar.py file. @@ +96,5 @@ > + self.mkdir_p(cwd) > + result = self.run_command(cmd, cwd=cwd, env=env) > + return result > + > + def query_build_id(self, mar_unpack_dir, prettynames): I think we are getting a few methods like these across our scripts. I'll be looking forward to hunting down duplicate code with you and unifying how we get things like buildid. :) @@ +106,5 @@ > + > + # log the content of application.ini > + with self.opened(ini_file, 'r') as (ini, error): > + if error: > + self.fatal('cannot open {0}'.format(ini_file)) do we need to s/{0}".format/%s" %/ this here like you patched above in Basescript

Comment on attachment 8458993 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - mozharness files.patch Review of attachment 8458993 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozharness/mozilla/mar.py @@ +84,5 @@ > + cwd=dst_dir, > + env=env, > + halt_on_failure=True) > + > + def do_incremental_update(self, previous_dir, current_dir, partial_filename, prettynames): oh I just noticed while looking at desktop_l10n.py: do we need this prettynames param here or in query_build_id signature?

Comment on attachment 8458988 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - desktop repacks script.patch Review of attachment 8458988 [details] [diff] [review]: ----------------------------------------------------------------- I'm nit'n a lot here. Overall it looks sweet. I'm excited to see this and desktop builds both land and get one step closer to mozharness'n all the things ;) I'll r+ once you reply or fix up anything you agree with me on. ::: scripts/desktop_l10n.py @@ +105,4 @@ > ]] > > def __init__(self, require_config_file=True): > + # fxbuild style: NICE! @@ +195,5 @@ > + """returns the enviorment used for the upload step""" > + if self.upload_env: > + return self.upload_env > + c = self.config > + buildid = self._query_buildid() moar query_buildid's lol. we are gonna have to fix this soon once we are all through the production gates @@ +300,5 @@ > return self.version > > + def upload_repacks(self): > + """iterates through the list of locales and calls make upload""" > + self.summarize(self.make_upload, self.query_locales()) oh, I like this summarize tactic. @@ +311,5 @@ > + total_count = len(items) > + name = func.__name__ > + for item in items: > + func(item) > + if self.return_code == 0: are you sure self.return_code will always be touched by func? I think we only mutate it if in certain methods, e.g. run_command() only touches it if halt_on_failure=True, in which case, we'd fatal out before even getting here. for self.make_upload I think it returns the return_code so we could use something like: if func(item) == 0: then, if you want to change the script's return code (self.return_code), you can set it in the else condition depending on if you want the build to green or not but please tell me if I'm wrong ;) @@ +378,5 @@ > + if self._make_dirs(): > + self.fatal("make dir failed!") > + # do we need it? > + if self.make_export(buildid): > + self.fatal("make export failed!") I am pretty sure none of these _setup_configure fatals could ever be hit. Have you seen them get hit with their messages before? I say that because it looks like each of _make_configure, _make_dirs, and make_export all rely on your _make() method _make() uses run_command and IIUC halt_on_failure=True on all of them from within _setup_configure. So if run_command returns any code other than success_codes(defaults to [0]), we will call self.fatal from within run_command. Does that make sense or do you think I have some false logic? for reference: http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#731 and http://mxr.mozilla.org/build/source/mozharness/scripts/desktop_l10n.py#402 @@ +493,2 @@ > if buildid is None: > return if this returns None, won't we silently fail with logic from _setup_configure ? @@ +525,5 @@ > + # no base_post_upload_cmd in configuration, just skip it > + pass > + target = ['upload', 'AB_CD=%s' % (locale)] > + cwd = dirs['abs_locales_dir'] > + parser = MakeUploadOutputParser(config=self.config, nice re-use :) @@ +552,5 @@ > # adding a replace(...) because make.py doesn't like > # --locale-mergedir=e:\...\...\... > # replacing \ with / > # this kind of hacks makes me sad > + # env['LOCALE_MERGEDIR'] = env['LOCALE_MERGEDIR'].replace("\\", "/") sanity check: do we still need these comments above? and if we are not replacing slashes or ..., did you add it in query_repack_env or do we not need to worry about that for windows anymore? @@ +587,5 @@ > + return > + > + if self.generate_complete_mar(locale) != 0: > + self.error("generate complete %s mar failed" % (locale)) > + return do these above need != 0? As in, could they be None, "", etc and those be valid? @@ +616,5 @@ > + localized_mar = os.path.join(self._mar_dir('update_mar_dir'), > + localized_mar) > + return localized_mar > + > + def create_partial_updates(self, locale): wow, so much cleaner than mine thanks to MarMixin and helper methods ;) @@ +729,5 @@ > + self.set_buildbot_property('properties', properties) > + > + # submit complete mar to balrog > + # clean up buildbot_properties > + self.summarize(self.submit_repack_to_balrog, self.query_locales()) hmm, so I suppose here is where why you used self.return_code in summarize. I think we will need submit_balrog_updates to also return the code and not just set self.return_code e.g. like you had, add: return return_code to here: http://mxr.mozilla.org/build/source/ash-mozharness/mozharness/mozilla/updates/balrog.py#45 @@ +770,5 @@ > + if locale not in self.partials: > + return [] > + > + # we have only a single partial for now > + # MakeUploadOutputParser can match a single parser so this means we can only support 1 partial? is that good enough to enable nightlies or is this WIP? @@ +797,5 @@ > + url += os.path.basename(self.query_marfile_path()) > + return url.format(branch=self.query_branch()) > + self.fatal("Couldn't find complete mar url in config or package_urls") > + > + def _query_partial_mar_url(self, locale): is this used anymore now that you're using MakeUploadParser? @@ +804,5 @@ > + return self.package_urls[locale]["partialMarUrl"] > + except KeyError: > + msg = "Couldn't find package_urls: {0} {1}".format(locale, self.package_urls) > + self.error("package_urls: %s" % (self.package_urls)) > + self.fatal(msg) I am not sure if we want to fatal out if we can't get a partial mar. double check with bhearsum though. @@ +812,5 @@ > + after make upload""" > + partial_mar_name = self.package_urls[locale]['partial_filename'] > + return os.path.join(self._update_mar_dir(), partial_mar_name) > + > + def _query_previous_mar_buildid(self, locale): not sure if we use this either. @@ +939,2 @@ > """returns the full path to current.work""" > return self._mar_dir('current_work_mar_dir') can the *_mar_dir() calls path joins get put in query_abs_dirs or do you prefer/require separate methods?

Comment on attachment 8458206 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - configuration.patch Review of attachment 8458206 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. fighting bit rot is hard eh? :) looks like you picked up on the tooltool/boto and other env changes. High level question for all these configs: are you using both the 'dev-' and non 'dev-' equivalent configs? If so, do they represent staging vs production, or developer vs automation? ::: configs/single_locale/dev-macosx64.py @@ +113,5 @@ > + "balrog_api_root": "http://127.0.0.1:9000", > + "balrog_username": "stage-ffxbld", > + "balrog_usernames": { > + "Firefox": "pass" > + } are you still using these or are you using staging from: /mozharness/configs/balrog/production.py /mozharness/configs/balrog/staging.py this question applies to everywhere else balrog stuff is mentioned ::: configs/single_locale/linux.py @@ +4,4 @@ > EN_US_BINARY_URL = "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central" > OBJDIR = "obj-l10n" > MOZ_UPDATE_CHANNEL = "nightly" > +STAGE_SERVER = "dev-stage01.srv.releng.scl3.mozilla.com" So I understand - so for now we are only using staging pool values like this one above? OOC how do we include production equivalents for enabling on our first branch. ::: configs/single_locale/linux64.py @@ +66,3 @@ > > + # MAR > + "previous_mar_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n", I see here and few places reference mozilla-central. is that the branch you are going to enable first? how do we support other branches? Sorry if I'm wrong or missing something ::: configs/single_locale/win32.py @@ +23,1 @@ > "mozilla_dir": MOZILLA_DIR, again mozilla-central explicitly mentioned here. @@ +23,2 @@ > "mozilla_dir": MOZILLA_DIR, > "snippet_base_url": "http://example.com", # fix it I see in linux64 you removed this item, are you going to use it in the future or can we remove it from every config? @@ +47,4 @@ > "MOZ_UPDATE_CHANNEL": MOZ_UPDATE_CHANNEL, > "DIST": "%(abs_objdir)s\\dist", > "LOCALE_MERGEDIR": "%(abs_merge_dir)s\\", > + # "MOZ_MAKE_COMPLETE_MAR": "1", can this be removed? @@ +59,4 @@ > "UPLOAD_USER": STAGE_USER, > "UPLOAD_SSH_KEY": STAGE_SSH_KEY, > "UPLOAD_HOST": STAGE_SERVER, > + # "POST_UPLOAD_CMD": "post_upload.py -b mozilla-central-android-l10n -p mobile -i %(buildid)s --release-to-latest --release-to-dated", can this mobile reference be removed?

Comment on attachment 8458993 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - mozharness files.patch Review of attachment 8458993 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozharness/mozilla/mar.py @@ +67,5 @@ > + """creates a temporary directory for mar unpack""" > + # tempfile.makedir() and TemporaryDir() work great outside mock envs > + mar_dir = os.path.join(self._temp_mar_base_dir(), name) > + # delete mar_dir, it prints a message if temp_dir does not exist.. > + self.rmtree(self._temp_mar_dir()) just noticed this. IIUC we might hit a recursive loop here no? do you mean to self.rmtree(mar_dir)?

Thanks for the reviews jlund! I have landed the code on ash-mozharness so next patches will be smaller

Hi Jordan, in this patch: * moved _mar_tool_dir() and _incremental_update_script() from scripts/desktop_l10n.py to mozharness/mozilla/mar.py * removed any reference to prettynames, I've done some tests in staging, you can set it to any value, results will not change. * removed .format() calls * remove unpack_script, incremental_update_script and update_packaging_dir from CONFIG, unfortunately we need them per platform as windows needs \\ instead of / * renamed query_build_id() -> get_buildid_from_mar_dir(); we already have a query_buildid() method

Comment on attachment 8458988 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - desktop repacks script.patch Review of attachment 8458988 [details] [diff] [review]: ----------------------------------------------------------------- since we will be going with smaller patches from here on, I'll reset these large guys.

First round of fixes. This patch is generated against ash-mozilla changeset: 204643858d0f changes included: * moved _mar_tool_dir() and _incremental_update_script() from desktop_l10n.py to mar.py * removed any reference to prettynames * removed format() calls * removed unused imports

Comment on attachment 8462255 [details] [diff] [review] ash-mozharness.patch Review of attachment 8462255 [details] [diff] [review]: ----------------------------------------------------------------- awesome! lvgtm You didn't even fight back with me on any of my comments ;)

Thanks Jordan. Landed in ash-mozharness.

Code on ash, fails to do the repacks because the configuration was pointing (on purpose) on our staging environment and try machines do not have the credentials to copy files on staging servers so make_upload fails. This patch is a first step to have complete working system on ash: it removes the hardcoded mozilla-central.py values from the configuration, adding two new files config/single_locale/ash.py and config/single_locale/mozilla-central.py (I'm thinking to move them into config/single_locale/branches - for a more clear layout). Each files contains the information about their branch. Following the same idea, I have added staging.py with all the information about the staging environment (servers, upload location,... ). A production.py configuration will follow soon. ... and thanks to the new files, tons on options from platform configuration, macosx64.py, linux.py and linux64.py have been removed. For example the balrog credentials that are already in present locally (mozharness/configs/balrog/staging.py) and there's no need to have them again in repacks config. Calling the repack script now requires some extra parameters: python scripts/desktop_l10n.py --cfg mozharness/configs/single_locale/ash.py \ --cfg mozharness/configs/single_locale/staging.py \ --cfg mozharness/configs/balrog/staging.py \ --cfg mozharness/configs/single_locale/macosx64.py Before this change the same info now spread through the few configuration files, was in stored in macosx64.py (or linux.py, or linux64.py, ...). This caused the current configuration to be totally inflexible and hard to maintain (It would end up in several copies of the "almost" same configuration with just few changes in each file). Now each entity, branch, platform, environment and balrog credentials have their own configuration. (I'm planning use better option names as --branch, --project, ... in a next version) On the other side, this new configuration schema requires extra care: all the %(foo)s elements in our files need to be replaced in with a real value and this is done on the expenses of more code (more replace_dict). As it is now, the script works locally with ash configuration but I hit bug 1052536 on mozilla-central (make configure fails) I'd love also to test this code on the real to be sure it uploads files with a path that does not conflict with *important* repacks (e.g. mozilla-central repacks are not overwritten), and when we are sure no important files are replaced, we can enable the production configuration and have repacks live on ash. But before proceeding any further, I would like to know your opinion on this mechanism since, I guess, you had similar issues with the desktop build.

(In reply to Massimo Gervasini [:mgerva] from comment #68) > Created attachment 8473323 [details] [diff] [review] > [mozharness] Bug 740142 - Move Firefox Desktop repacks to use > mozharness.patch > will take deep look tomorrow :)

Hi Jordan, I am felling bad for the gigantic patch I've sent you before so I have generated a new one from a more recent commit (6790b11df71556 - from my github repo). It's still huge but it's not bad as the one before. I haven't removed the attachment 8473323 [details] [diff] [review] because I don't know the status of your feedback and it would not be fair to obsolete it in the middle of your task but If you haven't started yet... this one should make your work easier.

Comment on attachment 8474471 [details] [diff] [review] [mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - a smaller patch Review of attachment 8474471 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good to me :) I think you'll be able to scale this better now that you ripped out branches and staging/prod env. Looking forward to seeing this in action! ::: configs/single_locale/ash.py @@ +1,4 @@ > +config = { > + "branch": "ash", > + "en_us_binary_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/%(branch)s-%(platform)s/latest/", > + "branch_repo": "https://hg.mozilla.org/projects/%(branch)s", I'm not sure how branch_repo works in your script but I ended up removing 'projects' to support release branches like mozilla-central. this is how I did it: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#676 @@ +15,5 @@ > + "mar_tools_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/%(branch)s-%(platform)s/latest/", > + > + # just for testing... (patials/complete mar are not generated on ash) > + "previous_mar_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n", > + "current_mar_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central", 1) do we do nightlies on ash? 2) IIRC I think this might be specific to mozilla-central no? ::: configs/single_locale/linux.py @@ +2,1 @@ > BRANCH = "mozilla-central" IIUC we don't need this var anymore? ::: configs/single_locale/linux64.py @@ +68,1 @@ > "partials_url": "%(base_url)s/latest-mozilla-central/", I think you ripped this out into ash.py ::: configs/single_locale/mozilla-central.py @@ +1,4 @@ > +config = { > + "branch": "mozilla-central", > + "en_us_binary_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central", > + "branch_repo": "https://hg.mozilla.org/mozilla-central", ah ok. I guess this way works if you supply the whole url like you are. I think the other reason I did stuff like: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#676 was because I realized later on that I wouldn't need a config for every project branch. Some branches the only difference was the branch name. Well the only difference that mozharness cared about anyway ::: configs/single_locale/staging.py @@ +4,5 @@ > +AUS_USER = "ffxbld" > +AUS_SSH_KEY = "~/.ssh/ffxbld_dsa" > +AUS_UPLOAD_BASE_DIR = "/opt/aus2/incoming/2/Firefox" > +AUS_BASE_DIR = "%(branch)s/%(build_target)s/%(buildid)s/%(locale)s" > +AUS_SERVER = "blah" lol

Thanks Jordan, and sorry again for the huge patch. I am going through the list of comments you made and generate a new one soon. > 1) do we do nightlies on ash? IIRC we do not do nightlies on ash, and I guess they need to be enabled. catlee, any objections on enabling nightlies on ash?

Enable nightlies on ash: I've updated my staging master with this patch. This is the list of added builders: Android 2.3 ash nightly Android 4.2 x86 ash nightly Linux ash nightly Linux x86-64 ash nightly OS X 10.7 ash nightly WINNT 5.2 ash nightly and no builders have been removed

Something here went into production :)

desktop repacks have a new configuration structure; removed 'config': 'single_locale/... from project branches.

added new parameters required by desktop repacks: * branch-config * platform-config * environment-config * balrog-config

(In reply to Massimo Gervasini [:mgerva] from comment #76) > Created attachment 8482794 [details] [diff] [review] > [buildbotcustom] Bug 740142 - updated configuration for desktop_repacks with > mozharness.patch > > added new parameters required by desktop repacks: > > * branch-config > * platform-config > * environment-config > * balrog-config I have forgot to mention, values for now are hard coded to staging configuration files, I'd like to run some desktop repacks on ash and check that all the uploaded files are not overwriting our real nightly files.

Hey Jordan, sorry for the monster patch, again. Trying to have repacks working on ash, exposed how badly the configuration was dependent on mozilla-central values. With this patch I'm partitioning the monolithic configuration file (was single_locales/linux64.py) into several files: * branch configuration * platform configuration * environment configuration (tells the script where upload our files) * balrog configuration - it's already part of our configuration so I've just used it I have used this approach to avoid having several almost similar files as: * ash-macosx64.py * ash-linux.py * ash-linux64.py * ash-win32.py * ash-win64.py ... repeat for each branch using this pattern should make things easier to extend the repack to other branches/platforms/environments The idea is to configure each branch and platform just once and let the desktop digest them as it was an old style single long file. Having an easier to understand configuration is done at the code expenses. To be universal, the configuration has now some "tokens". For example "platform" is defined in linux.py/linux64/... but the platform value is needed in configs/single_locale/ash.py too to define the en_us_binary_url and the mar_tools_url urls. To be sure that every important configuration option such as platform, branch, ... will have a real value at run time, the code is now enforcing that they are defined and they have a value before the configuration becomes read only. If any of these values is not defined, the script just refuses to run. Better fail soon rather than running an 1h script and fail at the last step. When the configuration meets the basic requirements, tokens ( %(branch)s,... ) are replaced with their real value, so in the rest of the script we can use configuration values straight away and remove some of the replace_dict patterns: replace_dict = {'platform': config['platform'], 'other_value': ...} values = config['key'] % replace_dict ...at least for values that are already defined before we run the script. to do: * make sure it works on ash (uploading repacks into the right places) * make configuration more stable: some values are still moving from a configuration file to another. I'm trying to find the right place for each option/value. next steps: * extend it to m-c * extend it to any other branch ... and in the future * if this configuration pattern works we could create a decorator: @detokenize_configuration __pre_config_lock(self, rw_config): ... * it would be also interesting to define a set of runtime tokens, and enforce that only this set of tokens appears into our configuration after the configuration becomes read only (this could be a decorator too)

Comment on attachment 8482794 [details] [diff] [review] [buildbotcustom] Bug 740142 - updated configuration for desktop_repacks with mozharness.patch Review of attachment 8482794 [details] [diff] [review]: ----------------------------------------------------------------- one concern with hardcoding staging but will reply under your comment about that.

> I have forgot to mention, values for now are hard coded to staging > configuration files, I'd like to run some desktop repacks on ash and check > that all the uploaded files are not overwriting our real nightly files. so with hardcoding staging on ash, will we not run into issues? i.e. does ash (and the slaves run under it) not use production keys? IIUC, I'm pretty sure many parts will barf when you try to use a prod slave -> ash -> staging env. Sorry if I'm wrong, just wanted to raise the question before we go ahead with this.

Comment on attachment 8482848 [details] [diff] [review] [mozharness] Bug 740142 - desktop repacks updated configuration management.patch Review of attachment 8482848 [details] [diff] [review]: ----------------------------------------------------------------- These changes look great. I will submit this for now. I've done every file but desktop_l10n.py I'll have a look at that tomorrow. ::: configs/single_locale/ash.py @@ +1,3 @@ > +config = { > + "branch": "ash", > + "en_us_binary_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/%(branch)s-%(platform)s/latest/", since we know the branch throughout this file, why are we choosing to tokenize these parts at runtime instead of doing say: BRANCH = 'ash' config = { "branch": BRANCH, # I think the syntax is a double %% but you know what I mean "en_us_binary_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/%s-%%(platform)s/latest/", % (branch,) totally cool with what you're doing but just making sure that I am understanding things clearly ::: configs/single_locale/dev-linux.py @@ +1,1 @@ > +BRANCH = "mozilla-central" do we still need to hard code m-c ? do I understand correctly that these dev-{platform}.py configs are meant to be used by dev-master/staging? if so, does this mean that we can only do staging mozilla-central repacks? why do we need the dev-{platform}.py configs at all if we are using --cfg staging.py now? ::: configs/single_locale/linux.py @@ +1,2 @@ > +PLATFORM = 'linux' > +BRANCH = "mozilla-central" can we delete BRANCH? The same goes for any config that mentions mozilla-central outside of mozilla-central.py @@ +1,5 @@ > +PLATFORM = 'linux' > +BRANCH = "mozilla-central" > +HG_SHARE_BASE_DIR = "/builds/hg-shared" > +OBJDIR = "obj-l10n" > +MOZILLA_DIR = "%(branch)s" do we need MOZILLA_DIR? ::: configs/single_locale/staging.py @@ +4,5 @@ > +AUS_USER = "ffxbld" > +AUS_SSH_KEY = "~/.ssh/ffxbld_dsa" > +AUS_UPLOAD_BASE_DIR = "/opt/aus2/incoming/2/Firefox" > +AUS_BASE_DIR = "%(branch)s/%(build_target)s/%(buildid)s/%(locale)s" > +AUS_SERVER = "blah" blah? lol :) ::: configs/single_locale/win32.py @@ +12,5 @@ > +# AUS_SERVER = "aus2-staging.mozilla.org" > +AUS_USER = "ffxbld" > +AUS_SSH_KEY = "~/.ssh/ffxbld_dsa" > +AUS_UPLOAD_BASE_DIR = "/opt/aus2/incoming/2/Firefox" > +AUS_BASE_DIR = BRANCH + "/%(build_target)s/%(buildid)s/%(locale)s" again lot's of branch and build pool env specific stuff in this file and win64.py. I'm guessing you have only done linux/mac? If so, we should probably not push this windows stuff ::: mozharness/mozilla/mar.py @@ +4,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > +# ***** END LICENSE BLOCK ***** > +"""MarMixin, manages mar files""" > + hmm, so does this replace http://mxr.mozilla.org/build/source/mozharness/mozharness/base/mar.py ? are we removing that file? I think this file addresses my concerns from https://bugzilla.mozilla.org/show_bug.cgi?id=740142#c57 but it is hard to tell without a diff.

Comment on attachment 8482848 [details] [diff] [review] [mozharness] Bug 740142 - desktop repacks updated configuration management.patch Review of attachment 8482848 [details] [diff] [review]: ----------------------------------------------------------------- OK I think I've skimmed over everything. At this size, I am relying heavily on you for correct logic and implementation. This seems like a massive improvement. I really like the way you have tackled the 'config' problem with the tokenizer. Like you mentioned, it will be great to extend this from desktop_l10n and put it as a part of the internals in mozharness. I bet others will appreciate it :) I think it will be good to see these changes in action on something like ash (now that we have nightlies). Then cross reference we are doing everything needed from the buildbot factory counterpart. Disclaimer: I am unfamiliar with the details in a repack process. We should have someone verify the actions/logs from a real run of this before moving forward. I am going to leave this review open at a ? until you reply to my questions. I don't think we need this to be perfect but I also don't want to leave anything I've got that shouldn't be there left there as I suspect this code will live for a long time. ::: scripts/desktop_l10n.py @@ +55,5 @@ > + 'branch_repo') > +# some other values such as "%(version)s", "%(buildid)s", ... > +# are defined at run time and they cannot be enforced in the _pre_config_lock > +# phase > +runtime_config_tokens = ('buildid', 'version') so remind me here, we are not using these at all but we define them none the less? @@ +140,4 @@ > ]] > > def __init__(self, require_config_file=True): > + # fxbuild style: awwww yeee :) @@ +165,5 @@ > + "buildid_option": "BuildID", > + "application_ini": "application.ini", > + "unpack_script": "tools/update-packaging/unwrap_full_update.pl", > + "log_name": "single_locale", > + "clobber_file": 'CLOBBER', so how does this work? It seems 'CLOBBER' is the only value for this? @@ +203,5 @@ > + # in the configuration before it is locked down > + # mandatory tokens > + for token in configuration_tokens: > + if token not in self.config: > + self.fatal('No %s in configuration!' % token) I like the validation here @@ +233,5 @@ > + > + # now scan self.abs_dirs too. It has been populated before > + # _pre_config_lock so it might have some %(...)s tokens in it > + # let's remove them. > + for key in self.abs_dirs: We no longer need this thanks to Bug 1060310 correct? @@ +256,5 @@ > + for key in common_keys: > + old_ = before[key] > + new_ = after[key] > + if old_ != new_: > + self.info("{0} => {1}".format(old_, new_)) removed_keys, added_keys, and common_keys feel like perfect unittests that should be part of unit.sh. maybe we can rip that out and test various script calls against desktop_l10n.py. Then if/when we rip this out as a _pre_config_lock decorator, we can add more unittests. @@ +304,3 @@ > repack_env = self.query_env(partial_env=config.get("repack_env"), > replace_dict=replace_dict) > + if config.get('en_us_binary_url') and \ is it possible to not have en_us_binary_url now? IIUC, _pre_config_lock defines it as mandatory @@ +495,5 @@ > config = self.config > dirs = self.query_abs_dirs() > repos = [] > + for option in config: > + self.info('{0} {1}'.format(option, config[option])) wait, why are we dumping self.config in the log again? @@ +571,5 @@ > dst = os.path.join(dirs['abs_mozilla_dir'], '.mozconfig') > self.copyfile(src, dst) > > # STUPID HACK HERE > # should we update the mozconfig so it has the right value? this is a good Q. who can we ask about this. this seems bad to be doing at runtime. is this what we do in MozillaRepackFactory? @@ +732,5 @@ > + self.error("generate partials %s failed" % (locale)) > + return > + else: > + self.info("partial updates are not enabled, skipping") > + return 0 are we swallowing the return from this method? It appears like your summarize looks for changes to self.return_code. Or do I have that wrong? @@ +855,5 @@ > + > + # balrog submitter requires buildbot['properties']['product'] > + # if it does not exist the submission will fail. > + try: > + properties = self.query_buildbot_property("properties") I think this touches self.buildbot_properties (mozharness runtime props that are uploaded after script runs). don't you want self.buildbot_config (represents props set before script run and read from buildprops.json) ? @@ +858,5 @@ > + try: > + properties = self.query_buildbot_property("properties") > + except AttributeError: > + # no properties set for buildbot, initialize to empty dict > + self.buildbot_properties = {} IIUC you always hit this exception because of said above @@ +875,5 @@ > + """submit a single locale to balrog""" > + if not self.query_is_nightly(): > + self.info("Not a nightly build") > + # extra safe > + # return is the return here supposed to be commented out? @@ +930,5 @@ > + return self.config["complete_mar_url"] > + if "completeMarUrl" in self.package_urls[locale]: > + return self.package_urls[locale]["completeMarUrl"] > + # XXX: remove this after everything is uploading publicly > + url = self.config.get("update", {}).get("mar_base_url") do we still use this? does self.config['update'] still exist?

Thanks for the reviews, Jordan I am uploading a new patch soon to address all the problems. Answers inline. > ::: scripts/desktop_l10n.py > @@ +55,5 @@ > > + 'branch_repo') > > +# some other values such as "%(version)s", "%(buildid)s", ... > > +# are defined at run time and they cannot be enforced in the _pre_config_lock > > +# phase > > +runtime_config_tokens = ('buildid', 'version') > > so remind me here, we are not using these at all but we define them none the > less? added the missing method: _get_configuration_tokens(), now pre_config_lock() fails if a token is in our configuration but it is not in runtime_config_tokens. > > @@ +165,5 @@ > > + "buildid_option": "BuildID", > > + "application_ini": "application.ini", > > + "unpack_script": "tools/update-packaging/unwrap_full_update.pl", > > + "log_name": "single_locale", > > + "clobber_file": 'CLOBBER', > > so how does this work? It seems 'CLOBBER' is the only value for this? removed clobber_file from platform configuration and using the default value. This configuration points to a file, 'CLOBBER' that is used to prevent downloaded binaries to be deleted just after the download. > > @@ +233,5 @@ > > + > > + # now scan self.abs_dirs too. It has been populated before > > + # _pre_config_lock so it might have some %(...)s tokens in it > > + # let's remove them. > > + for key in self.abs_dirs: > > We no longer need this thanks to Bug 1060310 correct? Correct, bug 1060310 is in production, snippet removed. > @@ +256,5 @@ > > + for key in common_keys: > > + old_ = before[key] > > + new_ = after[key] > > + if old_ != new_: > > + self.info("{0} => {1}".format(old_, new_)) > > removed_keys, added_keys, and common_keys feel like perfect unittests that > should be part of unit.sh. maybe we can rip that out and test various script > calls against desktop_l10n.py. Then if/when we rip this out as a > _pre_config_lock decorator, we can add more unittests. > > @@ +304,3 @@ > > repack_env = self.query_env(partial_env=config.get("repack_env"), > > replace_dict=replace_dict) > > + if config.get('en_us_binary_url') and \ > > is it possible to not have en_us_binary_url now? IIUC, _pre_config_lock > defines it as mandatory removed. > @@ +495,5 @@ > > config = self.config > > dirs = self.query_abs_dirs() > > repos = [] > > + for option in config: > > + self.info('{0} {1}'.format(option, config[option])) > > wait, why are we dumping self.config in the log again? right! removed. > @@ +571,5 @@ > > dst = os.path.join(dirs['abs_mozilla_dir'], '.mozconfig') > > self.copyfile(src, dst) > > > > # STUPID HACK HERE > > # should we update the mozconfig so it has the right value? > > this is a good Q. who can we ask about this. this seems bad to be doing at > runtime. is this what we do in MozillaRepackFactory? I don't have an answer for this. I need to run a complete run to verify if this is sill happening. > @@ +732,5 @@ > > + self.error("generate partials %s failed" % (locale)) > > + return > > + else: > > + self.info("partial updates are not enabled, skipping") > > + return 0 > > are we swallowing the return from this method? It appears like your > summarize looks for changes to self.return_code. Or do I have that wrong? yes, here we have some serial actions: compare locale => make_installers => generate_complete_mar => generate_partial_updates, they can be executed only if the previous action was successful. If any of them fail, we interrupt the repack for the current locale (we can extend the repack_locale() to do retries in the future), and summarize will report it as failed repack. Removed the last 2 return statements. > @@ +855,5 @@ > > + > > + # balrog submitter requires buildbot['properties']['product'] > > + # if it does not exist the submission will fail. > > + try: > > + properties = self.query_buildbot_property("properties") > > I think this touches self.buildbot_properties (mozharness runtime props that > are uploaded after script runs). thanks, updated. > don't you want self.buildbot_config (represents props set before script run > and read from buildprops.json) ? > > @@ +858,5 @@ > > + try: > > + properties = self.query_buildbot_property("properties") > > + except AttributeError: > > + # no properties set for buildbot, initialize to empty dict > > + self.buildbot_properties = {} > > IIUC you always hit this exception because of said above > > @@ +875,5 @@ > > + """submit a single locale to balrog""" > > + if not self.query_is_nightly(): > > + self.info("Not a nightly build") > > + # extra safe > > + # return > > is the return here supposed to be commented out? this script as it is now, works only for nightlies, better fail if this is not a nightly build. We should remove the check when we extend this script to non-nightly repacks. > > @@ +930,5 @@ > > + return self.config["complete_mar_url"] > > + if "completeMarUrl" in self.package_urls[locale]: > > + return self.package_urls[locale]["completeMarUrl"] > > + # XXX: remove this after everything is uploading publicly > > + url = self.config.get("update", {}).get("mar_base_url") > > do we still use this? does self.config['update'] still exist? This method is a copy and paste from b2g module, investigating.

Hey Jordan, just a delta between the last patch and changes requested in comment 82.

> > @@ +732,5 @@ > > > + self.error("generate partials %s failed" % (locale)) > > > + return > > > + else: > > > + self.info("partial updates are not enabled, skipping") > > > + return 0 > > > > are we swallowing the return from this method? It appears like your > > summarize looks for changes to self.return_code. Or do I have that wrong? > > yes, here we have some serial actions: compare locale => make_installers => > generate_complete_mar => generate_partial_updates, they can be executed only > if the previous action was successful. If any of them fail, we interrupt the > repack for the current locale (we can extend the repack_locale() to do > retries in the future), and summarize will report it as failed repack. > Removed the last 2 return statements. > Sorry if I'm not getting it but it seems to me that all the returns in this method do nothing. I would've thought we would have to mutate self.return_code so summarize would pick it up. I am up to speed on everything else, thanks for the comments. Reviewing now

Comment on attachment 8486479 [details] [diff] [review] [mozharness] Bug 7404142 - desktop_l10n.py updates per comment 82.patch Review of attachment 8486479 [details] [diff] [review]: ----------------------------------------------------------------- love it! :) ::: scripts/desktop_l10n.py @@ +237,5 @@ > + msg.append(t) > + self.fatal(' '.join(msg)) > + self.info('configuration looks ok') > + > + def _get_configuration_tokens(self, iterable): all of this and pre_config_lock is coming along really nicely. I <3 the changes u made! @@ +860,4 @@ > # balrog submitter requires buildbot['properties']['product'] > # if it does not exist the submission will fail. > try: > + # properties = self.query_buildbot_property("properties") sweet. this can go I think. I think u have it now @@ +936,5 @@ > return self.package_urls[locale]["completeMarUrl"] > + # url = self.config.get("update", {}).get("mar_base_url") > + # if url: > + # url += os.path.basename(self.query_marfile_path()) > + # return url.format(branch=self.query_branch()) I'm guessing you're just testing this but if you want to keep it, we should include why

Comment on attachment 8482848 [details] [diff] [review] [mozharness] Bug 740142 - desktop repacks updated configuration management.patch Review of attachment 8482848 [details] [diff] [review]: ----------------------------------------------------------------- we are going with changes. r-'ing this for now

Log of the last upload from dev-master01; ash repacks are uploaded to: http://dev-stage01.srv.releng.scl3.mozilla.com/pub/mozilla.org/firefox/nightly/2014/09/2014-09-25-04-02-05-ash-l10n/... Since ash repacks have their own paths and they will not overwrite anything on the upload server, I am creating a patch to switch uploads from dev-stage01.srv.releng.scl3.mozilla.com to stage.mozilla.org

use stage.mozilla.org for uploads

Enable production configuration for desktop repacks. Just as a note: repacks are enabled only on ash and they are on demand.

Comment on attachment 8495353 [details] [diff] [review] [buildbotcustom] Bug 740142 - enable production configuration for desktop repacks.patch Review of attachment 8495353 [details] [diff] [review]: ----------------------------------------------------------------- won't block since it is just ash but I think we need to do something like this: http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1023 where we pivot off: http://mxr.mozilla.org/build/source/buildbot-configs/mozilla/staging_config.py#33 that way we can test on dev-master or local setup and it will be safe.

Comment on attachment 8495350 [details] [diff] [review] [mozharness] Bug 740142 - added upload environment for production.patch Review of attachment 8495350 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. btw - you still have a bunch of prod/staging stuff outside of staging.py and production.py. what is happening with that stuff. I mentioned this before. see snippet from comment 81: """ ::: configs/single_locale/win32.py @@ +12,5 @@ > +# AUS_SERVER = "aus2-staging.mozilla.org" > +AUS_USER = "ffxbld" > +AUS_SSH_KEY = "~/.ssh/ffxbld_dsa" > +AUS_UPLOAD_BASE_DIR = "/opt/aus2/incoming/2/Firefox" > +AUS_BASE_DIR = BRANCH + "/%(build_target)s/%(buildid)s/%(locale)s" again lot's of branch and build pool env specific stuff in this file and win64.py. I'm guessing you have only done linux/mac? If so, we should probably not push this windows stuff """ I also see stuff like: http://mxr.mozilla.org/build/search?string=dev-stage01&find=configs%2Fsingle_locale&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=build

You can rip out any code that generates snippets and uploads them to aus2-staging, because that's going to get disabled in bug 933426, and possibly removed from buildbotcustom. We'll just need Balrog support.

Merged to production, and deployed.

Hi Jordan, test repacks on ash have just worked fine. I'd like to merge the changes from ash-mozharness back into mozharness. What we have in ash-mozharness right now is the sum all the previous patches/reviews plus some minor changes in the configuration (AUS_* values are gone,...), the patch is *huge* so let'me try to summarize it. Changes made by this patch against the version we currently have in mozharness: * updated the desktop_l10n.py new configuration style and green builds on linux/linux64 * moved mar.py from mozharness/base/ to mozharness/mozilla/ => updated docs/mozharness.{base,mozilla}.rst to reflect this change * created new configuration files, in configs/single_locale: ash.py, mozilla-central.py, linux.py, linux64.py,... * removed obsolete configuration files: mozilla-central_{desktop,linux,..}.py Any change made by this patch is not touching scripts/configuration/libraries outside the desktop repacks except for: mozharness/base/script.py; it updates the _touch_file() adding a new parameter, error_level with FATAL as default. full list of changes: files updated: M docs/mozharness.base.rst M docs/mozharness.mozilla.rst M mozharness/base/script.py M scripts/desktop_l10n.py new files: A configs/single_locale/ash.py A configs/single_locale/linux.py A configs/single_locale/linux64.py A configs/single_locale/macosx64.py A configs/single_locale/mozilla-central.py A configs/single_locale/staging.py A configs/single_locale/win32.py A configs/single_locale/win64.py A mozharness/mozilla/mar.py files removed: R configs/single_locale/mozilla-central_desktop.py R configs/single_locale/mozilla-central_linux.py R configs/single_locale/mozilla-central_linux64.py R configs/single_locale/mozilla-central_macosx64.py R configs/single_locale/mozilla-central_win32.py R configs/single_locale/mozilla-central_win64.py R mozharness/base/mar.py next steps: * ash repacks are on demand, we should create repacks every time a new nightly is created on ash - we could use the same logic introduced by mozharness build, here. * test if the new binaries, created by this script, are what we are really expecting (can we install, update, ...?) * extend the script to work on windows platforms (replace _make() calls with mach?) * extend to real nightlies

(In reply to Massimo Gervasini [:mgerva] from comment #95) > Created attachment 8501822 [details] [diff] [review] > [mozharness] Bug 740142 - mozharness desktop repacks - fixes from ash.patch > > Hi Jordan, > > test repacks on ash have just worked fine. > > I'd like to merge the changes from ash-mozharness back into mozharness. > What we have in ash-mozharness right now is the sum all the previous > patches/reviews plus some minor changes in the configuration (AUS_* values > are gone,...), the patch is *huge* so let'me try to summarize it. > hmm, ++ that we are landing this. though, this is the third 60-120kb patch based on the same code. To save me having to go through everything line by line again, is there a way you can get me an interdiff I can cross reference with? I'm mostly interested in things that I mentioned concern about before but allowed a checkin anyway since we don't require reviews on ash-mozharness.

> > test repacks on ash have just worked fine. > > > > I'd like to merge the changes from ash-mozharness back into mozharness. > > What we have in ash-mozharness right now is the sum all the previous > > patches/reviews plus some minor changes in the configuration (AUS_* values > > are gone,...), the patch is *huge* so let'me try to summarize it. > > > > hmm, ++ that we are landing this. though, this is the third 60-120kb patch > based on the same code. To save me having to go through everything line by > line again, is there a way you can get me an interdiff I can cross reference > with? I'm mostly interested in things that I mentioned concern about before > but allowed a checkin anyway since we don't require reviews on > ash-mozharness. mgerva: ping wrt ^ if an interdiff is not possible, I'll review on monday :)

Hi Jordan, sorry for the delay, here are some diffs I have created from my git repository, I hope it helps! Changes in mar.py moved from mozharness/base/ to mozharness/mozilla/ * https://github.com/gerva/mozharness/commit/8285517e5e83ceb39fbdea24488bbaa08b07f083 Removed deepcopy * https://github.com/gerva/mozharness/commit/443c302dbf76402c83c7104000577d4e87cbffe0 Added replace_dict in mar.py * https://github.com/gerva/mozharness/commit/6c562a6131faf96454d3b7d72ecdbbf0a2a529cc Changes in desktop_l10n.py: comment #82 * https://github.com/gerva/mozharness/commit/0f6f671e0765f74d32699cbb8707665199b771bc removed return 0 * https://github.com/gerva/mozharness/commit/afa8a194f2d685b305468c172e7df1af70f7e3e9 removed MobileSigningMixin * https://github.com/gerva/mozharness/commit/5a46f57046893be8ad18242186dc02514732edfb removed TransferMixin * https://github.com/gerva/mozharness/commit/1c1ad60c1fb87c515408d9e31df9db67f1705109 macosx64 needs "MOZ_PKG_PLATFORM": "mac" * https://github.com/gerva/mozharness/commit/b0bb9caa950249d156c8adc40f48dfadcf7ac5ff Changes in configuration files: moving to new style configurations: * https://github.com/gerva/mozharness/commit/d8eac3890694d9f56822ac109598153c2c2d0f1b moved "repos" section from platform to branch configuration: * https://github.com/gerva/mozharness/commit/8aaa29fa2db566939c24e2252adee5396e318b0a removed clobber file (comment #82): * https://github.com/gerva/mozharness/commit/7f65c8a2442676fae5fb8733d3b239a7c422910c added update_platform (replaces build_target) * https://github.com/gerva/mozharness/commit/f246e6cf84d6d7d3b17b9229dc58390cd1a1f7f5 removed aus_* entries in configuration; removed variables not in use: * https://github.com/gerva/mozharness/commit/982c679d9115e4acc3a23e92333ac7abb0ad33db

Comment on attachment 8501822 [details] [diff] [review] [mozharness] Bug 740142 - mozharness desktop repacks - fixes from ash.patch Review of attachment 8501822 [details] [diff] [review]: ----------------------------------------------------------------- lgtm r+ :) I am relying heavily on you for the logic and at this size, I'm not going over every line so we will need to test this heavily before proceeding. I think we should have an experienced l10n person to evaluate an actual build and verify we are doing things correctly. As mentioned, I think these changes are a big improvement (I'd love to re-use the tokenizer). Let's get this beast in to mainline mozharness so we can iterate on small patches going forward. :D Saying that, I'd like to address my concerns with summarize() before doing any buildbot-config patches or enabling mozharness desktop l10n branches (outside of ash). ::: configs/single_locale/ash.py @@ +9,5 @@ > + # mar > + "enable_partials": True, > + "mar_tools_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-%(platform)s/latest/", > + "previous_mar_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n", > + "current_mar_url": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central", these still mention mozilla-central. is it supposed to be ash? ::: configs/single_locale/linux.py @@ +1,2 @@ > +PLATFORM = 'linux' > +BRANCH = "mozilla-central" from https://bugzilla.mozilla.org/show_bug.cgi?id=740142#c71 and https://bugzilla.mozilla.org/show_bug.cgi?id=740142#c81 I don't think this BRANCH is needed anymore. nor is MOZILLA_DIR @@ +13,5 @@ > + "repack_env": { > + "MOZ_OBJDIR": OBJDIR, > + "EN_US_BINARY_URL": "%(en_us_binary_url)s", > + "LOCALE_MERGEDIR": "%(abs_merge_dir)s/", > + "MOZ_UPDATE_CHANNEL": MOZ_UPDATE_CHANNEL, IIRC, MOZ_UPDATE_CHANNEL changes by branch. May need to double check that ::: configs/single_locale/linux64.py @@ +1,1 @@ > +BRANCH = "mozilla-central" same as linux.py ::: configs/single_locale/staging.py @@ +2,5 @@ > + "upload_env": { > + "UPLOAD_USER": "ffxbld", > + "UPLOAD_SSH_KEY": "~/.ssh/ffxbld_dsa", > + "UPLOAD_HOST": "dev-stage01.srv.releng.scl3.mozilla.com", > + "POST_UPLOAD_CMD": "post_upload.py -b %(branch)s-l10n -p firefox -i %(buildid)s --release-to-latest --release-to-dated", in desktop build world[1], post_upload_cmd changes quite a bit. is it as static as this in desktop l10n? http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#798 ::: scripts/desktop_l10n.py @@ +194,2 @@ > if 'mock_target' in self.config: > self.enable_mock() so remind me, do we just run every run_command through mock in this script for linux? @@ +194,5 @@ > if 'mock_target' in self.config: > self.enable_mock() > > + def _pre_config_lock(self, rw_config): > + """replaces 'configuration_tokens' with their values, before the still <3 this @@ +455,5 @@ > + total_count = len(items) > + name = func.__name__ > + for item in items: > + func(item) > + if self.return_code == 0: summarize() looks neat and appears to help reduce a lot of duplicate code but I am wary that it works as you expect. I don't think we are tracking failures. It might always be success... we talked about this in https://bugzilla.mozilla.org/show_bug.cgi?id=740142#c83 but I still think something is wrong. e.g. what code will manipulate self.return_code change for say the repack action? The make targets in repack appear to call run_command but they don't seem to change self.return_code. Or am I incorrect? Also, self.return_code is just one var used to track the return_code of the entire script. Once it no longer is 0 (let's say you manipulate it from some self.run_command return code) it will stay until you overwrite it again. but this method appears like you expect it to be set back to 0 after each func() see how it's used overall here: http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#1288 see how I use it for say desktop builds here (I try to not explicitly set it to something incase I have set it to a 'worse' level before that): http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#1485 apologies if I am still wrong. I won't block you landing this. However, I think I must be proven wrong or have this corrected before enabling any branches outside of ash.

Hi Jordan, This is the full patch with changes requested in comment #99 I am creating the inter diffs so it should be easier to review it.

Inter-diff #1 changes in configuration files: * removed redundant tokens in configuration: BRANCH, PLATFORM, ... * added update_channel in branch configuration (it was hardcoded in platform) * added update_channel in configuration_tokens

Minor changes in full patch. * updated application_ini for osx from Contents/MacOS/application.ini to Contents/Resources/application.ini * re-enable return statement in summarize()

Inter-diff #2 changes in summarize(): * added SUCCESS and FAILURE constants * summarize *should* be able to handle correctly failures/successes

Comment on attachment 8515080 [details] [diff] [review] [mozharness] bug 740142 mozharness deskotp repacks.patch r+ based on interdiffs awesome. I think that should work. Have you tested it somewhere? remind me how you are rolling this out so I know what this affects: i.e. what branches/builders, what other patches are needed, etc

Thanks for the review, Jordan. > awesome. I think that should work. Have you tested it somewhere? Yes, tested on linux, linux64 and macosx64 > remind me how you are rolling this out so I know what this affects: i.e. > what branches/builders, what other patches are needed, etc Desktop repacks are active only on ash, nothing else is affected. We need another buildbotcustom patch so the repacks are generated each time there's a successful ash-nightly build (they are on demand, now) When the repacks are generated periodically we can ask QA/QE to test the binaries generated by this script.

Comment on attachment 8515080 [details] [diff] [review] [mozharness] bug 740142 mozharness deskotp repacks.patch Landed with the following changes: in configs/single_locale/linux.py, removed redundant mock_packages line: 'gcc472_0moz1', 'gcc473_0moz1', 'yasm', 'ccache', # <-- from releng repo removed old configuration files (comment #95): configs/single_locale/mozilla-central_desktop.py configs/single_locale/mozilla-central_linux.py configs/single_locale/mozilla-central_linux64.py configs/single_locale/mozilla-central_macosx64.py configs/single_locale/mozilla-central_win32.py configs/single_locale/mozilla-central_win64.py removed mozharness/base/mar.py because the mar module is now in mozharness/mozilla/mar.py

Checked in code deployed to production

> Desktop repacks are active only on ash, nothing else is affected. > cool. btw I merged mainline mozharness with ash-mozharness today. I ran into a number of conflicts with desktop_l10n.py. I think I sorted it all out but when I diff hg.m.o/build/mozharness, I do get: comparing with http://hg.mozilla.org/build/mozharness searching for changes diff --git a/scripts/desktop_l10n.py b/scripts/desktop_l10n.py --- a/scripts/desktop_l10n.py +++ b/scripts/desktop_l10n.py @@ -451,30 +451,30 @@ class DesktopSingleLocale(LocalesMixin, def upload_repacks(self): """iterates through the list of locales and calls make upload""" self.summarize(self.make_upload, self.query_locales()) def summarize(self, func, items): """runs func for any item in items, calls the add_failure() for each error. It assumes that function returns 0 when successful. returns a two element tuple with (success_count, total_count)""" success_count = 0 - # total_count = len(items) + total_count = len(items) name = func.__name__ for item in items: result = func(item) if result == SUCCESS: # success! success_count += 1 else: # func failed... message = 'failure: %s(%s)' % (name, item) self._add_failure(item, message) - # return (success_count, total_count) + return (success_count, total_count) def _add_failure(self, locale, message, **kwargs): """marks current step as failed""" self.locales_property[locale] = "Failed" prop_key = "%s_failure" % locale prop_value = self.query_buildbot_property(prop_key) if prop_value: prop_value = "%s %s" % (prop_value, message) else: I am not sure if that was from my conflict resolving or a missing/wrong patch in ash-mozharness. Either way, something you may want to confirm is right/wrong

Hi Jordan, I think I've commented out the lines on ash by mistake. I've updated the ash-mozharness and re-tested it. Tests are looking good.

Think I spotted a (minor) bug.

Comment on attachment 8523132 [details] [diff] [review] bug740142_minor_bugfix.patch Good catch, Pete! Without your patch, calling _temp_mar_dir() will cause a "RuntimeError: maximum recursion depth exceeded" and since this never happens, the only explanation is that we are not using this method at all (https://mxr.mozilla.org/build agrees) r- only because we should delete the method instead of patching it.

Comment on attachment 8524530 [details] [diff] [review] bug740142_minor_bugfix_v2.patch thanks Pete!

Comment on attachment 8524530 [details] [diff] [review] bug740142_minor_bugfix_v2.patch https://hg.mozilla.org/build/mozharness/rev/c80c99866252

In production: https://hg.mozilla.org/build/mozharness/rev/c80c99866252

merged mainline mozharness into ash-mozharness again. looks like _temp_mar_dir()[1] exists on ash but not on mainline. I stuck with mainline so if you are expecting that method to be there, you'll need to back it back in! :) [1] http://hg.mozilla.org/build/ash-mozharness/rev/4ce22e2200ce#l15.81

(In reply to Jordan Lund (:jlund) from comment #116) > merged mainline mozharness into ash-mozharness again. > > looks like _temp_mar_dir()[1] exists on ash but not on mainline. I stuck > with mainline so if you are expecting that method to be there, you'll need > to back it back in! :) > > [1] http://hg.mozilla.org/build/ash-mozharness/rev/4ce22e2200ce#l15.81 Thanks Jordan, This method was removed in comments 110-115 above. Pete

Triggers chunked l10n desktop repacks after a successful build nightly

Clean up. When don't need this block anymore

The old patch did not apply anymore, here's an updated version.

Comment on attachment 8530319 [details] [diff] [review] [buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build.patch Review of attachment 8530319 [details] [diff] [review]: ----------------------------------------------------------------- phew! I made it. Okay, I think this is good. As I am new to the repack parts of all of this, I have *a lot* of questions before we can proceed. So bear with me, have a read through the review, and then I'm happy to meet and hash out all the details. :) ::: misc.py @@ +717,4 @@ > factory = factory_class( > scriptRepo=scriptRepo, > interpreter=mh_cfg.get('mozharness_python', pf.get('mozharness_python')), > + scriptName='scripts/fakebuild.py', This might break some builds ;) I'm guessing this was part of your testing and didn't intend to make it into patch @@ +1265,5 @@ > nightlyBuilders.append(builder) > # Fill the l10nNightly dict > + # trying to do repacks with mozharness > + if is_l10n_with_mh(config, platform): > + mh_l10n_update_branch_objects(branchObjects, config, name, so we are calling this here for just the l10n nightly builder names. We only need the names for the scheduler correct? If true, one thing I find confusing is that mh_l10n_update_branch_objects() has a side effect of calling mh_l10n_branch_objects(). mh_l10n_branch_objects actually creates the builder (+ factory). So essentially we call mh_l10n_branch_objects twice, once here for just the builder names and once (below) just for the builders + factories but the work for both is done both times. @@ +1474,5 @@ > 'localesURL', None) > )) > + else: > + # looping through l10n builders > + if config.get('desktop_mozharness_repacks_enabled', False) and \ taking a look at the above if to this else from http://mxr.mozilla.org/build/source/buildbotcustom/misc.py?rev=4a4350574867#1449 we have: if config['enable_l10n'] and config['enable_nightly'] and builder in l10nNightlyBuilders: I'm confused as to why the new mh repacks won't eval to true under that condition? I think config['enable_l10n'] is False for ash because ash never had enable_l10n set to True. But IIUC, this will blow up in a bad way once we switch on m-c or other l10n enabled branches @@ +1480,5 @@ > + for l10n_builder in l10nNightlyBuilders[builder]['l10n_builder']: > + nomergeBuilders.add(l10n_builder) > + triggerable = Triggerable(name=l10n_builder, > + builderNames=[l10n_builder]) > + branchObjects['schedulers'].append(triggerable) I think this will create 10 triggerable 'schedulers' per desktop nightly build. I don't think we need to do this. Instead, we can create one scheduler that schedules 10 builds. bhearsum, I think you might have touched on this. Am I correct or should we have a scheduler for each chunk? @@ +2157,5 @@ > + if is_l10n_with_mh(config, platform): > + mh_l10n_update_branch_objects(branchObjects, config, name, > + platform, secrets, is_nightly=True) > + > + elif config['enable_l10n']: again, same here. I think this will cause issues outside of ash. maybe we can avail of similar techniques from desktop mozharness. i.e. add the builder factory in generateDesktopMozharnessBuilders and then here in this patch do something like: if not builder_tracker['done_repack_build']: # create the non mh equivalent since it is not a mh repack enabled branch and/or platform @@ +2251,5 @@ > > # end do_nightly > > + # We still want l10n_dep builds if nightlies are off > + # no mozharness desktop repacks if nightlies are off so I understand, how come we are not replacing l10n_dep builders with mh? AFAICT we enable mh for the nightly l10n builders but not here. Can mh do these types of builds? @@ +2252,5 @@ > # end do_nightly > > + # We still want l10n_dep builds if nightlies are off > + # no mozharness desktop repacks if nightlies are off > + if config['enable_l10n'] and config['enable_l10n_onchange'] and \ again here with 'enable_l10n' @@ +3352,5 @@ > + repacks_enabled = config.get('desktop_mozharness_repacks_enabled', False) > + if repacks_enabled: > + # mozharness desktop repacks are enabled for this project_branch > + # now, let's check if repacks are enabled for this platform too > + if platform in config['mozharness_desktop_l10n_platforms']: I used to have something like this because I couldn't go off pf['mozharness_config'] due to the fact that would be true for things like b2g_builds and spider. But now that we have pf['mozharness_desktop_build'] and pf['mozharness_desktop_l10n'], I think, IIUC, we can remove the buildbot-config item mozharness_desktop_l10n_platforms and pivot off mozharness_desktop_l10n instead? @@ +3387,5 @@ > + use_credentials_file = repacks['use_credentials_file'] > + config_dir = 'single_locale' > + branch_config = os.path.join(config_dir, '%s.py' % branch) > + platform_config = os.path.join(config_dir, '%s.py' % platform) > + environment_config = os.path.join(config_dir, 'production.py') IIUC this will make it so we can not do staging builds without hacking here. can we use config.get('staging') ? @@ +3444,5 @@ > + > +def mh_l10n_builder_names(config, platform, is_nightly): > + # let's check if we need to create builders for this config/platform > + names = [] > + if not is_l10n_with_mh(config, platform): looks like we wrap is_l10n_with_mh() around mh_l10n_builder_names() already every time we call it. So, IIUC, the is_l10n_with_mh here isn't needed. If it's just a last failsafe, maybe we should throw an error here instead? @@ +3451,5 @@ > + > + pf = config['platforms'][platform] > + name = pf['base_name'] > + if is_nightly: > + name = '%s nightly' % (pf['base_name']) ash l10n builder names before this patch didn't include 'nightly' in the name. Is that because it was a mistake before and we want to have it in now?

Comment on attachment 8530319 [details] [diff] [review] [buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build.patch Review of attachment 8530319 [details] [diff] [review]: ----------------------------------------------------------------- have to do a r- for things like fakebuild.py. but I'd like to address all the questions/comments I had

> @@ +1480,5 @@ > > + for l10n_builder in l10nNightlyBuilders[builder]['l10n_builder']: > > + nomergeBuilders.add(l10n_builder) > > + triggerable = Triggerable(name=l10n_builder, > > + builderNames=[l10n_builder]) > > + branchObjects['schedulers'].append(triggerable) > > I think this will create 10 triggerable 'schedulers' per desktop nightly > build. I don't think we need to do this. Instead, we can create one > scheduler that schedules 10 builds. > > bhearsum, I think you might have touched on this. Am I correct or should we > have a scheduler for each chunk? > bhearsum ^

Comment on attachment 8530962 [details] [diff] [review] [buildbot-configs] Bug 740142 - configuration clean up II.patch Review of attachment 8530962 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/config.py @@ -2895,5 @@ > - continue > - # for all other branches delete mozharness_desktop_l10n > - for p in branch["platforms"]: > - if "mozharness_desktop_l10n" in p: > - del p["mozharness_desktop_l10n"] I touched on this in the buildbotcustom patch. I think we can leave pf['mozharness_desktop_l10n'] in for all platforms that's True and pivot off that instead of mozharness_desktop_l10n_platforms let's chat if that's not making sense

Thanks for the review, Jordan > This might break some builds ;) I'm guessing this was part of your testing > and didn't intend to make it into patch ops! > @@ +1265,5 @@ > > nightlyBuilders.append(builder) > > # Fill the l10nNightly dict > > + # trying to do repacks with mozharness > > + if is_l10n_with_mh(config, platform): > > + mh_l10n_update_branch_objects(branchObjects, config, name, > > so we are calling this here for just the l10n nightly builder names. We only > need the names for the scheduler correct? Right! Updated to use mh_l10n_builder_names & mh_l10n_scheduler_name, here. > If true, one thing I find confusing is that mh_l10n_update_branch_objects() > has a side effect of calling mh_l10n_branch_objects(). > mh_l10n_branch_objects actually creates the builder (+ factory). > > So essentially we call mh_l10n_branch_objects twice, once here for just the > builder names and once (below) just for the builders + factories but the > work for both is done both times. removed. > @@ +1474,5 @@ > > 'localesURL', None) > > )) > > + else: > > + # looping through l10n builders > > + if config.get('desktop_mozharness_repacks_enabled', False) and \ > taking a look at the above if to this else from > http://mxr.mozilla.org/build/source/buildbotcustom/misc. > py?rev=4a4350574867#1449 we have: > if config['enable_l10n'] and config['enable_nightly'] and builder in > l10nNightlyBuilders: > > > I'm confused as to why the new mh repacks won't eval to true under that > condition? I think config['enable_l10n'] is False for ash because ash never > had enable_l10n set to True. But IIUC, this will blow up in a bad way once > we switch on m-c or other l10n enabled branches > Moved config.get('desktop_mozharness_repacks_enabled', False) in the if branch and enable_l10n check into a elif branch > @@ +1480,5 @@ > > + for l10n_builder in l10nNightlyBuilders[builder]['l10n_builder']: > > + nomergeBuilders.add(l10n_builder) > > + triggerable = Triggerable(name=l10n_builder, > > + builderNames=[l10n_builder]) > > + branchObjects['schedulers'].append(triggerable) > > I think this will create 10 triggerable 'schedulers' per desktop nightly > build. I don't think we need to do this. Instead, we can create one > scheduler that schedules 10 builds. Fixed, creating a triggerable that starts "chunks" builders > bhearsum, I think you might have touched on this. Am I correct or should we > have a scheduler for each chunk? > > @@ +2157,5 @@ > > + if is_l10n_with_mh(config, platform): > > + mh_l10n_update_branch_objects(branchObjects, config, name, > > + platform, secrets, is_nightly=True) > > + > > + elif config['enable_l10n']: > > again, same here. I think this will cause issues outside of ash. removed the if is_l10n_with_mh() branch > maybe we can avail of similar techniques from desktop mozharness. > > i.e. add the builder factory in generateDesktopMozharnessBuilders and then > here in this patch do something like: > > if not builder_tracker['done_repack_build']: > # create the non mh equivalent since it is not a mh repack enabled > branch and/or platform Yes, using the builder_tracker simplifies the code, thank! > @@ +2251,5 @@ > > > > # end do_nightly > > > > + # We still want l10n_dep builds if nightlies are off > > + # no mozharness desktop repacks if nightlies are off > > so I understand, how come we are not replacing l10n_dep builders with mh? > AFAICT we enable mh for the nightly l10n builders but not here. Can mh do > these types of builds? No deps builds with mozharness for now. > @@ +2252,5 @@ > > # end do_nightly > > > > + # We still want l10n_dep builds if nightlies are off > > + # no mozharness desktop repacks if nightlies are off > > + if config['enable_l10n'] and config['enable_l10n_onchange'] and \ > > again here with 'enable_l10n' > > @@ +3352,5 @@ > > + repacks_enabled = config.get('desktop_mozharness_repacks_enabled', False) > > + if repacks_enabled: > > + # mozharness desktop repacks are enabled for this project_branch > > + # now, let's check if repacks are enabled for this platform too > > + if platform in config['mozharness_desktop_l10n_platforms']: > > I used to have something like this because I couldn't go off > pf['mozharness_config'] due to the fact that would be true for things like > b2g_builds and spider. > > But now that we have pf['mozharness_desktop_build'] and > pf['mozharness_desktop_l10n'], I think, IIUC, we can remove the > buildbot-config item mozharness_desktop_l10n_platforms and pivot off > mozharness_desktop_l10n instead? Updated. > @@ +3387,5 @@ > > + use_credentials_file = repacks['use_credentials_file'] > > + config_dir = 'single_locale' > > + branch_config = os.path.join(config_dir, '%s.py' % branch) > > + platform_config = os.path.join(config_dir, '%s.py' % platform) > > + environment_config = os.path.join(config_dir, 'production.py') > > IIUC this will make it so we can not do staging builds without hacking here. > > can we use config.get('staging') ? I was not aware of this! now balrog and environment configuration files depend on config.get('staging') > > @@ +3444,5 @@ > > + > > +def mh_l10n_builder_names(config, platform, is_nightly): > > + # let's check if we need to create builders for this config/platform > > + names = [] > > + if not is_l10n_with_mh(config, platform): > > looks like we wrap is_l10n_with_mh() around mh_l10n_builder_names() already > every time we call it. So, IIUC, the is_l10n_with_mh here isn't needed. If > it's just a last failsafe, maybe we should throw an error here instead? > removed all the occurrences of is_l10n_with_mh inside the mh_* functions. > @@ +3451,5 @@ > > + > > + pf = config['platforms'][platform] > > + name = pf['base_name'] > > + if is_nightly: > > + name = '%s nightly' % (pf['base_name']) > > ash l10n builder names before this patch didn't include 'nightly' in the > name. Is that because it was a mistake before and we want to have it in now? Added 'nightly' because it's a better description of the job. Updated patches for buildbotcustom and buildbot-configs will follow soon (+ interdiffs). Thanks again, Jordan!

Updated buildbotcustom patch based on comment #124

removed "desktop repacks with mozharness" block; added "capable" to mozharness_desktop_l10n. l10n repacks builders will be created if: * the platform is capable of doing repacks (linux, linux64, macosx64) AND * the repacks are enabled on a branch ('desktop_mozharness_repacks_enabled' = True)

Diffs between the last two buildbotcustom patches

(In reply to Jordan Lund (:jlund) from comment #123) > > @@ +1480,5 @@ > > > + for l10n_builder in l10nNightlyBuilders[builder]['l10n_builder']: > > > + nomergeBuilders.add(l10n_builder) > > > + triggerable = Triggerable(name=l10n_builder, > > > + builderNames=[l10n_builder]) > > > + branchObjects['schedulers'].append(triggerable) > > > > I think this will create 10 triggerable 'schedulers' per desktop nightly > > build. I don't think we need to do this. Instead, we can create one > > scheduler that schedules 10 builds. > > > > bhearsum, I think you might have touched on this. Am I correct or should we > > have a scheduler for each chunk? > > > > bhearsum ^ That's right - we should have one scheduler with _all_ of the builder names - not one scheduler per builder. Seems like this should be as simple as unindenting the block and passing l10nNightlyBuilders[builder]['l10n_builder'] as the builder names.

Comment on attachment 8532296 [details] [diff] [review] [buildbotcustom] interdiff.patch Review of attachment 8532296 [details] [diff] [review]: ----------------------------------------------------------------- ::: misc.py @@ +660,5 @@ > > + # Disable merging of nightly jobs > + if req1.properties.getProperty('nightly_build', False) or req2.properties.getProperty('nightly_build', False): > + log.msg("mergeRequests: nightly_build; returning False") > + return False this stuff isn't in your actual review and things like test_misc_merging.py. Is that for a different bug?

Comment on attachment 8532289 [details] [diff] [review] [buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build.patch Review of attachment 8532289 [details] [diff] [review]: ----------------------------------------------------------------- nice. looks great dude! r+ if you change config.get('staging', True) and answer my in line questions :D ::: misc.py @@ +1288,5 @@ > + else: > + # no repacks with mozharness, old style repacks > + if config['enable_l10n'] and platform in config['l10n_platforms']: > + l10nNightlyBuilders[builder] = {} > + l10nNightlyBuilders[builder]['tree'] = config['l10n_tree'] what does this 'tree' stuff get us for the non mozharness? is it old code? @@ +1472,5 @@ > + # looping through l10n builders > + > + if builder in l10nNightlyBuilders and \ > + 'l10n_repacks_with_mh' in l10nNightlyBuilders[builder]: > + # it's a repacks with mozharness this schedule creation block and the associated generation of l10n_builders is the hardest part to envision in this patch. It *looks* right but we should do some thorough testing. Have you been able to force build a nightly in staging and see the correct l10n builds being triggered? If not, I don't think that's a complete blocker but it certainly is before we expand beyond ash. @@ +1477,5 @@ > + l10n_builders = l10nNightlyBuilders[builder]['l10n_builder'] > + nomergeBuilders.update(l10n_builders) > + triggerable_name = l10nNightlyBuilders[builder]['scheduler_name'] > + triggerable = Triggerable(name=triggerable_name, > + builderNames=l10n_builders) so I understand, why do we use Triggerable over TriggerableL10n here? @@ +3358,5 @@ > return logUploadCmd > + > + > +def _is_l10n_enabled(config): > + """returns True if mozharness desktop repacks are enabled""" loves these methods. will make ripping out misc.py slightly less painful :) @@ +3379,5 @@ > + > + > +def is_l10n_with_mh(config, platform): > + """mozharness desktop repacks are enabled if they are active on the project > + and 'patform' is capable of creating repacks. s/patform/platform @@ +3383,5 @@ > + and 'patform' is capable of creating repacks. > + """ > + if not _is_l10n_enabled(config): > + return False > + return _is_l10n_capable(config, platform) it is probably safe to just do the following if you only care about truthy/falsey values and not boolean values themselves. return _is_l10n_enabled(config) and _is_l10n_capable(config, platform) @@ +3412,5 @@ > + branch_config = os.path.join(config_dir, '%s.py' % branch) > + platform_config = os.path.join(config_dir, '%s.py' % platform) > + environment_config = os.path.join(config_dir, 'production.py') > + balrog_config = os.path.join('balrog', 'production.py') > + if config.get('staging', True): 'staging' is only defined in staging_config.py. so because 'staging' does not exist in production_config.py, this will always eval to True. the default here should be False