The default bug view has changed. See this FAQ.

Move Firefox Desktop repacks to use mozharness

RESOLVED FIXED

Status

Release Engineering
Mozharness
P3
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: armenzg, Assigned: massimo)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [automation][mozharness])

Attachments

(8 attachments, 74 obsolete attachments)

191.76 KB, text/x-log
Details
4.44 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
rail
: review+
Details | Diff | Splinter Review
3.95 KB, patch
Details | Diff | Splinter Review
1.50 KB, patch
catlee
: review+
massimo
: checked-in+
Details | Diff | Splinter Review
15.05 KB, patch
jlund
: review+
Details | Diff | Splinter Review
438 bytes, patch
catlee
: review+
massimo
: checked-in+
Details | Diff | Splinter Review
1.06 KB, patch
rail
: review+
massimo
: checked-in+
Details | Diff | Splinter Review
"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

Comment 1

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

Updated

5 years ago
OS: Mac OS X → All
Priority: -- → P3
Assignee: nobody → jlund
Priority: P3 → --

Comment 2

5 years ago
"""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 :)

Comment 3

5 years ago
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.

Comment 4

5 years ago
Created attachment 612331 [details]
most recent log of mobile_l10n.py

Comment 5

5 years ago
(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?

Comment 6

5 years ago
(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?

Comment 7

5 years ago
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?

Comment 8

5 years ago
Created attachment 618884 [details] [diff] [review]
config diff

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 .

Comment 9

5 years ago
Created attachment 618885 [details] [diff] [review]
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.

Comment 10

5 years ago
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.
Assignee: jlund → nobody
Priority: -- → P3

Updated

5 years ago
Assignee: nobody → aki

Updated

5 years ago
Blocks: 700290

Updated

5 years ago
Depends on: 756594

Comment 11

5 years ago
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

Comment 12

5 years ago
(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.

Comment 13

4 years ago
This has been on my "tackle soon" list for a while now, but I'm not actively working on it.
Assignee: aki → nobody

Updated

4 years ago
No longer blocks: 700290

Updated

4 years ago
Assignee: nobody → mgervasini
One random note: when doing Windows development/testing, it should be done against 64-bit Windows slaves. 32-bit ones are deprecated.

Updated

4 years ago
Blocks: 882528
Duplicate of this bug: 799915
Product: mozilla.org → Release Engineering
(Assignee)

Comment 16

4 years ago
Created attachment 797879 [details] [diff] [review]
desktop-repacks.patch

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.
Attachment #797879 - Flags: feedback?(aki)
(Assignee)

Comment 17

4 years ago
Created attachment 798827 [details] [diff] [review]
desktop-repacks.patch

Removed from helpers some functions that are already present in ScriptMixin
Attachment #797879 - Attachment is obsolete: true
Attachment #797879 - Flags: feedback?(aki)
(Assignee)

Updated

4 years ago
Attachment #798827 - Flags: feedback?(aki)

Comment 18

4 years ago
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, ?
Attachment #798827 - Flags: feedback?(aki) → feedback+
Created attachment 802339 [details] [diff] [review]
buildbotcustom

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
Attachment #802339 - Flags: feedback?(aki)
Created attachment 802340 [details] [diff] [review]
configs
Attachment #802340 - Flags: feedback?(aki)

Comment 21

4 years ago
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.
Attachment #802339 - Flags: feedback?(aki) → feedback+

Updated

4 years ago
Attachment #802340 - Flags: feedback?(aki) → feedback+
(Assignee)

Comment 22

4 years ago
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
Created attachment 818505 [details] [diff] [review]
configs

updated configs, carry over f+aki
Attachment #802340 - Attachment is obsolete: true
Attachment #818505 - Flags: feedback+
Created attachment 818594 [details] [diff] [review]
buildbotcustom

refresh, f+aki
Attachment #802339 - Attachment is obsolete: true
Attachment #818594 - Flags: feedback+
Created attachment 819001 [details] [diff] [review]
configs

syntax error fix, f+aki carry over
Attachment #818505 - Attachment is obsolete: true
Attachment #819001 - Flags: feedback+
(Assignee)

Comment 26

3 years ago
Created attachment 823475 [details] [diff] [review]
work in progess on destktop repacks

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
Attachment #798827 - Attachment is obsolete: true
Attachment #823475 - Flags: feedback?(aki)

Comment 27

3 years ago
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 28

3 years ago
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.
Attachment #823475 - Flags: feedback?(aki) → feedback+
(Assignee)

Comment 29

3 years ago
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
Attachment #823475 - Attachment is obsolete: true
Attachment #8341856 - Flags: feedback?(aki)

Comment 30

3 years ago
(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 32

3 years ago
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.
Attachment #8341856 - Flags: feedback?(aki) → feedback+
(Assignee)

Comment 33

3 years ago
Created attachment 8345935 [details] [diff] [review]
desktop-repacks-common.patch

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!
Attachment #8345935 - Flags: review?(aki)

Updated

3 years ago
Attachment #8345935 - Flags: review?(aki) → review+
(Assignee)

Updated

3 years ago
Attachment #8345935 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8341856 - Flags: checked-in+
(Assignee)

Comment 34

3 years ago
Created attachment 8348800 [details] [diff] [review]
desktop-repacks.patch

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.
Attachment #8341856 - Attachment is obsolete: true
Attachment #8345935 - Attachment is obsolete: true
Attachment #8348800 - Flags: review?(aki)

Updated

3 years ago
Attachment #8348800 - Flags: review?(aki) → review+
Massimo: no update since last year. How's progress?
(Assignee)

Comment 36

3 years ago
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.

Updated

3 years ago
Depends on: 1006035

Updated

3 years ago
Duplicate of this bug: 642715

Updated

3 years ago
Duplicate of this bug: 700790
(Assignee)

Comment 39

3 years ago
Created attachment 8444946 [details] [diff] [review]
[mozharness] desktop repacks.patch

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.
Attachment #618885 - Attachment is obsolete: true
Attachment #8348800 - Attachment is obsolete: true
Attachment #8444946 - Flags: feedback?(aki)
(Assignee)

Comment 40

3 years ago
Created attachment 8444948 [details] [diff] [review]
[buildbotcustom] adding mozharness desktop repacks.patch

Hi rail,

I've updated your patch because it does not apply anymore :(
Attachment #818594 - Attachment is obsolete: true
Attachment #8444948 - Flags: feedback?(rail)
(Assignee)

Comment 41

3 years ago
Created attachment 8444950 [details] [diff] [review]
[buildbot-configs] updated config for desktop repacks.patch

... 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)
Attachment #819001 - Attachment is obsolete: true
Attachment #8444950 - Flags: feedback?(rail)
(Assignee)

Comment 42

3 years ago
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"]
       ....
Attachment #8444950 - Flags: feedback?(rail) → feedback-
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.
Attachment #8444948 - Flags: feedback?(rail) → feedback+

Comment 45

3 years ago
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.
Attachment #8444946 - Flags: feedback?(aki) → feedback+

Comment 46

3 years ago
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.
(Assignee)

Comment 47

3 years ago
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.

Updated

3 years ago
Component: General Automation → Mozharness
(Assignee)

Comment 48

3 years ago
Created attachment 8458206 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - configuration.patch

First patch for desktop repacks with mozharness, this patch includes only configuration changes
Attachment #8444946 - Attachment is obsolete: true
Attachment #8458206 - Flags: review?(jlund)
(Assignee)

Comment 49

3 years ago
Created attachment 8458209 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - mozharness libraries.patch

Second patch: changes in mozharness/ directory
Attachment #8458209 - Flags: review?(jlund)
(Assignee)

Comment 50

3 years ago
Created attachment 8458210 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - desktop repacks script.patch

Last one... the desktop_l10n.py script
Attachment #8458210 - Flags: review?(jlund)
(Assignee)

Comment 51

3 years ago
Created attachment 8458984 [details] [diff] [review]
[mozharness] Bug 740142 - mozharness libraries.patch

Updated patch including last changes made in Bug 1034925
Attachment #8458209 - Attachment is obsolete: true
Attachment #8458209 - Flags: review?(jlund)
(Assignee)

Updated

3 years ago
Attachment #8458984 - Flags: review?(jlund)
(Assignee)

Comment 52

3 years ago
Created attachment 8458988 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - desktop repacks script.patch

small update because of Bug 1034925 changes
Attachment #8458210 - Attachment is obsolete: true
Attachment #8458210 - Flags: review?(jlund)
Attachment #8458988 - Flags: review?(jlund)
(Assignee)

Comment 53

3 years ago
Created attachment 8458993 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - mozharness files.patch

I'm sorry, Jordan. I have uploaded a wrong patch file. Here's the correct one.
Attachment #8458984 - Attachment is obsolete: true
Attachment #8458984 - Flags: review?(jlund)
Attachment #8458993 - Flags: review?(jlund)
(Assignee)

Comment 54

3 years ago
Created attachment 8459053 [details] [diff] [review]
[buildbot-configs] Bug 740142 - enable mozharness desktop repacks on ash.patch

Moved from cedar to ash
Attachment #8444950 - Attachment is obsolete: true
Attachment #8459053 - Flags: review?(rail)
(Assignee)

Comment 55

3 years ago
Created attachment 8459054 [details] [diff] [review]
[buildbotcustom] Bug 740142 - enable mozharness desktop repacks.patch

...and buildbotcustom changes
Attachment #8444948 - Attachment is obsolete: true
Attachment #8459054 - Flags: review?(rail)
Attachment #8459053 - Flags: review?(rail) → review+
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.
Attachment #8459054 - Flags: review?(rail) → review+
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
Attachment #8458993 - Flags: review?(jlund) → review+
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)?
(Assignee)

Comment 62

3 years ago
Thanks for the reviews jlund! 

I have landed the code on ash-mozharness so next patches will be smaller
(Assignee)

Comment 63

3 years ago
Created attachment 8461183 [details] [diff] [review]
patch for mozharness common files

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
(Assignee)

Updated

3 years ago
Attachment #8461183 - Flags: review?(jlund)
(Assignee)

Updated

3 years ago
Attachment #8461183 - Flags: review?(jlund)
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.
Attachment #8458988 - Flags: review?(jlund) → review-

Updated

3 years ago
Attachment #8458206 - Flags: review?(jlund) → review-
(Assignee)

Comment 65

3 years ago
Created attachment 8462255 [details] [diff] [review]
ash-mozharness.patch

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
Attachment #8458206 - Attachment is obsolete: true
Attachment #8458988 - Attachment is obsolete: true
Attachment #8458993 - Attachment is obsolete: true
Attachment #8462255 - Flags: review?(jlund)
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 ;)
Attachment #8462255 - Flags: review?(jlund) → review+
(Assignee)

Comment 67

3 years ago
Thanks Jordan. Landed in ash-mozharness.
(Assignee)

Updated

3 years ago
Attachment #8459053 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8459054 - Flags: checked-in+
(Assignee)

Comment 68

3 years ago
Created attachment 8473323 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness.patch

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.
Attachment #8462255 - Attachment is obsolete: true
Attachment #8473323 - Flags: feedback?
Flags: needinfo?(jlund)
(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 :)
Flags: needinfo?(jlund)
(Assignee)

Comment 70

3 years ago
Created attachment 8474471 [details] [diff] [review]
[mozharness] Bug 740142 - Move Firefox Desktop repacks to use mozharness - a smaller patch

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
Attachment #8474471 - Flags: feedback+
(Assignee)

Comment 72

3 years ago
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?
Flags: needinfo?(catlee)
(Assignee)

Comment 73

3 years ago
Created attachment 8476742 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable nightlies on ash.patch

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
Attachment #8476742 - Flags: review?(catlee)
Flags: needinfo?(catlee)

Updated

3 years ago
Attachment #8476742 - Flags: review?(catlee) → review+
(Assignee)

Updated

3 years ago
Attachment #8476742 - Flags: checked-in+
Something here went into production :)
(Assignee)

Updated

3 years ago
Depends on: 1060310
(Assignee)

Comment 75

3 years ago
Created attachment 8482789 [details] [diff] [review]
[buildbot-configs] Bug 740142 - removed obsolete options for mozharness desktop repacks.patch

desktop repacks have a new configuration structure; removed

 'config': 'single_locale/... 

from project branches.
Attachment #8459053 - Attachment is obsolete: true
Attachment #8482789 - Flags: review?(jlund)
(Assignee)

Comment 76

3 years ago
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
Attachment #8459054 - Attachment is obsolete: true
Attachment #8482794 - Flags: review?(jlund)
(Assignee)

Comment 77

3 years ago
(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.
(Assignee)

Comment 78

3 years ago
Created attachment 8482848 [details] [diff] [review]
[mozharness] Bug 740142 - desktop repacks updated configuration management.patch

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)
Attachment #8473323 - Attachment is obsolete: true
Attachment #8474471 - Attachment is obsolete: true
Attachment #8473323 - Flags: feedback?
Attachment #8482848 - Flags: review?(jlund)

Updated

3 years ago
Attachment #8482789 - Flags: review?(jlund) → review+
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.
Attachment #8482794 - Flags: review?(jlund) → review+
> 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?
(Assignee)

Comment 83

3 years ago
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.
(Assignee)

Comment 84

3 years ago
Created attachment 8486479 [details] [diff] [review]
[mozharness] Bug 7404142 - desktop_l10n.py updates per comment 82.patch

Hey Jordan,

just a delta between the last patch and changes requested in comment 82.
Attachment #8486479 - Flags: feedback?(jlund)
> > @@ +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
Attachment #8486479 - Flags: feedback?(jlund) → feedback+
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
Attachment #8482848 - Flags: review?(jlund) → review-
(Assignee)

Updated

3 years ago
Attachment #8482789 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8482794 - Flags: checked-in+
(Assignee)

Comment 88

3 years ago
Created attachment 8495347 [details]
upload_log

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
(Assignee)

Comment 89

3 years ago
Created attachment 8495350 [details] [diff] [review]
[mozharness] Bug 740142 - added upload environment for production.patch

use stage.mozilla.org for uploads
Attachment #8495350 - Flags: review?(jlund)
(Assignee)

Comment 90

3 years ago
Created attachment 8495353 [details] [diff] [review]
[buildbotcustom] Bug 740142 - enable production configuration for desktop repacks.patch

Enable production configuration for desktop repacks.

Just as a note: repacks are enabled only on ash and they are on demand.
Attachment #8495353 - Flags: review?
(Assignee)

Updated

3 years ago
Attachment #8495353 - Flags: review? → review?(jlund)
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.

Updated

3 years ago
Attachment #8495353 - Flags: review?(jlund) → review+
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
Attachment #8495350 - Flags: review?(jlund) → review+
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.
(Assignee)

Updated

3 years ago
Attachment #8495350 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8495353 - Flags: checked-in+
Merged to production, and deployed.
(Assignee)

Comment 95

3 years ago
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.

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
Attachment #8461183 - Attachment is obsolete: true
Attachment #8482848 - Attachment is obsolete: true
Attachment #8486479 - Attachment is obsolete: true
Attachment #8495347 - Attachment is obsolete: true
Attachment #8495350 - Attachment is obsolete: true
Attachment #8501822 - Flags: review?(jlund)
(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 :)
Flags: needinfo?(mgervasini)
(Assignee)

Comment 98

3 years ago
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
Flags: needinfo?(mgervasini)
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.
Attachment #8501822 - Flags: review?(jlund) → review+
(Assignee)

Comment 100

2 years ago
Created attachment 8515067 [details] [diff] [review]
[mozharness] bug 740142 mozharness deskotp repacks.patch

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.
Attachment #8482789 - Attachment is obsolete: true
Attachment #8501822 - Attachment is obsolete: true
Attachment #8515067 - Flags: review?(jlund)
(Assignee)

Comment 101

2 years ago
Created attachment 8515072 [details] [diff] [review]
[mozharness] Bug 740142 - configuration changes, comment #99.patch

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
(Assignee)

Comment 102

2 years ago
Created attachment 8515080 [details] [diff] [review]
[mozharness] bug 740142 mozharness deskotp repacks.patch

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()
Attachment #8515067 - Attachment is obsolete: true
Attachment #8515067 - Flags: review?(jlund)
(Assignee)

Updated

2 years ago
Attachment #8515080 - Flags: review?(jlund)
(Assignee)

Comment 103

2 years ago
Created attachment 8515084 [details] [diff] [review]
[mozharness] Bug 740142 - summarize changes.patch

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
Attachment #8515080 - Flags: review?(jlund) → review+
(Assignee)

Comment 105

2 years ago
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.
(Assignee)

Comment 106

2 years ago
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
Attachment #8515080 - Flags: checked-in+
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
(Assignee)

Comment 109

2 years ago
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.
Created attachment 8523132 [details] [diff] [review]
bug740142_minor_bugfix.patch

Think I spotted a (minor) bug.
Attachment #8523132 - Flags: review?(mgervasini)
(Assignee)

Comment 111

2 years ago
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.
Attachment #8523132 - Flags: review?(mgervasini) → review-
Created attachment 8524530 [details] [diff] [review]
bug740142_minor_bugfix_v2.patch
Attachment #8523132 - Attachment is obsolete: true
Attachment #8524530 - Flags: review?(mgervasini)
(Assignee)

Comment 113

2 years ago
Comment on attachment 8524530 [details] [diff] [review]
bug740142_minor_bugfix_v2.patch

thanks Pete!
Attachment #8524530 - Flags: review?(mgervasini) → review+
Comment on attachment 8524530 [details] [diff] [review]
bug740142_minor_bugfix_v2.patch

https://hg.mozilla.org/build/mozharness/rev/c80c99866252
Attachment #8524530 - Flags: checked-in+
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
Blocks: 1083328
(Assignee)

Comment 118

2 years ago
Created attachment 8530319 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build.patch

Triggers chunked l10n desktop repacks after a successful build nightly
Attachment #8530319 - Flags: review?(jlund)
(Assignee)

Comment 119

2 years ago
Created attachment 8530320 [details] [diff] [review]
[buildbot-configs] Bug 740142 - configuration clean up.patch

Clean up. When don't need this block anymore
Attachment #8530320 - Flags: review?(jlund)
(Assignee)

Comment 120

2 years ago
Created attachment 8530962 [details] [diff] [review]
[buildbot-configs] Bug 740142 - configuration clean up II.patch

The old patch did not apply anymore, here's an updated version.
Attachment #8530320 - Attachment is obsolete: true
Attachment #8530320 - Flags: review?(jlund)
Attachment #8530962 - Flags: review?(jlund)
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
Attachment #8530319 - Flags: review?(jlund) → review-
> @@ +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 ^
Flags: needinfo?(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
(Assignee)

Comment 125

2 years ago
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!
(Assignee)

Comment 126

2 years ago
Created attachment 8532289 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build.patch

Updated buildbotcustom patch based on comment #124
Attachment #8530319 - Attachment is obsolete: true
Attachment #8532289 - Flags: review?(jlund)
(Assignee)

Comment 127

2 years ago
Created attachment 8532295 [details] [diff] [review]
[buildbot-configs] bug 740142 configuration clean up.patch

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)
Attachment #8530962 - Attachment is obsolete: true
Attachment #8530962 - Flags: review?(jlund)
Attachment #8532295 - Flags: review?(jlund)
(Assignee)

Comment 128

2 years ago
Created attachment 8532296 [details] [diff] [review]
[buildbotcustom] interdiff.patch

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.
Flags: needinfo?(bhearsum)
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
Attachment #8532289 - Flags: review?(jlund) → review+
Comment on attachment 8532295 [details] [diff] [review]
[buildbot-configs] bug 740142 configuration clean up.patch

Review of attachment 8532295 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/config.py
@@ -128,5 @@
>      # mozharness repacks will be enabled per branch
> -    'mozharness_desktop_l10n_platforms': [
> -        'linux', 'linux64', 'macosx64'
> -    ],
> -    # bug 1027890: excluding win32/win64 for now

boom! one less config item

@@ +160,4 @@
>                  'script_maxtime': int(5.5 * 3600),
>              },
>              'mozharness_desktop_l10n': {
> +                'capable': True,

I might adopt this strategy. Though it's an extra item, I think it helps readability

@@ -2886,5 @@
>  # END B2G's INBOUND
>  
> -# desktop repacks with mozharness
> -for name, branch in BRANCHES.items():
> -    if branch.get('desktop_mozharness_repacks_enabled'):

byebye block! we don't need your loops anymore ;)
Attachment #8532295 - Flags: review?(jlund) → review+
(Assignee)

Comment 133

2 years ago
Thanks Jordan!

> 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?

'tree' is set in buildbot-configs: BRANCHES['mozilla-central']['l10n_tree'] = 'fxcentral' and it is used by BaseRepackFactory.tinderboxPrintBuildInfo(), here http://hg.mozilla.org/build/buildbotcustom/file/5bcd7430ed32/process/factory.py#l3308.


> @@ +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?

Yes, repacks jobs are triggered after a successful nightly build (tested on dev-stage01)

> 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?
> 

mozharness desktop repacks are somehow similar to the mobile ones, so I have decided to use the same scheduler we already in use for the mobile world - In the future we might want to switch to TriggerableL10n, but it's easier for now if we use the Triggerable scheduler.

This reminds me, we might want to add a standalone repack builder to avoid to redo a full chunk if a single locale fails. I'll file a separate bug for this.

> @@ +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

Fixed.

> 
> 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)

yes, I'm stealing your solution :) 

> 
> @@ +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

Fixed.

A new patch + interdiff coming soon
(Assignee)

Comment 134

2 years ago
Created attachment 8534369 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build II.patch

Fix requested in comment #131
Attachment #8482794 - Attachment is obsolete: true
Attachment #8495353 - Attachment is obsolete: true
Attachment #8532289 - Attachment is obsolete: true
Attachment #8532296 - Attachment is obsolete: true
Attachment #8534369 - Flags: review?(jlund)
(Assignee)

Comment 135

2 years ago
Created attachment 8534370 [details] [diff] [review]
[buildbotcustom] interdiff.patch

Diff between last two buildbotcustom patches
Comment on attachment 8534369 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Enable l10n desktop repacks with mozharness after a nightly build II.patch

Review of attachment 8534369 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you s/stage/staging

::: misc.py
@@ +3397,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('stage', False):

you set the default correctly now but you 'staging' to 'stage' ?

what I'm hoping to pivot off is the staging key: http://mxr.mozilla.org/build/source/buildbot-configs/mozilla/staging_config.py?rev=2cc554bc3a33#18
Attachment #8534369 - Flags: review?(jlund) → review+
(Assignee)

Updated

2 years ago
Attachment #8532295 - Flags: checked-in+
(Assignee)

Updated

2 years ago
Attachment #8534369 - Flags: checked-in+
(Assignee)

Updated

2 years ago
Attachment #8534370 - Attachment is obsolete: true
In production: https://hg.mozilla.org/build/buildbot-configs/rev/953e898e9805
In production: https://hg.mozilla.org/build/buildbotcustom/rev/578500508491
(Assignee)

Comment 139

2 years ago
Created attachment 8537872 [details] [diff] [review]
[buildbot-configs] Bug 740142 - set l10n_chunks=1.patch

Repacks are using a lot of resources, let's set l10n_chunks=1 to lower the number of resources needed by ash l10n jobs.
Attachment #8537872 - Flags: review?(pmoore)
Comment on attachment 8537872 [details] [diff] [review]
[buildbot-configs] Bug 740142 - set l10n_chunks=1.patch

Review of attachment 8537872 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks mgerva!
Attachment #8537872 - Flags: review?(pmoore) → review+
(Assignee)

Updated

2 years ago
Attachment #8537872 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/34bb25dbee10
Duplicate of this bug: 608005
Duplicate of this bug: 644616
(Assignee)

Comment 144

2 years ago
Created attachment 8545298 [details] [diff] [review]
[mozharness] Bug 740142 - fixes for windows and partial updates.patch

Hi Kim,

I have been able to generate repacks for windows 32, using this script!
So it's probably time to have another review.

I am sorry, this patch is quite long. Let me try to describe the most important changes, so the review may be less painful:

changes in scripts/desktop_l10n.py
* _query_make_variable()
  + now logs the variable value

* _setup_configure()
  + runs tooltool, calls _run_tooltool() ported from Jordan's build script

* _current_mar_filename(), _previous_mar_filename(), ...
  + now require the local property

* generate_complete_mar()
  + now copies the complete mar file into the right place, when it's done

* repack_locale()
  + removes the contents of 'package_basedir' directory 

* added mach methods,
   + they are not used yet, but I plan to migrate the script to use mach configure instead of make configure as soon I fix the linux64 problem (probably there's something wrong with this configuration)

... and other minor fixes
  
Changes in configuration files:
* configs/single_locale/ash.py now points to mozilla central binaries - artifacts generated by this script are uploaded on ash directories so they cannot overwrite mozilla-central files - and then we can compare "classical" and mozharness repacks.

* config/single_locale/{macosx,linux*,win*}.py
  + added tooltool_configuration
  + updated previous_mar_dir and current_mar_dir
  + updated mock_packages on for linux* (same as packages we are using for builds)
  + fixed win* configuration, adding:
      L10NBASEDIR and MAKE_COMPLETE_MAR in the repack env
Attachment #8515072 - Attachment is obsolete: true
Attachment #8515080 - Attachment is obsolete: true
Attachment #8515084 - Attachment is obsolete: true
Attachment #8545298 - Flags: review?(kmoir)

Comment 145

2 years ago
Comment on attachment 8545298 [details] [diff] [review]
[mozharness] Bug 740142 - fixes for windows and partial updates.patch

yes this was a large patch :-) lgtm 
I might advise hold off on landing the mach stuff until they are actually used by that's a minor note
Attachment #8545298 - Flags: review?(kmoir) → review+
(Assignee)

Comment 146

2 years ago
Comment on attachment 8545298 [details] [diff] [review]
[mozharness] Bug 740142 - fixes for windows and partial updates.patch

Thanks Kim!

Landed without mach* methods
Attachment #8545298 - Flags: checked-in+
In production: https://hg.mozilla.org/build/mozharness/rev/5aee34548792
(Assignee)

Comment 148

2 years ago
Created attachment 8549024 [details] [diff] [review]
[mozharness] Bug 740142 - generate complete mars using in tree logic.patch

Hi Kim,

here's the updated version of the patch - Now it uses mach configure instead of make -f client.mk configure and it uses in tree complete mar generation.

changes in this patch:
* replaced make configure call with mach configure
* renamed repack_env => bootstrap_env
* added MOZ_MAKE_COMPLETE_MAR to force the in tree complete mar generation
other changes:
* added _requires_generate_mar() to check if partial/complete mar files have been created - if not they will be created by mozharness
* added latest_mar_dir in ash.py
* minor configuration changes
Attachment #8545298 - Attachment is obsolete: true
Attachment #8549024 - Flags: review?(kmoir)

Updated

2 years ago
Attachment #8549024 - Flags: review?(kmoir) → review+
(Assignee)

Comment 149

2 years ago
Comment on attachment 8549024 [details] [diff] [review]
[mozharness] Bug 740142 - generate complete mars using in tree logic.patch

Thanks Kim!
Attachment #8549024 - Flags: checked-in+
(Assignee)

Comment 150

2 years ago
Created attachment 8549843 [details] [diff] [review]
[mozharness] Bug 740142 - fix error in partial generation.patch

Hi Kim,

this patch fixes a problem when calling make_incremental_update.sh (wrong parameters order) and adds some extra clean up steps in generate_partial_updates()
Attachment #8549024 - Attachment is obsolete: true
Attachment #8549843 - Flags: review?(kmoir)

Updated

2 years ago
Attachment #8549843 - Flags: review?(kmoir) → review+
(Assignee)

Comment 151

2 years ago
Comment on attachment 8549843 [details] [diff] [review]
[mozharness] Bug 740142 - fix error in partial generation.patch

Thanks Kim
Attachment #8549843 - Flags: checked-in+
(Assignee)

Comment 152

2 years ago
Created attachment 8550481 [details] [diff] [review]
[mozharness] Bug 740142 - use mozmake.exe for windows.patch

Hi Kim,

new patch:
* switched win32/win64 to use mozmake.exe
* using "MOZ_MAKE_COMPLETE_MAR" instead of "MAKE_COMPLETE_MAR" in win*/macosx64 configuration
* removed some unnecessary clean up steps
* updated _requires_generate_mar() so it checks the right 'complete' file 
* updated 'binary_path' for windows
Attachment #8549843 - Attachment is obsolete: true
Attachment #8550481 - Flags: review?(kmoir)

Updated

2 years ago
Attachment #8550481 - Flags: review?(kmoir)
Attachment #8550481 - Flags: review+
In production: https://hg.mozilla.org/build/mozharness/rev/79ebf7c302b0
In production: https://hg.mozilla.org/build/mozharness/rev/8334eb960d29
Created attachment 8553996 [details] [diff] [review]
150123_disable_l10n_on_ash.patch

ash is being active at the moment and something is configured incorrectly with l10n builds :(

our savior, mgerva, is away until Feb so I propose we just disable l10n for ash pushes.

coop: builder_list diff ->

[dev_master]jlund@Hastings163:~/tmp
> diff -pU 20 $BUILD_CLEAN_BUILDER_LIST $BUILD_DIRTY_BUILDER_LIST
--- /Users/jlund/devel/mozilla/dev_master/build-builderlists/2015-01-23--14:42:21--clean        2015-01-23 22:42:35.000000000 +0000
+++ /Users/jlund/devel/mozilla/dev_master/build-builderlists/2015-01-23--14:42:21--dirty        2015-01-23 22:43:10.000000000 +0000
@@ -1,64 +1,50 @@
 Android armv7 API 11+ ash build NightlyBuildFactory
-Android armv7 API 11+ ash l10n nightly-1 SigningScriptFactory
-Android armv7 API 11+ ash l10n nightly-2 SigningScriptFactory
-Android armv7 API 11+ ash l10n nightly-3 SigningScriptFactory
-Android armv7 API 11+ ash l10n nightly-4 SigningScriptFactory
-Android armv7 API 11+ ash l10n nightly-5 SigningScriptFactory
 Android armv7 API 11+ ash nightly NightlyBuildFactory
 Android armv7 API 11+ ash debug build NightlyBuildFactory
 Android armv7 API 9 ash build NightlyBuildFactory
-Android armv7 API 9 ash l10n nightly-1 SigningScriptFactory
-Android armv7 API 9 ash l10n nightly-2 SigningScriptFactory
-Android armv7 API 9 ash l10n nightly-3 SigningScriptFactory
-Android armv7 API 9 ash l10n nightly-4 SigningScriptFactory
-Android armv7 API 9 ash l10n nightly-5 SigningScriptFactory
 Android armv7 API 9 ash nightly NightlyBuildFactory
 Android armv7 API 9 ash debug build NightlyBuildFactory
 Android 4.2 x86 ash build NightlyBuildFactory
 Android 4.2 x86 ash nightly NightlyBuildFactory
 Linux ash build SigningScriptFactory
 Linux ash nightly SigningScriptFactory
-Linux ash nightly l10n 1/1 SigningScriptFactory
 Linux ash pgo-build SigningScriptFactory
 Linux ash leak test build ScriptFactory
 Linux x86-64 ash build SigningScriptFactory
 Linux x86-64 ash nightly SigningScriptFactory
-Linux x86-64 ash nightly l10n 1/1 SigningScriptFactory
 Linux x86-64 ash pgo-build SigningScriptFactory
 Linux x86-64 ash valgrind ScriptFactory
 Linux x86-64 ash asan build ScriptFactory
 Linux x86-64 ash asan nightly SigningScriptFactory
 Linux x86-64 ash debug asan build ScriptFactory
 Linux x86-64 ash debug asan nightly SigningScriptFactory
 linux64-br-haz_ash_dep ScriptFactory
 Linux x86-64 ash leak test build ScriptFactory
 Linux x86-64 ash debug static analysis build ScriptFactory
 OS X 10.7 ash build SigningScriptFactory
 OS X 10.7 ash nightly SigningScriptFactory
-OS X 10.7 ash nightly l10n 1/1 SigningScriptFactory
 OS X 10.7 64-bit ash leak test build SigningScriptFactory
 OS X 10.7 64-bit ash debug static analysis build ScriptFactory
 WINNT 5.2 ash build SigningScriptFactory
 WINNT 5.2 ash nightly SigningScriptFactory
 WINNT 5.2 ash pgo-build SigningScriptFactory
-Firefox ash win32 l10n nightly NightlyRepackFactory
 WINNT 5.2 ash leak test build SigningScriptFactory
 WINNT 6.1 x86-64 ash build NightlyBuildFactory
 WINNT 6.1 x86-64 ash pgo-build NightlyBuildFactory
 WINNT 6.1 x86-64 ash nightly NightlyBuildFactory
 WINNT 6.1 x86-64 ash leak test build NightlyBuildFactory
 Android armv7 API 11+ jamun build NightlyBuildFactory
 Android armv7 API 11+ jamun debug build NightlyBuildFactory
 Android armv7 API 9 jamun build NightlyBuildFactory
 Android armv7 API 9 jamun debug build NightlyBuildFactory
 Android 4.2 x86 jamun build NightlyBuildFactory
 Linux jamun build SigningScriptFactory
 Linux jamun leak test build ScriptFactory
 Linux x86-64 jamun build SigningScriptFactory
 Linux x86-64 jamun valgrind ScriptFactory
 Linux x86-64 jamun asan build ScriptFactory
 Linux x86-64 jamun debug asan build ScriptFactory
 linux64-br-haz_jamun_dep ScriptFactory
 Linux x86-64 jamun leak test build ScriptFactory
 Linux x86-64 jamun debug static analysis build ScriptFactory
 OS X 10.7 jamun build SigningScriptFactory





mgerva, here is what postrun.py is not happy about ftr: 

 1 Running [u'/builds/buildbot/build1/bin/python', u'/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/postrun.py', u'-c', u'/builds/buildbot/build1/master/    postrun.cfg', u'--master-name', u'buildbot-master94.srv.releng.use1.mozilla.com:/builds/buildbot/build1/master', u'--master-incarnation', u'pid18096-boot1421346935', u'/build    s/buildbot/build1/master/Linux-x86-64-ash-l10n-1_1/0', u'60020473']
  2 2015-01-23 10:35:00,819 - Loading build pickle
  3 2015-01-23 10:35:01,362 - Build info: {'platform': 'linux64', 'product': 'firefox', 'branch': 'Linux x86-64 ash'}
  4 2015-01-23 10:35:01,362 - uploading log
  5 2015-01-23 10:35:01,362 - Build info: {'platform': 'linux64', 'product': 'firefox', 'branch': 'Linux x86-64 ash'}
  6 2015-01-23 10:35:01,363 - Build info: {'platform': 'linux64', 'product': 'firefox', 'branch': 'Linux x86-64 ash'}
  7 2015-01-23 10:35:01,363 - Running ['/builds/buildbot/build1/bin/python', '/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/log_uploader.py', '-r', '2',     '-t', '10', '--master-name', u'bm94-build1', '--l10n', '--product', 'firefox', '--platform', 'linux64', '--branch', 'Linux x86-64 ash', '--user', u'ffxbld', '-i', u'/home/clt    bld/.ssh/ffxbld_dsa', u'stage.mozilla.org', '/builds/buildbot/build1/master/Linux-x86-64-ash-l10n-1_1', '0']
  8 2015-01-23 10:35:01,363 - command: START
  9 2015-01-23 10:35:01,363 - command: /builds/buildbot/build1/bin/python /builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/log_uploader.py -r 2 -t 10 --mast    er-name bm94-build1 --l10n --product firefox --platform linux64 --branch "Linux x86-64 ash" --user ffxbld -i /home/cltbld/.ssh/ffxbld_dsa stage.mozilla.org /builds/buildbot/b    uild1/master/Linux-x86-64-ash-l10n-1_1 0
 10 2015-01-23 10:35:01,363 - command: stdin: <open file '/dev/null', mode 'r' at 0x2350ae0>
 11 2015-01-23 10:35:01,363 - command: cwd: /
 12 Traceback (most recent call last):
 13   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/log_uploader.py", line 363, in <module>
 14     print ssh(user=options.user, identity=options.identity, host=host, remote_cmd=post_upload_cmd)
 15   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/log_uploader.py", line 46, in ssh
 16     return retry(do_cmd, attempts=retries + 1, sleeptime=retry_sleep, args=(cmd,))
 17   File "/builds/buildbot/build1/tools/lib/python/util/retry.py", line 33, in retry
 18     return action(*args, **kwargs)
 19   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/log_uploader.py", line 37, in do_cmd
 20     cmd, retcode, output))
 21 Exception: Command ['ssh', '-l', 'ffxbld', '-i', '/home/cltbld/.ssh/ffxbld_dsa', '-p', '22', 'stage.mozilla.org', u'post_upload.py --tinderbox-builds-dir Linux x86-64 ash-l10    n -b Linux x86-64 ash-l10n -p firefox -i 20150123040229 --revision 7aad55d51bad --release-to-tinderbox-builds /tmp/tmp.WDhqHj0ehr /tmp/tmp.WDhqHj0ehr/Linux-x86-64-ash-l10n-1_    1-unknown-bm94-build1-build0.txt.gz'] returned non-zero exit code 1:
 22 sys.argv: ['/usr/local/bin/post_upload.py', '--tinderbox-builds-dir', 'Linux', 'x86-64', 'ash-l10n', '-b', 'Linux', 'x86-64', 'ash-l10n', '-p', 'firefox', '-i', '201501230402    29', '--revision', '7aad55d51bad', '--release-to-tinderbox-builds', '/tmp/tmp.WDhqHj0ehr', '/tmp/tmp.WDhqHj0ehr/Linux-x86-64-ash-l10n-1_1-unknown-bm94-build1-build0.txt.gz']
 23 Error, /data/home/ffxbld/x86-64 is not a directory!
 24 2015-01-23 10:35:45,385 - command: END (44.02 elapsed)
 25
 26 Traceback (most recent call last):
 27   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/postrun.py", line 364, in <module>
 28     main()
 29   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/postrun.py", line 361, in main
 30     post_runner.processBuild(options, build_path, request_ids)
 31   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/postrun.py", line 291, in processBuild
 32     log_url = self.uploadLog(build)
 33   File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbotcustom/bin/postrun.py", line 99, in uploadLog
 34     output = get_output(cmd, stdin=devnull)
 35   File "/builds/buildbot/build1/tools/lib/python/util/commands.py", line 160, in get_output
 36     raise e
Attachment #8553996 - Flags: review?(coop)

Updated

2 years ago
Attachment #8553996 - Flags: review?(coop) → review+
Comment on attachment 8553996 [details] [diff] [review]
150123_disable_l10n_on_ash.patch

this was taken care of while resolving reconfig conflict here: https://bugzilla.mozilla.org/show_bug.cgi?id=1110286#c19
Attachment #8553996 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8550481 - Flags: checked-in+
Looks like maybe the problem is in the 'branch':
{'platform': 'linux64', 'product': 'firefox', 'branch': 'Linux x86-64 ash'}

'Linux x86-64 ash' should probably be 'ash'
In production: https://hg.mozilla.org/build/mozharness/rev/8d3c6f24781a
(Assignee)

Comment 159

2 years ago
Created attachment 8560024 [details] [diff] [review]
[mozharness] Bug 740142 - updated clobber method; updated current_mar_filename for win64, added cleanup action.patch

Hi Jordan,

changes in this version:

* support for win64 (same result as win32)
* added ssh_key_dir in platform; (under mock ~/.ssh/ expands to /build/.ssh, so linux* cannot find the ssh keys) 
* added support for runner. The build directory gets renamed if there enough space (runner will take care of removing it on reboot) - this saves ~7 mins build time on windows platforms. Since runner is not ready yet, I have added new action "cleanup", which takes care of removing the renamed directory at the end of the job
* to support runner, I have introduced few new methods in mozharness/base/script.py:
   + disk_info: reports some stats about disk usage
   + delegate_deletion_to_runner: deletes or renames the build directory
I am not totally sure mozharness/base/script.py is the right place for delegate_deletion_to_runner (and its support methods) maybe mozharness/mozilla/runner.py is a better place for them. Any suggestions?
Attachment #8550481 - Attachment is obsolete: true
Attachment #8560024 - Flags: review?(jlund)
Blocks: 1012865
Comment on attachment 8560024 [details] [diff] [review]
[mozharness] Bug 740142 - updated clobber method; updated current_mar_filename for win64, added cleanup action.patch

Review of attachment 8560024 [details] [diff] [review]:
-----------------------------------------------------------------

this is neat. :)

So, IIUC, we are trying to save purge from clobbering 1 buildir: abs_work_dir, and getting runner to do it later. This saves build time if we don't actually need the space for the build right? How is this going to tie in with PurgeMixin. My understanding is its clobber will go through many builddirs and start clobbering them if they are old or we need disk space (I think PurgeMixin.clobber() uses c['build_space'] And what if abs_work_dir doesn't really have much contents or is empty on the slave, this won't save us anything correct?

Thanks for putting up with me as I get up to speed with all this :)

As to the location of where to put the runner stuff, I agree that base/script.py is probably not optimal. That file is not only for non mozilla stuff but also should be pretty stripped down from anything specific. self.disk_info() looks insanely useful so we should keep that in script.py but your suggestion of having a runner.py makes sense to me :D

going to r- for at least needing to rip stuff out of script.py

::: mozharness/base/script.py
@@ +319,4 @@
>              return -1
>          return 0
>  
> +    def delegate_deletion_to_runner(self, dirname, log_level=INFO,

this method will always clobber dirname whether it does it itself or else it will move the dirname and tell runner to do it. I just want to sanity check that that logic is what we want. i.e. my concern is our current behaviour in purge's clobber() is to not nuke dirname (abs_work_dir) unless we are doing a nightly or we configure to do so: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/purge.py#128

maybe desktop_l10n builds always clobber as maybe we don't reuse src or other tools in the build_dir, but I'm not sure we want this default behaviour from other builders (that is if we are extending this method).

@@ +332,5 @@
> +            self.info('Nothing to do, %s does not exist' % dirname)
> +            return 0
> +        config = self.config
> +        # required_disk_space in Gb
> +        if 'required_disk_space' not in config:

I think your try/catch below covers this condition block

@@ +343,5 @@
> +        except (KeyError, ValueError):
> +            self.info("Cannot delegate deletion to runner")
> +            self.info("required_disk_space is not set or it is not an integer")
> +            self.info("calling rmtree instead")
> +            return self.rmtree(dirname, log_level=log_level,

hmm, so I understand, if this is an osx or windows run, we will do a full clobber of the work dir every time?

maybe instead we just log like you are and return early? Or add a force_on_error param to delegate_deletion_to_runner

@@ +348,5 @@
> +                               error_level=error_level, exit_code=exit_code)
> +
> +        di = self.disk_info(dirname)
> +        free_gb = di['free'] / 1024 / 1024 / 1024
> +        if required_disk_space > free_gb:

you have it so disk_info() can return 0s for di.values() for at least the unknown platform situation. we should check if disk_info gave us actual real values

@@ +367,5 @@
> +           otherwise mv becomes more expensive than deleting the directory in
> +           the first place (it will be moved file by file and than runner will
> +           eventually delete it)"""
> +        dirs = self.query_abs_dirs()
> +        return dirs['base_work_dir']

do you plan on _get_base_tempdir_for_runner to every not be dirs['base_work_dir']

@@ +394,5 @@
> +        except AttributeError:
> +            # stagvfs is available only under Unix
> +            pass
> +
> +        if os.name == 'nt':

will if self._is_windows work? http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#122

@@ +395,5 @@
> +            # stagvfs is available only under Unix
> +            pass
> +
> +        if os.name == 'nt':
> +            # we're on windows

sh** just got meta. is there really no high level module that can do this for us?

@@ +396,5 @@
> +            pass
> +
> +        if os.name == 'nt':
> +            # we're on windows
> +            import ctypes

pep 8 nit: I understand this is an edge case but I don't think it's that expensive to keep all imports at the top of the file.

@@ +416,5 @@
> +                d_info['total'] = total.value
> +                d_info['used'] = total.value - free.value
> +                d_info['free'] = free.value
> +        else:
> +            # not a windows or posix machine

maybe log something here?

::: scripts/desktop_l10n.py
@@ +516,5 @@
>          config = self.config
> +        # Try to rename the directory build directory instead of deleting it.
> +        # The renamed directory will be removed by runner (when it's ready)
> +        # runner will also make 'cleanup' step redundant so remove the cleanup
> +        # action annd its associated method

so, IIUC, once runner does this clean up for us, you will delete cleanup() action?
Attachment #8560024 - Attachment filename: 740142 - updated clobber method; updated current_mar_filename for win64, added cleanup action.patch → 740142 - updated clobber method; updated current_mar_filename for win64, added cleanup action.patch
Attachment #8560024 - Flags: review?(jlund) → review-
(Assignee)

Comment 161

2 years ago
Thanks Jordan,

(In reply to Jordan Lund (:jlund) from comment #160)
> Comment on attachment 8560024 [details] [diff] [review]
> [mozharness] Bug 740142 - updated clobber method; updated
> current_mar_filename for win64, added cleanup action.patch
> 
> Review of attachment 8560024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is neat. :)
> 
> So, IIUC, we are trying to save purge from clobbering 1 buildir:
> abs_work_dir, and getting runner to do it later. This saves build time if we
> don't actually need the space for the build right?How is this going to tie
> in with PurgeMixin. My understanding is its clobber will go through many
> builddirs and start clobbering them if they are old or we need disk space (I
> think PurgeMixin.clobber() uses c['build_space'] And what if abs_work_dir
> doesn't really have much contents or is empty on the slave, this won't save
> us anything correct?

Using abs_work_dir is l10n desktop repacks specific, other types of builds may use different directories. 
After the delegate_deletion_to_runner call, we run PurgeMixin.clobber() anyway to remove whatever is left (oauth.txt, logs, ...)
It makes no sense to wait for runner to delete a small directory, we should use this function only on large directories.


> ::: mozharness/base/script.py
> @@ +319,4 @@
> >              return -1
> >          return 0
> >  
> > +    def delegate_deletion_to_runner(self, dirname, log_level=INFO,
> 
> this method will always clobber dirname whether it does it itself or else it
> will move the dirname and tell runner to do it. I just want to sanity check
> that that logic is what we want. i.e. my concern is our current behaviour in
> purge's clobber() is to not nuke dirname (abs_work_dir) unless we are doing
> a nightly or we configure to do so:
> http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/purge.
> py#128
> maybe desktop_l10n builds always clobber as maybe we don't reuse src or
> other tools in the build_dir, but I'm not sure we want this default
> behaviour from other builders (that is if we are extending this method).

Yes it the final script that should decide what must be deleted, and so it must pass the right directories to delegate_deletion_to_runner.

I've created two different bugs to address all the requested changes:
 
 * Bug 1130336 - Create a function to determine: total, used and free disk space
 * Bug 1130340 - Create a RunnerMixin to track the changes needed

> ::: scripts/desktop_l10n.py
> @@ +516,5 @@
> >          config = self.config
> > +        # Try to rename the directory build directory instead of deleting it.
> > +        # The renamed directory will be removed by runner (when it's ready)
> > +        # runner will also make 'cleanup' step redundant so remove the cleanup
> > +        # action annd its associated method
> 
> so, IIUC, once runner does this clean up for us, you will delete cleanup()
> action?

Yes, when runner is ready, we must remove the cleanup action and the cleanup method
(Assignee)

Comment 162

2 years ago
Created attachment 8562260 [details] [diff] [review]
[buildbotcustom] Bug 740142 - wrong branch property in desktop repacks.patch

Fixed wrong branch name as suggested in Comment 157
Attachment #8534369 - Attachment is obsolete: true
Attachment #8562260 - Flags: review?(jlund)
(Assignee)

Comment 163

2 years ago
Created attachment 8562279 [details] [diff] [review]
[buildbot-configs] Bug 740142 - re-enable l10n with mozharness on ash; set l10n_chunks = 3.patch

changes:
* re-enabled l10n with mozharness on ash
* set l10n_chunks to 3
Attachment #8532295 - Attachment is obsolete: true
Attachment #8537872 - Attachment is obsolete: true
Attachment #8562279 - Flags: review?(jlund)
Blocks: 1125433
Blocks: 1077598

Updated

2 years ago
Attachment #8562279 - Flags: review?(jlund) → review+

Updated

2 years ago
Attachment #8562260 - Flags: review?(jlund) → review+
(Assignee)

Updated

2 years ago
Attachment #8562279 - Flags: checked-in+
(Assignee)

Comment 164

2 years ago
Comment on attachment 8562260 [details] [diff] [review]
[buildbotcustom] Bug 740142 - wrong branch property in desktop repacks.patch

Thanks Jordan
Attachment #8562260 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/e6b46a64ad0a
In production: https://hg.mozilla.org/build/buildbotcustom/rev/a11ef8db7d42
(Assignee)

Comment 167

2 years ago
Created attachment 8565982 [details] [diff] [review]
[mozharness] Bug 740142 - Added ssh_key_dir, fixed current_mar_filename for win64; removed gapi.data for linux*.patch

Changes in this patch:

* added ssh_key_dir (~/.ssh expands to /builds/.ssh in our mock environment)
* fixed current_mar_filename for windows 64
* removed gapi.data from mock files, we don't need it
Attachment #8560024 - Attachment is obsolete: true
Attachment #8565982 - Flags: review?(mshal)
Comment on attachment 8565982 [details] [diff] [review]
[mozharness] Bug 740142 - Added ssh_key_dir, fixed current_mar_filename for win64; removed gapi.data for linux*.patch

>-        ('/builds/gapi.data', '/builds/gapi.data'),

Can you double-check that this file isn't needed for repacks? I don't really know how they work, but I see /builds/gapi.data is referenced via mozconfigs.


>-    "current_mar_filename": "firefox-%(version)s.%(locale)s.win64.complete.mar",
>+    "current_mar_filename": "firefox-%(version)s.%(locale)s.win64-x86_64.complete.mar",

Isn't win64.complete.mar the correct filename given bug 1128953?
Attachment #8565982 - Flags: review?(mshal) → feedback+
(Assignee)

Comment 169

2 years ago
Thanks mshal!
> win64; removed gapi.data for linux*.patch
> 
> >-        ('/builds/gapi.data', '/builds/gapi.data'),
> 
> Can you double-check that this file isn't needed for repacks? I don't really
> know how they work, but I see /builds/gapi.data is referenced via mozconfigs.

Yes, let's keep the gapi.data there, I'll find if we need it for repacks
 
> 
> >-    "current_mar_filename": "firefox-%(version)s.%(locale)s.win64.complete.mar",
> >+    "current_mar_filename": "firefox-%(version)s.%(locale)s.win64-x86_64.complete.mar",
> 
> Isn't win64.complete.mar the correct filename given bug 1128953?

Yes, you're right! My staging environment is still pointing to old files.
(Assignee)

Comment 170

2 years ago
Created attachment 8566013 [details] [diff] [review]
[mozharness] - Bug 7410142 - add ssh_key_dir.patch

Added "ssh_key_dir" in configuration
Attachment #8565982 - Attachment is obsolete: true
Attachment #8566013 - Flags: review?(mshal)
Comment on attachment 8566013 [details] [diff] [review]
[mozharness] - Bug 7410142 - add ssh_key_dir.patch

>+        # ssh_key_dir is defined per platform: it is "~" for every platform

nit: It looks like it is really "~/.ssh", not just "~"

>+        # ssh_key_dir is defined per platform: it is "~" for every platform

Same here
Attachment #8566013 - Flags: review?(mshal) → review+
(Assignee)

Comment 172

2 years ago
Comment on attachment 8566013 [details] [diff] [review]
[mozharness] - Bug 7410142 - add ssh_key_dir.patch

Thanks mshal!
Attachment #8566013 - Flags: checked-in+
(Assignee)

Comment 173

2 years ago
Created attachment 8566548 [details] [diff] [review]
[mozharness] Bug 740142 - Test a fix for relative paths in make installers.patch

Repacks are enabled on ash nightlies but they are failing with this error:

..//builds/slave/Linux-x86-64-ash-l10n-2_3-0000/build/ash/obj-l10n/dist/update//previous.mar: No such file or directory

make installers fails because it cannot find the previous mar file (just downloaded).

We already have for fix on this on ash but to test it, we need to run the repacks with some minor changes:

* ash configuration needs to point to https://hg.mozilla.org/projects/ash
* we need to use the head of the ash tree (default revision)

This fix is just temporary and it's only needed to check that the makefile changes  work properly before porting them to mozilla-central.
Attachment #8566548 - Flags: review?(mshal)
Can't wait put these changes into your mozharness user repo and point ash/testing/mozharness/mozharness.json to that?
Comment on attachment 8566548 [details] [diff] [review]
[mozharness] Bug 740142 - Test a fix for relative paths in make installers.patch

Unflagging review for now per #c174.

(Also, "can't wait" instead of "can't we"? Sigh...)
Attachment #8566548 - Flags: review?(mshal)
(Assignee)

Comment 176

2 years ago
(In reply to Michael Shal [:mshal] from comment #174)
> Can't wait put these changes into your mozharness user repo and point
> ash/testing/mozharness/mozharness.json to that?

I've made a test; I have pointed ash/testing/mozharness/mozharness.json to my mozharness repo and it works fine for desktop builds; desktop repacks instead are still using the mozilla repositories. 

I think the fastest way to test the makefiles for the relative patch issue, is still the one in comment 173
Comment on attachment 8566548 [details] [diff] [review]
[mozharness] Bug 740142 - Test a fix for relative paths in make installers.patch

>+        self.info(" testing a fix for relative path in make installers, hardcoding revision to default")
>+        revision = default

Won't you need quotes around 'default'? Also, is there a way to only do this for ash?
Attachment #8566548 - Flags: feedback+
(Assignee)

Comment 178

2 years ago
Created attachment 8567922 [details] [diff] [review]
[mozharness] Bug 740142 - Test a fix for relative paths in make installers II.patch

Thanks mshal!

Yes, "default" should be quoted, we need a string there.

Unfortunately, I haven't found any better way to test it on ash, but even with "default" revision, we will not generate bad artifacts for the final users. (binaries created with this script will live in the "latest-ash" directory)
Attachment #8566548 - Attachment is obsolete: true
Attachment #8567922 - Flags: review?(mshal)

Updated

2 years ago
Attachment #8567922 - Flags: review?(mshal) → review+
(Assignee)

Updated

2 years ago
Attachment #8567922 - Flags: checked-in+
(Assignee)

Updated

2 years ago
See Also: → bug 1135687
(Assignee)

Updated

2 years ago
Depends on: 1136750
(Assignee)

Updated

2 years ago
Depends on: 1137177
(Assignee)

Updated

2 years ago
Depends on: 1137756
(Assignee)

Comment 179

2 years ago
Created attachment 8570562 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable desktop repacks with mozharness for windows 32.patch

This patch enable win32 repacks for windows (they are enabled only for ash)
Attachment #8570562 - Flags: review?(jlund)
Comment on attachment 8570562 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable desktop repacks with mozharness for windows 32.patch

love how your previous patches made this switch so trivial :)

if it passes test-masters.sh r+ stamp from me!
Attachment #8570562 - Flags: review?(jlund) → review+
(Assignee)

Updated

2 years ago
Attachment #8570562 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/efdd04a8c935
(Assignee)

Updated

2 years ago
Depends on: 1094364
(Assignee)

Comment 182

2 years ago
Created attachment 8573163 [details] [diff] [review]
[mozharness] Bug 740142 - backout changes for testing absolute paths in complete-patch.patch

Bug 1137756 has landed in mozilla-central, so we don't need the changes introduced by https://bug740142.bugzilla.mozilla.org/attachment.cgi?id=8567922 anymore
Attachment #8567922 - Attachment is obsolete: true
Attachment #8573163 - Flags: review?(pmoore)
Attachment #8573163 - Flags: review?(pmoore) → review+
(Assignee)

Comment 183

2 years ago
Comment on attachment 8573163 [details] [diff] [review]
[mozharness] Bug 740142 - backout changes for testing absolute paths in complete-patch.patch

Thanks Pete!

https://hg.mozilla.org/build/mozharness/rev/47572f6bbd44
Attachment #8573163 - Flags: checked-in+
(Assignee)

Comment 184

2 years ago
Created attachment 8575294 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable desktop repacks with mozharness for windows 64.patch
Attachment #8575294 - Flags: review?(jlund)
Comment on attachment 8575294 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable desktop repacks with mozharness for windows 64.patch

Review of attachment 8575294 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8575294 - Flags: review?(jlund) → review+
(Assignee)

Comment 186

2 years ago
Comment on attachment 8575294 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable desktop repacks with mozharness for windows 64.patch

https://hg.mozilla.org/build/buildbot-configs/rev/16d096aa98e2
Attachment #8575294 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/16d096aa98e2
(Assignee)

Updated

2 years ago
Depends on: 1144695
(Assignee)

Updated

2 years ago
Depends on: 1144703
(Assignee)

Comment 188

2 years ago
Created attachment 8584691 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Updated builder and slavebuilddir for mozharness l10n.patch

In preparation to the m-c switch, this patch updates mozharness l10n properties:

* builder names:
   from: "Linux ash nightly l10n 1/3"
     to: "Firefox ash linux nightly l10n 1/3"
  to be consistent with existing mozilla-central builder names ("Firefox mozilla-central linux l10n nightly")

* build directories names (on slaves)
   from: Linux-ash-l10n-1_10-0000000000
     to: ash-lx-ntly-l10n-1_3-000000000

mozharness l10n jobs are not enabled on mozilla-central with this patch
Attachment #8562260 - Attachment is obsolete: true
Attachment #8584691 - Flags: review?(catlee)
(Assignee)

Comment 189

2 years ago
Created attachment 8584693 [details] [diff] [review]
builders.diff

Here's the builder list diff

Updated

2 years ago
Attachment #8584691 - Flags: review?(catlee) → review+
(Assignee)

Comment 190

2 years ago
Comment on attachment 8584691 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Updated builder and slavebuilddir for mozharness l10n.patch

Thanks Chris!

landed: https://hg.mozilla.org/build/buildbotcustom/rev/ccba2a403d0e
Attachment #8584691 - Flags: checked-in+
(Assignee)

Comment 191

2 years ago
Created attachment 8585558 [details] [diff] [review]
[mozharness] Bug 740142 en_us_binary_url should use http

Using http instead of https in en_us_binary_url (to be consistent with current l10n builds)
Attachment #8585558 - Flags: review?(rail)
(Assignee)

Updated

2 years ago
Attachment #8585558 - Flags: review?(rail) → review?(kmoir)

Updated

2 years ago
Attachment #8585558 - Flags: review?(kmoir) → review+
(Assignee)

Comment 192

2 years ago
Comment on attachment 8585558 [details] [diff] [review]
[mozharness] Bug 740142 en_us_binary_url should use http

Thanks Kim!

landed: https://hg.mozilla.org/build/mozharness/rev/6b851eccfb68
Attachment #8585558 - Flags: checked-in+
(Assignee)

Comment 193

2 years ago
Created attachment 8585578 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable mozharness desktop repacks on mozilla-central.patch

Enable desktop repacks with mozharness on mozilla-central
Attachment #8476742 - Attachment is obsolete: true
Attachment #8562279 - Attachment is obsolete: true
Attachment #8570562 - Attachment is obsolete: true
Attachment #8575294 - Attachment is obsolete: true
Attachment #8585578 - Flags: review?(rail)
(Assignee)

Comment 194

2 years ago
Created attachment 8585579 [details] [diff] [review]
builders.diff

changes in builders list
Attachment #8585578 - Flags: review?(rail) → review+
In production: https://hg.mozilla.org/build/buildbotcustom/rev/ccba2a403d0e
(Assignee)

Comment 196

2 years ago
Created attachment 8586015 [details] [diff] [review]
[mozharness] Bug 740142 - Always set LATEST_MAR_DIR in upload_env.patch

In some cases LATEST_MAR_DIR is not set and make installers does not create partials.
Attachment #8586015 - Flags: review?(rail)
Attachment #8586015 - Flags: review?(rail) → review+
(Assignee)

Comment 197

2 years ago
Comment on attachment 8586015 [details] [diff] [review]
[mozharness] Bug 740142 - Always set LATEST_MAR_DIR in upload_env.patch

Thanks rail!

landed: https://hg.mozilla.org/build/mozharness/rev/fcb20780926f
Attachment #8586015 - Flags: checked-in+
(Assignee)

Comment 198

2 years ago
Comment on attachment 8585578 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable mozharness desktop repacks on mozilla-central.patch

Thanks Rail!

landed: https://hg.mozilla.org/build/buildbot-configs/rev/44068ffda891
Attachment #8585578 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/44068ffda891
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
(Assignee)

Updated

2 years ago
See Also: → bug 1150015
(Assignee)

Comment 202

2 years ago
Comment on attachment 8585578 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Enable mozharness desktop repacks on mozilla-central.patch

backed out:

https://hg.mozilla.org/build/buildbot-configs/rev/751bd107cf09

root cause of bug 1150015
Attachment #8585578 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/751bd107cf09
(Assignee)

Updated

2 years ago
See Also: → bug 1150093
(Assignee)

Comment 204

2 years ago
Landed again: https://hg.mozilla.org/build/buildbot-configs/rev/1e848214b03a
In production: https://hg.mozilla.org/build/buildbot-configs/rev/1e848214b03a

Updated

2 years ago
Depends on: 1150874
Depends on: 1151215
Depends on: 1151217
Depends on: 1151218
There's something odd going on with the mozilla-central share. The sequence seems to be
19:42:59     INFO - Updating shared repo
19:42:59     INFO - Pulling https://hg.mozilla.org/mozilla-central to /builds/hg-shared/mozilla-central and updating.

19:43:15     INFO - Updating /builds/hg-shared/mozilla-central revision default.
19:43:15     INFO - Running command: ['hg', '--config', 'ui.merge=internal:merge', 'update', '-C', '-r', 'default'] in /builds/hg-shared/mozilla-central
19:43:15     INFO - Copy/paste: hg --config ui.merge=internal:merge update -C -r default

19:52:37     INFO - Trying to share /builds/hg-shared/mozilla-central to /builds/slave/m-cen-m64-ntly-l10n-1_3-000000/build/mozilla-central
19:52:37     INFO - Sharing /builds/hg-shared/mozilla-central to /builds/slave/m-cen-m64-ntly-l10n-1_3-000000/build/mozilla-central.

19:52:38     INFO - Updating /builds/slave/m-cen-m64-ntly-l10n-1_3-000000/build/mozilla-central revision default.
19:52:38     INFO - Running command: ['hg', '--config', 'ui.merge=internal:merge', 'update', '-C', '-r', 'default'] in /builds/slave/m-cen-m64-ntly-l10n-1_3-000000/build/mozilla-central

The update in the share directory takes 10 minutes, and creates a working copy of files there. This isn't necessary given the 3rd and 4th steps.
(In reply to Nick Thomas [:nthomas] from comment #206)
> This isn't necessary given the 3rd and 4th steps.

... and actually is wasteful of disk and time. We should remove it.
The buildernames ended up being something like:
Firefox mozilla-central linux nightly l10n x/3
Firefox mozilla-central linux64 nightly l10n x/3
Firefox mozilla-central macosx64 nightly l10n x/3
Firefox mozilla-central win32 nightly l10n x/3
Firefox mozilla-central win64 nightly l10n x/3

This regressed the regex changes were we able to make as a result of bug 805138.

Please can the buildernames be changed to be of form "-N$" rather than " N/Y$" - to make them consistent with all other buildernames.

Also (but slightly less critically), please can we rearrange the name to make them consistent with the other l10n jobs? (so they match the existing regex r"(?:l10n|localizer) nightly")

eg instead of:
Firefox mozilla-central linux64 nightly l10n x/3
use:
Firefox mozilla-central linux64 l10n nightly-3

Also a quick reminder that we need Treeherder support for things like this added _before_ they get switched on ideally (since detection done at ingestion, not hackily-client-side like TBPL) - thanks :-)
Blocks: 1150880
Flags: needinfo?(mgervasini)
(Assignee)

Comment 209

2 years ago
Created attachment 8589502 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Update l10n builder names.patch

This patch updates builder names

from:
  Firefox mozilla-central linux64 nightly l10n 1/3 
to:
  Firefox mozilla-central linux l10n nightly-1
Attachment #8584691 - Attachment is obsolete: true
Attachment #8584693 - Attachment is obsolete: true
Flags: needinfo?(mgervasini)
Attachment #8589502 - Flags: review?(emorley)
(Assignee)

Comment 210

2 years ago
Created attachment 8589503 [details] [diff] [review]
builders_list.diff

builder list diff
Attachment #8585579 - Attachment is obsolete: true
(Assignee)

Comment 211

2 years ago
Sorry Ed, I've pasted the wrong line:
> to:
>   Firefox mozilla-central linux l10n nightly-1

the new builder name for Firefox mozilla-central linux64 nightly l10n 1/3 is:
 
 Firefox mozilla-central linux l10n nightly-3
Comment on attachment 8589502 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Update l10n builder names.patch

Review of attachment 8589502 [details] [diff] [review]:
-----------------------------------------------------------------

If is_nightly is false, the string 'l10n' no longer gets inserted at all. Is this intentional?
Other than that, looks fine to me - thank you for tweaking this :-)

On a side note, we should get buildbotcustom added to mozreview:
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview-user.html#adding-review-repositories
Attachment #8589502 - Flags: review?(emorley) → review+
(Assignee)

Comment 213

2 years ago
Comment on attachment 8589502 [details] [diff] [review]
[buildbotcustom] Bug 740142 - Update l10n builder names.patch

Thanks Ed!

Landed with minor changes, so the 'l10n' string is always in the builder name (comment #212). r+ from edmorley on irc 

https://hg.mozilla.org/build/buildbotcustom/rev/2ad242b35b83
Attachment #8589502 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbotcustom/rev/2ad242b35b83
(Assignee)

Comment 215

2 years ago
(In reply to Nick Thomas [:nthomas] from comment #207)
> (In reply to Nick Thomas [:nthomas] from comment #206)
> > This isn't necessary given the 3rd and 4th steps.
> 
> ... and actually is wasteful of disk and time. We should remove it.

this will be fixed by switching to 'hgtool' instead of 'hg'.
hgtool.py has some issues under windows so we need to wait for bug 1146892 to land before doing the switch.
Depends on: 1146892
Blocks: 1153138
(Assignee)

Comment 216

2 years ago
Created attachment 8591707 [details] [diff] [review]
[mozharness] Bug 740142 - use hgtools in mozharness repacks.patch

Using to hgtool instead of hg.

the hgtool.py script is in the tools repository, until we don't have it locally (bug 764077), we need to manage the tools repository with 'hg' (and it must be the first one to be cloned/updated). All the other repositories can be managed by the hgtool script.
Attachment #8591707 - Flags: review?(catlee)

Updated

2 years ago
Attachment #8591707 - Flags: review?(catlee) → review+
(Assignee)

Comment 217

2 years ago
Comment on attachment 8591707 [details] [diff] [review]
[mozharness] Bug 740142 - use hgtools in mozharness repacks.patch

Thanks Chris!

landed: https://hg.mozilla.org/build/mozharness/rev/61ab531a1a00
Attachment #8591707 - Flags: checked-in+
(Assignee)

Comment 218

2 years ago
Created attachment 8594903 [details] [diff] [review]
[buildbot-configs] Bug 740142 - increase script_timeout and script_maxtime for mozharness l10n repacks.patch

This patch updates the desktop_l10n.py script timeout/maxtime to the following values (in seconds):

            script      script
platform   timeout     maxtime
---------+----------+-----------
linux    |   1800   |   7200
linux64  |   1800   |   7200
macosx64 |   1800   |   7200
win32    |   7200   |  10800
win64    |   7200   |  10800
Attachment #8594903 - Flags: review?(catlee)
(Assignee)

Comment 219

2 years ago
Created attachment 8594906 [details] [diff] [review]
[buildbotcustom] Bug 740142 - set script_timeout ans script_maxtime in mozhanress l10n builders.patch

... and this patch passes the new timeout values to the SigningScriptFactory in mh_l10n_builders()
Attachment #8589502 - Attachment is obsolete: true
Attachment #8594906 - Flags: review?(catlee)

Updated

2 years ago
Attachment #8594903 - Flags: review?(catlee) → review+

Updated

2 years ago
Attachment #8594906 - Flags: review?(catlee) → review+
(Assignee)

Comment 220

2 years ago
Comment on attachment 8594903 [details] [diff] [review]
[buildbot-configs] Bug 740142 - increase script_timeout and script_maxtime for mozharness l10n repacks.patch

Landed: https://hg.mozilla.org/build/buildbot-configs/rev/3731e57ac6a3
Attachment #8594903 - Flags: checked-in+
(Assignee)

Comment 221

2 years ago
Comment on attachment 8594906 [details] [diff] [review]
[buildbotcustom] Bug 740142 - set script_timeout ans script_maxtime in mozhanress l10n builders.patch

Landed: https://hg.mozilla.org/build/buildbotcustom/rev/bef73c6c14c7
Attachment #8594906 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/3731e57ac6a3
In production: https://hg.mozilla.org/build/buildbotcustom/rev/bef73c6c14c7
(Assignee)

Comment 224

2 years ago
Created attachment 8596014 [details] [diff] [review]
[mozharness] Bug 740142 - code cleanup.patch

After we have switched to the in-tree mechanism (== what is in production now), a bunch of methods have become totally useless.
In particular, the following methods are not called, so they can be deleted:

generate_complete_mar(self, locale)
_copy_complete_mar(self, locale)
_requires_generate_mar(self, mar_type, locale)
localized_marfile(self, locale)
generate_partial_updates(self, locale)
_partial_filename(self, locale)
_get_partialInfo(self, locale)
_query_partial_mar_url(self, locale)
_query_partial_mar_filename(self, locale)
_query_previous_mar_buildid(self, locale)
_delete_pgc_files(self)
_current_mar_url(self, locale)
_previous_mar_url(self, locale)
_get_previous_mar(self, locale)
_current_mar_name(self, locale)
_localized_mar_name(self, locale)
_previous_mar_filename(self, locale)
_current_mar_filename(self, locale)
_unpack_script(self)
Attachment #8524530 - Attachment is obsolete: true
Attachment #8566013 - Attachment is obsolete: true
Attachment #8573163 - Attachment is obsolete: true
Attachment #8585558 - Attachment is obsolete: true
Attachment #8586015 - Attachment is obsolete: true
Attachment #8591707 - Attachment is obsolete: true
Attachment #8596014 - Flags: review?(jlund)
Comment on attachment 8596014 [details] [diff] [review]
[mozharness] Bug 740142 - code cleanup.patch

Review of attachment 8596014 [details] [diff] [review]:
-----------------------------------------------------------------

axe it
Attachment #8596014 - Flags: review?(jlund) → review+
we're still seeing some repacks hit the max time:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/04/2015-04-28-03-02-09-mozilla-central-l10n/mozilla-central-macosx64-l10n-nightly-1-unknown-bm82-build1-build3.txt.gz

07:17:45     INFO -  diffing "Contents/MacOS/crashreporter.app/Contents/Resources/crashreporter.ini"
07:17:45     INFO -        patch "Contents/MacOS/crashreporter.app/Contents/Resources/crashreporter.ini.patch" "Contents/MacOS/crashreporter.app/Contents/Resources/crashreporter.ini"
07:17:45     INFO -  diffing "Contents/MacOS/crashreporter.app/Contents/MacOS/crashreporter"
07:17:45     INFO -        patch "Contents/MacOS/crashreporter.app/Contents/MacOS/crashreporter.patch" "Contents/MacOS/crashreporter.app/Contents/MacOS/crashreporter"
07:17:45     INFO -  diffing "Contents/MacOS/XUL"

command timed out: 7200 seconds elapsed running ['/tools/buildbot/bin/python', 'scripts/scripts/desktop_l10n.py', '--branch-config', 'single_locale/mozilla-central.py', '--platform-config', 'single_locale/macosx64.py', '--environment-config', 'single_locale/production.py', '--balrog-config', 'balrog/production.py', '--total-chunks', '3', '--this-chunk', '1'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=7200.006538
(Assignee)

Comment 227

2 years ago
Created attachment 8599187 [details] [diff] [review]
[buildbot-configs] Bug 740142 - Increase desktop_l10n.py script_maxtime to 3 hour for macosx64.patch
Attachment #8594903 - Attachment is obsolete: true
Attachment #8599187 - Flags: review?(catlee)
(Assignee)

Comment 228

2 years ago
Created attachment 8599369 [details] [diff] [review]
[mozharness] Bug 740142 - use good_statuses in balrog retries.patch

we had several "requests.exceptions.HTTPError: 400 Client Error", this is because retry is not running the command again if the submit to balrog script did not complete successfully.
Attachment #8599369 - Flags: review?(rail)

Updated

2 years ago
Attachment #8599187 - Flags: review?(catlee) → review+
Attachment #8599369 - Flags: review?(rail) → review+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/198c64e0ff38
In production: https://hg.mozilla.org/build/buildbot-configs/rev/198c64e0ff38
(Assignee)

Comment 231

2 years ago
Comment on attachment 8599369 [details] [diff] [review]
[mozharness] Bug 740142 - use good_statuses in balrog retries.patch

landed: https://hg.mozilla.org/build/mozharness/rev/bcf7da255f66
(Assignee)

Updated

2 years ago
Attachment #8599369 - Flags: checked-in+
(Assignee)

Comment 232

2 years ago
Bug 740142 - use good_statuses in balrog retries.patch, in production: https://hg.mozilla.org/build/mozharness/rev/6d3b3681d9ff
(Assignee)

Updated

2 years ago
Attachment #8599187 - Flags: checked-in+
(Assignee)

Comment 233

2 years ago
Let's move the code cleanup patch into bug 1144709
See Also: → bug 1144709
(Assignee)

Comment 234

2 years ago
I think we can close this bug now.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1168999
(In reply to Massimo Gervasini [:mgerva] from comment #234)
> I think we can close this bug now.

\o/

Comment 236

2 years ago
There's bug 1169921 about missing nightlies, maybe related?
You need to log in before you can comment on or make changes to this bug.