978319 - Enable all mozharness desktop build linux variants on Cedar

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jordan Lund (:jlund)]

This will be the first step towards porting all of desktop builds to mozharness.

In this bug I will land the main scripts used for mozharness desktop builds and all the supporting linux configs.

Finally, we should enable this on Cypress only for now.

There are concerns with doing this: Cypress is for Mozharness but it is supposed to be only for testing changes landed to default. It should mirror m-c/buildbot in every way. This is so we can test out mozharness changes before being merged to prod.

However, my opinion is to use Cypress because Cedar is used heavily for other mozharness/automation/new-test features and I wanted something clean and consistent to work off of. NOTE: this shouldn't change mozharness default testing on cypress at all since we don't do linux builds in mozharness(yet!). Plus it seems appropriate being the official mozharn branch! In saying all that, I'm open to testing on any other twig if one is voted more appropriate.

Using any given branch should be easy once 978307 is resolved.

Comment 1

[Jordan Lund (:jlund)]

landed patches for bug 978307. Time to get my mozharness scripts in production. Patches incoming.

Comment 2

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cypress-140318.patch

This is to get my scripts in upstream mozharness.

It does not enable any desktop builds through mozharness in production.

This patch includes the configs for all linux variants under the NightlyBuildFactory umbrella bar xulrunner and the current WIP: non-profiling builds.

In other words, the configs include:

    * Linux mozilla-central
    * Linux mozilla-central leak test
    * Linux x86-64 mozilla-central
    * Linux x86-64 mozilla-central asan
    * Linux x86-64 mozilla-central debug asan
    * Linux x86-64 mozilla-central leak test
    * Linux x86-64 mozilla-central debug static analysis
    * Linux mozilla-central pgo-build
    * Linux mozilla-central nightly
    * Linux x86-64 mozilla-central pgo-build
    * Linux x86-64 mozilla-central nightly
    * Linux x86-64 mozilla-central asan nightly
    * Linux x86-64 mozilla-central debug asan nightly

These scripts also require no buildbot props and handle all the logic for build pool (staging vs production) and branch specifics. Only m-c is implemented but the plumbing is there for other branches to be enabled.

As far as the scripts go, there is use of subclassing BaseConfig to allow for more sophisticated self.config generation (originally possible since bug 974777).

These scripts have a few big TODO's. These are namely 

1) setting return_code and tbpl_level appropriately (right now we have a few instances where we set this but unless in most cases we just fail on failures (few instances of CRITICAL/EXCEPTION).

2) eliminating the need for buildbot_config and buildbot_properties and becoming more dev friendly.

Once landed, I will run through the list of above mentioned Builders on my dev master, post results and then upload the patch to enable on Cypress: ie: simply flipping this on -- http://hg.mozilla.org/build/buildbot-configs/file/0a4d5134bd56/mozilla/project_branches.py#l193

Comment 3

[Jordan Lund (:jlund)]

aki: for reference here is a:

linux m-c build: http://is.gd/hZibxb
linux m-c nightly build: http://is.gd/CTTTgX

Comment 4

[Aki Sasaki (not active)]

Comment on attachment 8393333 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cypress-140318.patch

I think my questions/comments about the config files don't block landing them and using them on a single testing branch, but may cause issues down the road when we add more branches, f/e.

>diff --git a/configs/builds/branch_specifics.py b/configs/builds/branch_specifics.py
>new file mode 100644
>index 0000000..7fa6332
>--- /dev/null
>+++ b/configs/builds/branch_specifics.py
>@@ -0,0 +1,12 @@
>+# this is a dict of branch specific keys/values. As this fills up and more
>+# fx build factories are ported, we might deal with this differently
>+
>+config = {
>+    "mozilla-central": {
>+        "update_channel": "nightly",
>+        "create_snippets": True,
>+        "create_partial": True,
>+        "graph_server_branch_name": "Firefox",
>+        "repo_path": 'mozilla-central'
>+    }
>+}

We may want an optional "generic" or "default" in here to fall back to if it exists; this helped with mozconfigs.
This file may end up living in-tree.

>diff --git a/configs/builds/build_pool_specifics.py b/configs/builds/build_pool_specifics.py
>new file mode 100644
>index 0000000..2b007c3
>--- /dev/null
>+++ b/configs/builds/build_pool_specifics.py
>@@ -0,0 +1,40 @@
>+# this is a dict of pool specific keys/values. As this fills up and more
>+# fx build factories are ported, we might deal with this differently
>+
>+config = {
>+    "staging": {

Curious about the decision to put these into the same file rather than split into separate files.

>+        "sendchange_masters": ["dev-master1.srv.releng.scl3.mozilla.com:9038"],
>+        'balrog_api_root': 'https://aus4-admin-dev.allizom.org',
>+        'balrog_username': 'stage-ffxbld',
>+        'download_base_url': 'http://dev-stage01.srv.releng.scl3.mozilla'
>+                             '.com/pub/mozilla.org/firefox/nightly',

IMO it's easier to read these strings at >80chars than splitting up.

>diff --git a/configs/builds/releng_base_linux_32_builds.py b/configs/builds/releng_base_linux_32_builds.py
>new file mode 100644
>index 0000000..0d26155
>--- /dev/null
>+++ b/configs/builds/releng_base_linux_32_builds.py
>@@ -0,0 +1,207 @@
>+CLOBBERER_URL = 'http://clobberer.pvt.build.mozilla.org/index.php'
>+STAGE_PRODUCT = 'firefox'
>+# TODO Reminder, stage_username and stage_ssh_key differ on Try
>+STAGE_USERNAME = 'ffxbld'
>+STAGE_SSH_KEY = 'ffxbld_dsa'

Do you know how you'll deal with Try?  Another file?  Or TRY_STAGE_*

>+    'mock_pre_package_cmds': [
>+        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
>+    ],

This is a branch- specific directory; do you plan on making this a templatized string ?

>+    'graph_server': 'graphs.allizom.org',
>+    'graph_selector': '/server/collect.cgi',
>+    'graph_branch': 'MozillaTest',

graph_server and graph_branch are staging-specific.  Are these going to get overridden by production values somewhere?

>+    'enable_package_tests': True,

I think this might be branch-specific.

>+    "enable_talos_sendchange": True,
>+    "do_pretty_name_l10n_check": True,
>+    'upload_symbols': True,

These might be branch-specific.

>+    'mock_packages': [

We might want to get this in-tree.

>--- /dev/null
>+++ b/configs/builds/releng_base_linux_64_builds.py
<snip>
>+    'mock_pre_package_cmds': [
>+        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
>+    ],

linux64 gets built in /builds/slave/m-cen-l64-00000000000000000000 for m-c...
If we're working without the proper directory in here, maybe these mock_pre_package_cmds aren't needed?

I think the above linux 32 comments apply to this file as well.

>diff --git a/mozharness/mozilla/buildbot.py b/mozharness/mozilla/buildbot.py
>index 4c3e292..4c19077 100755
>--- a/mozharness/mozilla/buildbot.py
>+++ b/mozharness/mozilla/buildbot.py
>@@ -104,9 +104,17 @@ class BuildbotMixin(object):
>         return self.buildbot_properties.get(prop_name)
>
>     def query_is_nightly(self):
>-        if self.buildbot_config and 'properties' in self.buildbot_config:
>+        # in an effort to move away from buildbot props, we allow for nightly
>+        # builds to be configured at a mozharness level
>+        if self.config.get('nightly_build'):
>+            return True
>+        # now see if we obtained buildbot props and if nightly_build is
>+        # there
>+        elif self.buildbot_config and 'properties' in self.buildbot_config:
>             return self.buildbot_config['properties'].get('nightly_build', False)
>-        return False
>+        # fall back to this
>+        else:
>+            return False

I think these are good changes.  I would prefer the comments to be consolidated as a docstring at the top so we can start towards getting all mozharness methods docstringed, but not a blocker.

Posting this comment and continuing the review.

Comment 5

[Jordan Lund (:jlund)]

> >+config = {
> >+    "mozilla-central": {
> >+        "update_channel": "nightly",
> >+        "create_snippets": True,
> >+        "create_partial": True,
> >+        "graph_server_branch_name": "Firefox",
> >+        "repo_path": 'mozilla-central'
> >+    }
> >+}
> 
> We may want an optional "generic" or "default" in here to fall back to if it
> exists; this helped with mozconfigs.


So right now, if the branch does not exist, it technically is generic. We don't have any addional overrides like we do in the above 'mozilla-central' example. My understanding is that these are specific to m-c. Other branches may have more or less overrides but items in branch_specifics.py should not be introducing new items. Therefore: items in branch_specifics should have defaults/generic values at platform or even default_config level. Many ways to implement this, open to expertise. My logic may fault as I port everything

> This file may end up living in-tree.

ya this is the plan. once implemented, I don't think it will be very hard to put whatever ends up in branch_specifics.py in tree. bug 978510 will track that

> 
> >diff --git a/configs/builds/build_pool_specifics.py b/configs/builds/build_pool_specifics.py
> >new file mode 100644
> >index 0000000..2b007c3
> >--- /dev/null
> >+++ b/configs/builds/build_pool_specifics.py
> >@@ -0,0 +1,40 @@
> >+# this is a dict of pool specific keys/values. As this fills up and more
> >+# fx build factories are ported, we might deal with this differently
> >+
> >+config = {
> >+    "staging": {
> 
> Curious about the decision to put these into the same file rather than split
> into separate files.

Originally this had all the preprod stuff as well. I thought rather than having a config for every release/project branch, and 3 configs for build pools, I'd consolidate them. Note self.config does not pick up all of these. It only knows about its pool or its branch (it doesn't pick up all of branch_specifics.py or build_pool_specifics.py). This logic is in my subclassed BaseConfig and the logs explain how it decides this on run. Again open to changing that behavior.


> >+        'download_base_url': 'http://dev-stage01.srv.releng.scl3.mozilla'
> >+                             '.com/pub/mozilla.org/firefox/nightly',
> 
> IMO it's easier to read these strings at >80chars than splitting up.

Cool, it's hard to know everyones preference. I tend to not like escapes or >80chars but I'd rather do what you guys prefer.


> >+# TODO Reminder, stage_username and stage_ssh_key differ on Try
> >+STAGE_USERNAME = 'ffxbld'
> >+STAGE_SSH_KEY = 'ffxbld_dsa'
> 
> Do you know how you'll deal with Try?  Another file?  Or TRY_STAGE_*

without hindsight of how things in TryBuildFactory or ReleaseBuildFactory work, it is hard to know the extent of how much I have to manipulate existing configs or create new ones. At this point all I can do is write notes to myself like above, noting when I come across things that will be different.

I'm happy to accommodate suggestions
> 
> >+    'mock_pre_package_cmds': [
> >+        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
> >+    ],
> 
> This is a branch- specific directory; do you plan on making this a
> templatized string ?

oops good catch. Thanks I'll fix that today.

> 
> >+    'graph_server': 'graphs.allizom.org',
> >+    'graph_selector': '/server/collect.cgi',
> >+    'graph_branch': 'MozillaTest',
> 
> graph_server and graph_branch are staging-specific.  Are these going to get
> overridden by production values somewhere?

good catch. caught this yesterday. they have already been ported to pool specific

> >+    "enable_talos_sendchange": True,
> >+    "do_pretty_name_l10n_check": True,
> >+    'upload_symbols': True,
> >+    'enable_package_tests': True,
> 
> I think this might be branch-specific.

it could be but my precedence goes default_config -> releng_base_*_config -> build_variant_config -> branch config -> build pool config -> CLI options

So if a platform sets something generic and it's not overwritten in a branch, it persists. (again logs show this and we have --dump-config-hierarchy to see how this is made up when we want it).

> 
> >+    'mock_packages': [
> 
> We might want to get this in-tree.

There are many instances where we change the packages on a branch, gecko version. all you need to do is add 'mock_packages' to a branch in branch_specifics.py. And to recap on above comments, branch_specifics should be portable to in-tree
> 
> >--- /dev/null
> >+++ b/configs/builds/releng_base_linux_64_builds.py
> <snip>
> >+    'mock_pre_package_cmds': [
> >+        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
> >+    ],
> 
> linux64 gets built in /builds/slave/m-cen-l64-00000000000000000000 for m-c...
> If we're working without the proper directory in here, maybe these
> mock_pre_package_cmds aren't needed?

maybe. that definitely is a typo. My guess is that since mozharness creates a 'build' as its work_dir, this actually isn't needed. 

> 
> I think the above linux 32 comments apply to this file as well.
k

> >     def query_is_nightly(self):

> 
> I think these are good changes.  I would prefer the comments to be
> consolidated as a docstring at the top so we can start towards getting all
> mozharness methods docstringed, but not a blocker.

/me adds a TODO to doctring all the things.

Thanks for the review so far.

Comment 6

[Aki Sasaki (not active)]

>+class MakeUploadOutputParser(OutputParser):
>+    tbpl_error_list = TBPL_UPLOAD_ERRORS
>+    # let's create a switch case using name-spaces/dict
>+    # rather than a long if/else with duplicate code
>+    property_conditions = {
>+        # key: property name, value: condition
>+        # TODO find out if we can rm these RPM conditions
>+        'develRpmUrl': "'devel' in m and m.endswith('.rpm')",
>+        'testsRpmUrl': "'tests' in m and m.endswith('.rpm')",
>+        'packageRpmUrl': "m.endswith('.rpm')",
>+        'symbolsUrl': "m.endswith('crashreporter-symbols.zip') or "
>+                      "m.endswith('crashreporter-symbols-full.zip')",
>+        'testsUrl': "m.endswith(('tests.tar.bz2', 'tests.zip'))",
>+        'unsignedApkUrl': "m.endswith('apk') and "
>+                          "'unsigned-unaligned' in m",
>+        'robocopApkUrl': "m.endswith('apk') and 'robocop' in m",
>+        'jsshellUrl': "'jsshell-' in m and m.endswith('.zip')",
>+        'completeMarUrl': "m.endswith('.complete.mar')",
>+        'partialMarUrl': "m.endswith('.mar') and '.partial.' in m",
>+    }

I'm going to hazard a guess this needs to be ordered... if packageRpmUrl comes before testsRpmUrl, you're going to match tests.rpm for the packageRpmUrl.
There's OrderedDict, or you can make this a list of dicts or something.
I'm not a fan of the original buildbotcustom code, but it's scope creep to try to fix it in this patch.

>+class CheckTestCompleteParser(OutputParser):
<snip>
>+        summary = tbox_print_summary(self.pass_count,
>+                                     self.fail_count,
>+                                     self.leaked)
>+        self.info("TinderboxPrint: check<br/>%s\n" % summary)

Calling a helper method here is good, since TinderboxPrint may be going away soon in bug 845388.

>+class BuildingConfig(BaseConfig):

I would love tests for this.

>+# noinspection PyUnusedLocal
>+class BuildOptionParser(object):

And this as well; I definitely want tests for this guy.
Small-scope tests might not be too hard: a test branch config file, a test builder config file, debug- and asan- type config files, each with unique keys and keys that override values from other config files. Then try creating variants of each, and verify the resulting config looks correct.
I'm not going to block landing on these tests, but I'd like to see them before you're done with this project.
Let me know if you need a hand grokking the existing nosetests.

>+    platform = None
>+    bits = None
>+    config_file_search_path = [
>+        '.', os.path.join(sys.path[0], '..', 'configs'),
>+        os.path.join(sys.path[0], '..', '..', 'configs')
>+    ]

I'm ok pulling this config file search and the one from parse_config_file() into its own helper method, if that helps.

>+    build_variants = {
>+        'asan': 'builds/releng_sub_%s_configs/%s_asan.py',
>+        'debug': 'builds/releng_sub_%s_configs/%s_debug.py',
>+        'asan-and-debug': 'builds/releng_sub_%s_configs/%s_asan_and_debug.py',
>+        'stat-and-debug': 'builds/releng_sub_%s_configs/%s_stat_and_debug.py',
>+        'non-unified': 'builds/releng_sub_%s_configs/%s_non_unified.py',
>+        'debug-and-non-unified':
>+            'builds/releng_sub_%s_configs/%s_debug_and_non_unified.py',
>+    }
>+    build_pools = {
>+        'staging': 'builds/build_pool_specifics.py',
>+        'production': 'builds/build_pool_specifics.py',
>+    }
>+    branch_cfg_file = 'builds/branch_specifics.py'

Hm, this feels wrong to hardcode so deep in the code, and doing it this way could make it harder to write test configs (though altering the search path could allow for tests).
I'm not sure I have a great solution other than maybe constants at the top of the file, and using those if they're not overridden during the class instantiation.
Maybe keeping the hardcodes and altering search paths is the easiest path forward, dunno.

>+BUILD_BASE_CONFIG_OPTIONS = [

Mixing constants, runtime code, and functions/classes in the code is a pet peeve of mine, since it bit me at a previous job.
I would prefer this live at the top of the file with the other constants.

>+    [['--developer-run', '--skip-buildbot-actions'], {
>+        "action": "store_false",
>+        "dest": "is_automation",
>+        "default": True,
>+        "help": "If this is running outside of Mozilla's build"
>+                "infrastructure, use this option. It ignores actions"
>+                "that are not needed and adds config checks."}],

You're missing spaces between words ("buildinfrastructure", "actionsthat").  If you construct strings like this, maybe always start a continuing line with a space?
This is one reason why I find splitting strings to meet the 80char requirement is less than ideal: the resulting string is harder to read.

>+    [['--platform'], {
>+        "action": "callback",
>+        "callback": BuildOptionParser.set_platform,
>+        "type": "string",
>+        "dest": "platform",
>+        "help": "Sets the platform we are running this against"
>+                "valid values: 'windows', 'mac', 'linux'"}],

"againstvalid" here, and in --bits

>+        "help": "Sets which bits we are building this against"
>+                "valid values: '32', '64'"}],
>+    [['--custom-build-variant-cfg'], {
>+        "action": "callback",
>+        "callback": BuildOptionParser.set_build_variant,
>+        "type": "string",
>+        "dest": "build_variant",
>+        "help": "Sets the build type and will determine appropriate "
>+                "additional config to use. Either pass a config path "
>+                " or use a valid shortname from: "

Better, but double space before "or" :)

>+                "%s " % (BuildOptionParser.build_variants.keys(),)}],

Trailing whitespace in the help text isn't as bad as trailing whitespace in code.
I think that's the last --help issue in this block ;-)
If there are future blocks like this, I'll probably point out a general whitespace problem in the block and move on.

>+class BuildScript(BuildbotMixin, PurgeMixin, MockMixin,
>+                  SigningMixin, MercurialScript, object):

I think 'object' is assumed, and also inherited from each Mixin and Script object.

>+    def _query_build_prop_from_app_ini(self, prop):
>+        dirs = self.query_abs_dirs()
>+        print_conf_setting_path = os.path.join(dirs['abs_src_dir'],
>+                                               'config',
>+                                               'printconfigsetting.py')
>+        application_ini_path = os.path.join(dirs['abs_obj_dir'],
>+                                            'dist',
>+                                            'bin',
>+                                            'application.ini')

This location *may* change for other non-desktop builds, but we don't have to worry about that right now.

Part 3 continues the BuildScript review...

Comment 7

[Jordan Lund (:jlund)]

I am going to branch off the cset that this patch was made for then fix up parts where appropriate, and finally merge where I am today. To keep track of things, mainly for my own scheduling, I'll reply in these comments with one of the following options:

WILL-FIX: I'll fix before landing
ADD-TODO: I'll add this as a TODO and will resolve it before closing this project.
NEED-INFO: I won't fix until further discussion is made.

Let me know if you feel like any "ADD-TODO"s needs to be a "WILL-FIX".

> I'm going to hazard a guess this needs to be ordered... if packageRpmUrl
> comes before testsRpmUrl, you're going to match tests.rpm for the
> packageRpmUrl.
> There's OrderedDict, or you can make this a list of dicts or something.
> I'm not a fan of the original buildbotcustom code, but it's scope creep to
> try to fix it in this patch.

ADD-TODO: I will put order to this dict.


> >+class BuildingConfig(BaseConfig):
> 
> I would love tests for this.
> 
> >+# noinspection PyUnusedLocal
> >+class BuildOptionParser(object):
> 
> And this as well; 

ADD-TODO: I will add as TODO and make tests for these. I whipped up some nosetests for --dump-config and --dump-config-hierarchy so I should be good.


> >+    config_file_search_path = [
> >+        '.', os.path.join(sys.path[0], '..', 'configs'),
> >+        os.path.join(sys.path[0], '..', '..', 'configs')
> >+    ]
> 
> I'm ok pulling this config file search and the one from parse_config_file()
> into its own helper method, if that helps.
> 
> >+    build_variants = {
> >+        'asan': 'builds/releng_sub_%s_configs/%s_asan.py',
            ...
> >+    }
> >+    build_pools = {
> >+        'staging': 'builds/build_pool_specifics.py',
> >+        'production': 'builds/build_pool_specifics.py',
> >+    }
> >+    branch_cfg_file = 'builds/branch_specifics.py'
> 
> Hm, this feels wrong to hardcode so deep in the code, and doing it this way
> could make it harder to write test configs 

ADD-TODO: I will fix up search path and hard coded options. I will probably have a hard coded default but add options in the init so I can do things like add a search path and file names in nosetests.

> 
> >+BUILD_BASE_CONFIG_OPTIONS = [
> 
> Mixing constants, runtime code, and functions/classes in the code is a pet
> peeve of mine, since it bit me at a previous job.
> I would prefer this live at the top of the file with the other constants.

WILL-FIX: I'll add this to the top.

> 
> >+    [['--developer-run', '--skip-buildbot-actions'], {
> >+        "action": "store_false",
> >+        "dest": "is_automation",
> >+        "default": True,
> >+        "help": "If this is running outside of Mozilla's build"
> >+                "infrastructure, use this option. It ignores actions"
> >+                "that are not needed and adds config checks."}],
> 
> You're missing spaces between words ("buildinfrastructure", "actionsthat"). 
> If you construct strings like this, maybe always start a continuing line
> with a space?
> This is one reason why I find splitting strings to meet the 80char
> requirement is less than ideal: the resulting string is harder to read.

WILL-FIX: add space at the start of each line. Or I'll break 80char if you put your foot down.


> >+        "help": "Sets the platform we are running this against"
> >+                "valid values: 'windows', 'mac', 'linux'"}],
> 
> "againstvalid" here, and in --bits

WILL-FIX:


> >+        "help": "Sets the build type and will determine appropriate "
> >+                "additional config to use. Either pass a config path "
> >+                " or use a valid shortname from: "
> 
> Better, but double space before "or" :)

WILL-FIX:
> 

> 
> >+class BuildScript(BuildbotMixin, PurgeMixin, MockMixin,
> >+                  SigningMixin, MercurialScript, object):
> 
> I think 'object' is assumed, and also inherited from each Mixin and Script
> object.

WILL-FIX:

> 
> >+    def _query_build_prop_from_app_ini(self, prop):
> >+        dirs = self.query_abs_dirs()
> >+        print_conf_setting_path = os.path.join(dirs['abs_src_dir'],
> >+                                               'config',
> >+                                               'printconfigsetting.py')
> >+        application_ini_path = os.path.join(dirs['abs_obj_dir'],
> >+                                            'dist',
> >+                                            'bin',
> >+                                            'application.ini')
> 
> This location *may* change for other non-desktop builds, but we don't have
> to worry about that right now.

ADD-TODO: I will add a TODO to allow for app path in method signature.

Comment 8

[Aki Sasaki (not active)]

(In reply to Jordan Lund (:jlund) from comment #7)
> I am going to branch off the cset that this patch was made for then fix up
> parts where appropriate, and finally merge where I am today. To keep track
> of things, mainly for my own scheduling, I'll reply in these comments with
> one of the following options:
> 
> WILL-FIX: I'll fix before landing
> ADD-TODO: I'll add this as a TODO and will resolve it before closing this
> project.
> NEED-INFO: I won't fix until further discussion is made.
> 
> Let me know if you feel like any "ADD-TODO"s needs to be a "WILL-FIX".
> 
> > I'm going to hazard a guess this needs to be ordered... if packageRpmUrl
> > comes before testsRpmUrl, you're going to match tests.rpm for the
> > packageRpmUrl.
> > There's OrderedDict, or you can make this a list of dicts or something.
> > I'm not a fan of the original buildbotcustom code, but it's scope creep to
> > try to fix it in this patch.
> 
> ADD-TODO: I will put order to this dict.

This has a very strong possibility of causing problems with your sendchanges, so either WILL-FIX or todo right after landing once you notice tests are busted.

Comment 9

[Jordan Lund (:jlund)]

> > ADD-TODO: I will put order to this dict.
> 
> This has a very strong possibility of causing problems with your
> sendchanges, so either WILL-FIX or todo right after landing once you notice
> tests are busted.

/me crosses out ADD-TODO and makes it a WILL-FIX

Comment 10

[Aki Sasaki (not active)]

Comment on attachment 8393333 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cypress-140318.patch

Marking r- due to multiple must fix before landing issues, but I think it's close... I think I've caught everything I want to catch, and we can fix anything else post-landing.  If you want to give me an interdiff next time, that works, or I can do that in the next review.  Let me know if you have any questions, and thanks for the patch!

>+    # TODO add this or merge to ToolToolMixin
>+    def _run_tooltool(self):
>+        self._assert_cfg_valid_for_action(
>+            ['tooltool_script', 'tooltool_bootstrap', 'tooltool_url_list'],
>+            'build'
>+        )
>+        c = self.config
>+        dirs = self.query_abs_dirs()
>+        if not c.get('tooltool_manifest_src'):
>+            return self.warning(ERROR_MSGS['tooltool_manifest_undetermined'])
>+        f_and_un_path = os.path.join(dirs['abs_tools_dir'],

Some of your abbreviations are intuitive for me; this one took me a while to figure out.
Maybe fetch_script?

>+    def _count_ctors(self):

I think we want to limit this to linux/linux64.  Doesn't look like you limit it in either this method or when calling:

>+    def generate_build_stats(self):
>+        """grab build stats following a compile.
>+
>+        this action handles all statitics from a build:
>+        count_ctors and graph_server_post
>+
>+        """
>+        self._count_ctors()

Should be fine for this portion (linux on cypress) but will bite you when you enable mac/windows.

>+    def _create_partial_mar(self):
<snip>
>+        # The mar file name will be the same from one day to the next,
>+        # *except* when we do a version bump for a release. To cope with
>+        # this, we get the name of the previous complete mar directly
>+        # from staging. Version bumps can also often involve multiple mars
>+        # living in the latest dir, so we grab the latest one.
>+        self.info('getting previous mar filename...')
>+        latest_mar_dir = c['latest_mar_dir'] % {'branch': self.branch}
>+        cmd = 'ssh -l %s -i ~/.ssh/%s %s ls -1t %s | grep %s$ | head -n 1' % (
>+            c['stage_username'], c['stage_ssh_key'], c['stage_server'],
>+            latest_mar_dir, c['platform_ftp_name']
>+        )

I hate the original code here, but making you fix it is scope creep.

>+        graph_server_post_path = os.path.join(dirs['abs_tools_dir'],
>+                                              'buildfarm',
>+                                              'utils',
>+                                              'graph_server_post.py')
>+        gs_pythonpath = os.path.join(dirs['abs_tools_dir'],
>+                                     'lib',
>+                                     'python')

I would prefer graph_server_pythonpath

>+    def _set_file_properties(self, file_name, find_dir, prop_type):
<snip>
>+        cmd = ['openssl', 'dgst', '-' + c.get("hash_type", "sha512"),
>+               file_path]

It's a good habit to self.query_exe() for external tools, but not a blocker.

>+    def setup_mock(self, mock_target=None, mock_packages=None, mock_files=None):
>+        """Override setup_mock found in MockMixin.
>+
>+        Initializes and runs any mock initialization actions.
>+        Finally, installs packages.
>+
>+        """
>+        if self.done_mock_setup:
>+            return
>+        self._assert_cfg_valid_for_action(['mock_target'], 'setup-mock')
>+        c = self.config
>+        self.init_mock(c['mock_target'])
>+        if c.get('mock_pre_package_copy_files'):
>+            self.copy_mock_files(c['mock_target'],
>+                                 c.get('mock_pre_package_copy_files'))
>+        for cmd in c.get('mock_pre_package_cmds', []):
>+            self.run_mock_command(c['mock_target'], cmd, '/')
>+        if c.get('mock_packages'):
>+            self.install_mock_packages(c['mock_target'],
>+                                       list(c.get('mock_packages')))
>+        self.done_mock_setup = True

Hm, what's the reason for overriding?  You're also losing Catlee's fix for not initializing mock.
I think we can add the pre_package_cmds to MockMixin if that's the main difference.  If the assert_cfg call is needed, we can either override the method and then call super(), or that assert_cfg call can go into a preflight method.
Let's use the MockMixin method unless there's some gating factor I'm not aware of.

>+    def generate_build_props(self):
>+        """set buildid, sourcestamp, appVersion, and appName."""
>+        dirs = self.query_abs_dirs()
>+        print_conf_setting_path = os.path.join(dirs['abs_src_dir'],
>+                                               'config',
>+                                               'printconfigsetting.py')
>+        application_ini_path = os.path.join(dirs['abs_obj_dir'],
>+                                            'dist',
>+                                            'bin',
>+                                            'application.ini')

You're setting these in multiple places.  Would it be better to get them from a central place?  Either something like query_abs_dirs() but for file paths, or self.something_path
Enhancement

>+    def generate_build_stats(self):
>+        """grab build stats following a compile.
>+
>+        this action handles all statitics from a build:
>+        count_ctors and graph_server_post
>+
>+        """
>+        self._count_ctors()
>+        # TODO in buildbot, we see if self.graphServer exists, but we know that
>+        # nightly does not use graph server so let's use that instead of adding
>+        # confusion to the configs. This may need to change once we
>+        # port xul, valgrind, etc and it turns out we need the
>+        # graphServer condition
>+        if not self.query_is_nightly():
>+            self._graph_server_post()
>+        else:
>+            num_ctors = self.buildbot_properties.get('num_ctors', 'unknown')
>+            self.info("TinderboxPrint: num_ctors: %s" % (num_ctors,))

Looks like your else: also needs a linux check before adding more platforms.

>+        # TODO insert check for uploadMulti factory 2526
>+        # if not self.uploadMulti when we introduce a platform/build that uses
>+        # uploadMulti

This isn't currently an issue for desktop.  If we expand usage to mobile this will become an issue.

>+    def pretty_names(self):
>+        self._assert_cfg_valid_for_action(
>+            ['mock_target'], 'prett-names'

Typo?

>+    def _post_fatal(self):
>+        # until this script has more defined return_codes, let's make sure
>+        # that we at least set the return_code to a failure for things like
>+        # _summarize()
>+        self.return_code = 2

Hm, do we want a worst_level call here?  Otherwise this will eat an EXCEPTION or RETRY.
I think this should be fixed before landing.

Comment 11

[Jordan Lund (:jlund)]

(In reply to Aki Sasaki [:aki] from comment #10)
> Comment on attachment 8393333 [details] [diff] [review]
> 978319_mozharness_desktop_builds_linux_cypress-140318.patch
> 
> Marking r- due to multiple must fix before landing issues, but I think it's
> close... I think I've caught everything I want to catch, and we can fix
> anything else post-landing.  If you want to give me an interdiff next time,
> that works, or I can do that in the next review.  Let me know if you have
> any questions, and thanks for the patch!
> 
> >+    # TODO add this or merge to ToolToolMixin
> >+    def _run_tooltool(self):
> >+        self._assert_cfg_valid_for_action(
> >+            ['tooltool_script', 'tooltool_bootstrap', 'tooltool_url_list'],
> >+            'build'
> >+        )
> >+        c = self.config
> >+        dirs = self.query_abs_dirs()
> >+        if not c.get('tooltool_manifest_src'):
> >+            return self.warning(ERROR_MSGS['tooltool_manifest_undetermined'])
> >+        f_and_un_path = os.path.join(dirs['abs_tools_dir'],
> 
> Some of your abbreviations are intuitive for me; this one took me a while to
> figure out.
> Maybe fetch_script?

WILL-FIX: ya, I don't know where I was going with that ;)

> 
> >+    def _count_ctors(self):
> 
> I think we want to limit this to linux/linux64.  Doesn't look like you limit
> it in either this method or when calling:
> 
> >+    def generate_build_stats(self):
> >+        """grab build stats following a compile.
> >+
> >+        this action handles all statitics from a build:
> >+        count_ctors and graph_server_post
> >+
> >+        """
> >+        self._count_ctors()
> 
> Should be fine for this portion (linux on cypress) but will bite you when
> you enable mac/windows.

WILL-FIX: I'll add a condition.
> 
> >+    def _create_partial_mar(self):
> <snip>
> >+        # The mar file name will be the same from one day to the next,
> >+        # *except* when we do a version bump for a release. To cope with
> >+        # this, we get the name of the previous complete mar directly
> >+        # from staging. Version bumps can also often involve multiple mars
> >+        # living in the latest dir, so we grab the latest one.
> >+        self.info('getting previous mar filename...')
> >+        latest_mar_dir = c['latest_mar_dir'] % {'branch': self.branch}
> >+        cmd = 'ssh -l %s -i ~/.ssh/%s %s ls -1t %s | grep %s$ | head -n 1' % (
> >+            c['stage_username'], c['stage_ssh_key'], c['stage_server'],
> >+            latest_mar_dir, c['platform_ftp_name']
> >+        )
> 
> I hate the original code here, but making you fix it is scope creep.

ADD-TODO: Ya, I felt a direct port here would be easiest. I'll add a todo. create_partial_mar is lonnnng.

> 
> >+        graph_server_post_path = os.path.join(dirs['abs_tools_dir'],
> >+                                              'buildfarm',
> >+                                              'utils',
> >+                                              'graph_server_post.py')
> >+        gs_pythonpath = os.path.join(dirs['abs_tools_dir'],
> >+                                     'lib',
> >+                                     'python')
> 
> I would prefer graph_server_pythonpath
> 
> >+    def _set_file_properties(self, file_name, find_dir, prop_type):
> <snip>
> >+        cmd = ['openssl', 'dgst', '-' + c.get("hash_type", "sha512"),
> >+               file_path]
> 
> It's a good habit to self.query_exe() for external tools, but not a blocker.

WILL-FIX: should be an easy fix to wrap openssl in that.

> 
> >+    def setup_mock(self, mock_target=None, mock_packages=None, mock_files=None):
> >+        """Override setup_mock found in MockMixin.
> >+
> >+        Initializes and runs any mock initialization actions.
> >+        Finally, installs packages.
> >+
> >+        """
> >+        if self.done_mock_setup:
> >+            return
> >+        self._assert_cfg_valid_for_action(['mock_target'], 'setup-mock')
> >+        c = self.config
> >+        self.init_mock(c['mock_target'])
> >+        if c.get('mock_pre_package_copy_files'):
> >+            self.copy_mock_files(c['mock_target'],
> >+                                 c.get('mock_pre_package_copy_files'))
> >+        for cmd in c.get('mock_pre_package_cmds', []):
> >+            self.run_mock_command(c['mock_target'], cmd, '/')
> >+        if c.get('mock_packages'):
> >+            self.install_mock_packages(c['mock_target'],
> >+                                       list(c.get('mock_packages')))
> >+        self.done_mock_setup = True
> 
> Hm, what's the reason for overriding?  You're also losing Catlee's fix for
> not initializing mock.
> I think we can add the pre_package_cmds to MockMixin if that's the main
> difference.  If the assert_cfg call is needed, we can either override the
> method and then call super(), or that assert_cfg call can go into a
> preflight method.
> Let's use the MockMixin method unless there's some gating factor I'm not
> aware of.

WILL-FIX: mock was one of the first thing I ported. AFAIK there is no reason to need this anymore. I think my original logic was for pre package cmds but since that just essentially creates the 'work_dir', we can just use mockmixin's version. Either way I'd have to use MockMixin.setup_mock so I avail of: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/mock.py#191


> 
> >+    def generate_build_props(self):
> >+        """set buildid, sourcestamp, appVersion, and appName."""
> >+        dirs = self.query_abs_dirs()
> >+        print_conf_setting_path = os.path.join(dirs['abs_src_dir'],
> >+                                               'config',
> >+                                               'printconfigsetting.py')
> >+        application_ini_path = os.path.join(dirs['abs_obj_dir'],
> >+                                            'dist',
> >+                                            'bin',
> >+                                            'application.ini')
> 
> You're setting these in multiple places.  Would it be better to get them
> from a central place?  Either something like query_abs_dirs() but for file
> paths, or self.something_path
> Enhancement

WILL-FIX: makes sense. To make this better, I always find it hard whether to hardcode in query_abs_dirs or add a config item. I think a combination is the best ie:

config_file.py ->
c['app_ini_path'] = "%(abs_obj_dir)/dist/bin/application.ini")
then in query_abs_dirs() ->
dirs['abs_app_ini_path'] = c['app_ini_path'] % (abs_obj_dir)

above is what I'll do unless suggested otherwise.

> 
> >+    def generate_build_stats(self):
> >+        """grab build stats following a compile.
> >+
> >+        this action handles all statitics from a build:
> >+        count_ctors and graph_server_post
> >+
> >+        """
> >+        self._count_ctors()
> >+        # TODO in buildbot, we see if self.graphServer exists, but we know that
> >+        # nightly does not use graph server so let's use that instead of adding
> >+        # confusion to the configs. This may need to change once we
> >+        # port xul, valgrind, etc and it turns out we need the
> >+        # graphServer condition
> >+        if not self.query_is_nightly():
> >+            self._graph_server_post()
> >+        else:
> >+            num_ctors = self.buildbot_properties.get('num_ctors', 'unknown')
> >+            self.info("TinderboxPrint: num_ctors: %s" % (num_ctors,))
> 
> Looks like your else: also needs a linux check before adding more platforms.

WILL-FIX: I'll add the same num_ctors able condition as mentioned above.

> 
> >+        # TODO insert check for uploadMulti factory 2526
> >+        # if not self.uploadMulti when we introduce a platform/build that uses
> >+        # uploadMulti
> 
> This isn't currently an issue for desktop.  If we expand usage to mobile
> this will become an issue.

ya, an early reminder I added for myself before gaining more bbot-cfgs hindsight. I'll remove the TODO since I think this is something that a base class of BuildScript like MobileBuildScript should overwrite this.

> 
> >+    def pretty_names(self):
> >+        self._assert_cfg_valid_for_action(
> >+            ['mock_target'], 'prett-names'
> 
> Typo?

WILL-FIX: yup, thanks, that would have been confusing when someone tried to look for that non-existing action

> 
> >+    def _post_fatal(self):
> >+        # until this script has more defined return_codes, let's make sure
> >+        # that we at least set the return_code to a failure for things like
> >+        # _summarize()
> >+        self.return_code = 2
> 
> Hm, do we want a worst_level call here?  Otherwise this will eat an
> EXCEPTION or RETRY.
> I think this should be fixed before landing.

NEED-INFO: Return codes are not implemented well right now. in my _summarize() I have if self.return_code != 0, let's consider this a TBPL_FAILURE. We take that and worst_level it against self.worst_tbpl_level and the result of that sets self.return_code via self.buildbot_status(). I plan to fix this up in: 985068. Do you think that logic is OK for now or will we need something further before landing?

Comment 12

[Jordan Lund (:jlund)]

> >+        graph_server_post_path = os.path.join(dirs['abs_tools_dir'],
> >+                                              'buildfarm',
> >+                                              'utils',
> >+                                              'graph_server_post.py')
> >+        gs_pythonpath = os.path.join(dirs['abs_tools_dir'],
> >+                                     'lib',
> >+                                     'python')
> 
> I would prefer graph_server_pythonpath

WILL-FIX: whoops, I missed this one from your review.

Comment 13

[Aki Sasaki (not active)]

(In reply to Jordan Lund (:jlund) from comment #11)
> NEED-INFO: Return codes are not implemented well right now. in my
> _summarize() I have if self.return_code != 0, let's consider this a
> TBPL_FAILURE. We take that and worst_level it against self.worst_tbpl_level
> and the result of that sets self.return_code via self.buildbot_status(). I
> plan to fix this up in: 985068. Do you think that logic is OK for now or
> will we need something further before landing?

I don't think we hit _summarize() in _post_fatal(), and we fall back to buildbot parsing the return code.
I do think we need EXCEPTION and RETRY support before this rolls out fully.  This doesn't have to block rolling out to Cypress.

Comment 14

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cypress-140323-partial.patch

this is a diff against the last r? patch.

I will follow this attachment up with a full diff against mozharness 'default'.

This attachment reflects the previous review.

In addition to the review, this patch adds the following that are required for cypress and mozilla-central:
- allows moz_symbols_extra_buildid to change on a branch and platform basis.
- removes aus2_user and aus2_ssh_key from build pools since they are now the same
- gives clobberer_url, symbol_server_host and graph_server staging and prod vals in staging/production
- sorts the build_pool_specific items in alphabetical order
- adds default_vcs, repos, and graph_selector to default_config instead of at platform level
- adds tooltool_url to default_config and makes it a string instead of an uneeded list
- adds missing comma to valgrind package in linux 32 mock packages
- removes hg_mozconfig as it uses old mozconfigs that are no longer maintained for NightlyBuildFactory builders
- removes unneeded rpm items from MakeUploadOutputParser

Comment 15

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cypress-140323.patch

if the previous partial patch is a r+, this would be the full patch that would be committed.

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 8395523 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cypress-140323-partial.patch

(In reply to Jordan Lund (:jlund) from comment #14)
> - removes aus2_user and aus2_ssh_key from build pools since they are now the
> same

I'm not sure about this, but if you don't need it for balrog, wfm.

> - adds tooltool_url to default_config and makes it a string instead of an
> uneeded list

It would be nice to have a list at some point, but looks like you were only using the first value so we would have had to add list support when needed anyway.

> - removes hg_mozconfig as it uses old mozconfigs that are no longer
> maintained for NightlyBuildFactory builders

Awesome.

>diff --git a/configs/builds/releng_base_linux_32_builds.py b/configs/builds/releng_base_linux_32_builds.py
<snip>
>-    'mock_pre_package_cmds': [
>-        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
>-    ],
>+    'mock_pre_package_cmds': [],

I don't think anything references these anymore.  Any reason you're keeping it around as an empty list?  (cleanup, not a blocker)

>diff --git a/mozharness/mozilla/buildbot.py b/mozharness/mozilla/buildbot.py
>index 4c19077..05a17cb 100755
>--- a/mozharness/mozilla/buildbot.py
>+++ b/mozharness/mozilla/buildbot.py
>@@ -104,15 +104,18 @@ class BuildbotMixin(object):
>         return self.buildbot_properties.get(prop_name)
>
>     def query_is_nightly(self):
>-        # in an effort to move away from buildbot props, we allow for nightly
>-        # builds to be configured at a mozharness level
>+        """returns whether or not the script should run as a nightly build.
>+
>+           First will check for 'nightly_build' in self.config and if that is
>+           not True, we will also allow buildbot_config to determine
>+           for us. Failing all of that, we default to False.
>+           Note, dependancy on buildbot_config is being deprectiated.

deprecated

>@@ -119,7 +113,7 @@ class MakeUploadOutputParser(OutputParser):
>         m = re.compile(pat).match(line)
>         if m:
>             m = m.group(1)
>-            for prop, condition in self.property_conditions.iteritems():
>+            for prop, condition in self.property_conditions:
>                 if eval(condition):

A typo might cause an exception here; maybe try/except with a fatal() ?  Not a blocker, but good for followup.

Comment 17

[Aki Sasaki (not active)]

Comment on attachment 8395524 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cypress-140323.patch

r+ based on the interdiff.

Comment 18

[Jordan Lund (:jlund)]

> > - removes aus2_user and aus2_ssh_key from build pools since they are now the
> > same
> 
> I'm not sure about this, but if you don't need it for balrog, wfm.

Sorry that wasn't explicit. I moved it to default_config since it should never change on a platform, branch, or build pool basis.


> >-    'mock_pre_package_cmds': [
> >-        'mkdir -p /builds/slave/m-cen-lx-000000000000000000000/build'
> >-    ],
> >+    'mock_pre_package_cmds': [],
> 
> I don't think anything references these anymore.  Any reason you're keeping
> it around as an empty list?  (cleanup, not a blocker)

removed

> >+           Note, dependancy on buildbot_config is being deprectiated.
> 
> deprecated

fixed

> 
> >@@ -119,7 +113,7 @@ class MakeUploadOutputParser(OutputParser):
> >         m = re.compile(pat).match(line)
> >         if m:
> >             m = m.group(1)
> >-            for prop, condition in self.property_conditions.iteritems():
> >+            for prop, condition in self.property_conditions:
> >                 if eval(condition):
> 
> A typo might cause an exception here; maybe try/except with a fatal() ?  Not
> a blocker, but good for followup.

added TODO

Comment 19

[Jordan Lund (:jlund)]

Comment on attachment 8395524 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cypress-140323.patch

pushed to 'default': https://hg.mozilla.org/build/mozharness/rev/f0bb54c56271

Comment 20

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cedar-impl_branches-140326.patch

This patch finishes Linux dev work. Following this we *should* be able to test on any branch with linux desktop builders (aside from Try)

- finishes branch specific changes
- adds packages action to non-unified (this was a bug)
- moves print lines from subclassed BaseConfig to BuildScript's pre_config_lock (this allows it to have proper logging and not be outputted at the end)
- defines pre_config_lock() for BuildScript. Here we explain what is happening with self.config before it locks.
- up till now we could config per platform/variant, branch, and pool. But customizing a particular platform at a branch level was not possible. This adds that through pre_config_lock's 'platform_overrides' check
- give 'repo_path' a default value if not set so we don't pollute branch_configs.py
- implements 'per-checkin' strategy. some branches set pgo_strategy='per-checkin'. This lets us fire sendchanges with branch 'pgo' instead of 'opt' in them.
- allows for unittest_platform (like buildbot-based key) to be set instead of using platform in sendchanges
- adds enable_checktests (like buildbot-based key)
- sets create_{snippet and partial} vals to True by default. There very very few cases where we enable_nightly but set create_snippets, partials to False.


As per discussion today, I'd like to try and land this on Cedar first. A patch, along with dev results will follow this patch.

Comment 21

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_enable_cedar-bbot-cfgs-140327.patch

time to start turning desktop mozharness builds on!

starting with.... cedar and linux builders.

this patch passes test-masters.sh

builder_list diff: https://pastebin.mozilla.org/4703239
dump_master diff: https://pastebin.mozilla.org/4703238

**NOTE** this patch is dependent on mozharness default -> production

Comment 22

[Jordan Lund (:jlund)]

Comment on attachment 8397705 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cedar-impl_branches-140326.patch

pushed to default: https://hg.mozilla.org/build/mozharness/rev/66b98a2c32e1

Comment 23

[Jordan Lund (:jlund)]

(In reply to Jordan Lund (:jlund) from comment #21)
> Created attachment 8397986 [details] [diff] [review]
> 978319_mozharness_desktop_builds_linux_enable_cedar-bbot-cfgs-140327.patch
> 
> time to start turning desktop mozharness builds on!
> 
> starting with.... cedar and linux builders.
> 
> this patch passes test-masters.sh
> 
> builder_list diff: https://pastebin.mozilla.org/4703239
> dump_master diff: https://pastebin.mozilla.org/4703238
> 
> **NOTE** this patch is dependent on mozharness default -> production

For example of these builds: http://dev-master1.srv.releng.scl3.mozilla.com:8037/builders

There are many example builds on cedar.

The only thing not tested is my production items as I could only use staging in my dev environment.

Comment 24

[John Hopkins (:jhopkins)]

in production.

Comment 25

[John Hopkins (:jhopkins)]

in production.

Comment 26

[Jordan Lund (:jlund)]

looks like we have cedar linux mozharn builds running: https://tbpl.mozilla.org/?showall=all&tree=Cedar&rev=ab6287eb1b52

resolving for now.

Comment 27

[Jordan Lund (:jlund)]

looks like the unittest sendchange step on debug builds is broken:

buildbot sendchange
--master buildbot-master81.build.mozilla.org:9301 --username
sendchange-unittest --branch cedar-linux-debug-opt-unittest -r
957442226e378bb6ae914d4c925b2592e3ff19be --username [e-mail]
--comments "Update package version" --property buildid:20140402133633
--property pgo_build:False --property
builduid:f326f4b59e194edfa50e07dd6acdc3dc --property nightly_build:False
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/cedar-linux-debug/1396470993/firefox-31.0a1.en-US.linux-i686.tar.bz2
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/cedar-linux-debug/1396470993/firefox-31.0a1.en-US.linux-i686.tests.zip


the above says it sent successfully but but it should have been: `--branch cedar-linux-debug-unittest` and not `--branch cedar-linux-debug-opt-unittest` obviously :)

patches incoming

Comment 28

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cedar--debug-sendchange-fix-140404.patch

somehow I removed 'debug_build' in my config.

but even if I still had that, the logic in branch naming needed some love as well.

Comment 29

[Jordan Lund (:jlund)]

Comment on attachment 8401934 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cedar--debug-sendchange-fix-140404.patch

thanks!

Comment 30

[Jordan Lund (:jlund)]

on default: https://hg.mozilla.org/build/mozharness/rev/a54513a0e6ba

Comment 31

[Jordan Lund (:jlund)]

looks like debug unittest sendchanges are working again

Comment 32

[[e-mail] (:bhearsum)]

something is in production

Comment 33

[Jordan Lund (:jlund)]

and linux builds are broken again.  I believe this is due to tooltool and simone's changes.

patches incoming

Comment 34

[Jordan Lund (:jlund)]

Attachment: 978319_mozharness_desktop_builds_linux_cedar--tooltool-fix-140407.patch

this uses mshals tooltool fetch and unpack script path change

Comment 35

[Justin Wood (:Callek)]

Comment on attachment 8403056 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cedar--tooltool-fix-140407.patch

using http://hg.mozilla.org/build/buildbotcustom/rev/f7ad8167f2fc as my guide looks good

Comment 36

[Jordan Lund (:jlund)]

Comment on attachment 8403056 [details] [diff] [review]
978319_mozharness_desktop_builds_linux_cedar--tooltool-fix-140407.patch

pushed to default: https://hg.mozilla.org/build/mozharness/rev/8345f39d0db3

Comment 37

[Jordan Lund (:jlund)]

problem seems to have been fixed

https://bugzilla.mozilla.org/attachment.cgi?id=8403056&action=edit

in production :)

Comment 38

[Jordan Lund (:jlund)]

FYI: I'm on PTO until April 15th. I probably will not reply until then.