1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax

Status: RESOLVED FIXED

(Testing :: General, defect, P1)

Description

[Jonathan Griffin (:jgriffin)]

We'd like mozharness build scripts (used by either buildbot or TaskCluster) to trigger artifact builds instead of full builds when a certain flag is present in try syntax.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Here's a rough try push that's updating the mozharness config with the artifact build variant in _pre_config_lock. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0435e8303f02&selectedJob=24164911

I'll work on detecting try syntax next.

In the mean time, chmanchester, can you help me decipher some errors? File not found errors; see Treeherder link above. Example:

 16:25:58     INFO -  configure: warning: Cannot --enable-signmar with --disable-compile-environment
 16:25:58     INFO -  Traceback (most recent call last):
 16:25:58     INFO -    File "/tools/python27/lib/python2.7/runpy.py", line 162, in _run_module_as_main
 16:25:58     INFO -      "__main__", fname, loader, pkg_name)
 16:25:58     INFO -    File "/tools/python27/lib/python2.7/runpy.py", line 72, in _run_code
 16:25:58     INFO -      exec code in run_globals
 16:25:58     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/python/mozbuild/mozbuild/configure/libstdcxx.py", line 79, in <module>
 16:25:58     INFO -      print 'MOZ_LIBSTDCXX_TARGET_VERSION=%s' % find_version(cxx_env)
 16:25:58     INFO -    File "/builds/slave/try-l64-0000000000000000000000/build/src/python/mozbuild/mozbuild/configure/libstdcxx.py", line 64, in find_version
 16:25:58     INFO -      p = subprocess.Popen(args, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
 16:25:58     INFO -    File "/tools/python27/lib/python2.7/subprocess.py", line 710, in __init__
 16:25:58     INFO -      errread, errwrite)
 16:25:58     INFO -    File "/tools/python27/lib/python2.7/subprocess.py", line 1327, in _execute_child
 16:25:58     INFO -      raise child_exception
16:25:58 INFO - OSError: [Errno 2] No such file or directory

I thought missing files shouldn't be an issue on Linux. Since it seems you were fixing something similar for other platforms in Bug 1278700, do you have any idea how to deal with these?

Comment 2

[Chris Manchester (limited bugmail, email directly)]

The issue in that buildbot build is https://bugzilla.mozilla.org/show_bug.cgi?id=1278700#c7 and below: if we disable signing in the mozharness config, we still get a MOZ_SIGN_COMMAND in the environment from buildbot, but other things aren't set up correctly. The workaround I found was to set enable_signing in the config.

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

As discussed, here's a more recent build failure on Linux when forcing artifact build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eca49169ca69d72def8e4a90e4331b5e3b2b690&selectedJob=26070576

With enabled_signing set to True, the artifact build on buildbot is successful, but I'm seeing failures in TC. What do you think?

Out of curiosity, I've added some OS X and Windows build jobs to that try push -- awaiting results.

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #3)
> Out of curiosity, I've added some OS X and Windows build jobs to that try
> push -- awaiting results.

Whoops, nevermind that last sentence. The configs for those artifact builds aren't defined yet!

First question in Comment 3 still applies though.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

It looks like the TC build is running the check tests and one is failing... this is one of the "actions" according to mozharness. We don't have this specified in the mozharness config, but I guess TaskCluster does this separately.

We can either skip this test for artifact builds (it's failing because it's trying to find js/src/config.status, which we don't generate during artifact builds), or we can make taskcluster detect this and do the right thing when artifact builds are requested.

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

'artifact' flag in try syntax being processed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d56706e34d68f63f43d2edef5d11b798fed771be

Comment 7

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

Review commit: https://reviewboard.mozilla.org/r/74194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74194/

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

WIP
Extra work is needed to make the "replacement" artifact builds succeed in TaskCluster

Review commit: https://reviewboard.mozilla.org/r/74196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74196/

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

https://reviewboard.mozilla.org/r/74194/#review72052

::: testing/mozharness/mozharness/base/config.py:520
(Diff revision 1)
> +        if options.list_actions:
> +            self.list_actions()
> +
> +        # Keep? This is for saving the volatile config in the dump_config
> +        self._config['volatile_config'] = self.volatile_config
> +
> +        self.options = options
> +        self.args = args
> +        return (self.options, self.args)
> +

I'm moving these out of `update_actions` so that I can reuse it in `_pre_config_lock`

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:644
(Diff revision 1)
>          if build_variant:
>              variant_cfg = BuildOptionParser.build_variants[build_variant] % (
>                  BuildOptionParser.platform,
>                  BuildOptionParser.bits
>              )
> +        self.info("Build variant cfg: %s" % variant_cfg)

whoops, this can probably go away

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Chris Manchester (:chmanchester) from comment #5)
> It looks like the TC build is running the check tests and one is failing...
> this is one of the "actions" according to mozharness. We don't have this
> specified in the mozharness config, but I guess TaskCluster does this
> separately.

Yes, I found that 'check-test' is included in the task config: https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/taskcluster/ci/legacy/tasks/builds/base_linux64.yml#24

Clarification: In the case of an artifact build, can we skip check-test completely, or do we prefer to keep that action and just disable some tests?

> 
> We can either skip this test for artifact builds (it's failing because it's
> trying to find js/src/config.status, which we don't generate during artifact
> builds), or we can make taskcluster detect this and do the right thing when
> artifact builds are requested.

I could also force the mozharness script remove the check-test action as part of updating the config for artifact builds. 

How would I go about disabling specific tests in the artifact case? (Where do we decide which tests run?)

Comment 11

[Armen [:armenzg]]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

https://reviewboard.mozilla.org/r/74194/#review72080

I like your changes. Nothing major. Just nits to be addressed. Feel free to land when you address them.

::: testing/mozharness/mozharness/base/config.py:519
(Diff revision 1)
>          for key in self.volatile_config.keys():
>              if self._config.get(key) is not None:
>                  self.volatile_config[key] = self._config[key]
>                  del(self._config[key])
>  
> -        """Actions.
> +        self.update_actions()

Thank you for this refactoring. Much better.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:458
(Diff revision 1)
> +        """ sets an extra config file.
> +
> +        This is done by either taking an existing filepath or by taking a valid
> +        shortname coupled with known platform/bits.
> +        """
> +        valid_variant_cfg_path, prospective_cfg_path = cls.find_variant_cfg_path(

This also seems a small refactoring unless I'm missing something. I'm keen of breaking things into smaller functions. Thank you.

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:93
(Diff revision 1)
>                  # from hg.
>                  props = self.buildbot_config['properties']
> +                repo_url = 'https://hg.mozilla.org/%s/'
> +                if 'revision' in props and 'repo_path' in props:
> -                rev = props['revision']
> +                    rev = props['revision']
> -                repo = props['repo_path']
> +                    branch = props['repo_path']

Can we please name this either repo_name or repo_path?
I want to stop using 'branch' for something that it is not. I know the code is full of it.

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:106
(Diff revision 1)
> +                    repo_url = os.environ.get('GECKO_HEAD_REPOSITORY',
> +                                              repo_url % 'try')
> +                if not repo_url.endswith('/'):
> +                    repo_url += '/'
> +
> +                url = '%sjson-pushes?changeset=%s&full=1' % (repo_url, rev)

Could you please use ''.format()?
```python
url = '{}json-pushes?changeset={}&full=1'.format(repo_url, rev))
```

::: testing/mozharness/scripts/fx_desktop_build.py:152
(Diff revision 1)
> +        c.update({
> +            'build_variant': variant,
> +            'config_files': c['config_files'] + [variant_cfg_path]
> +        })
> +
> +        self.info("Updating self.config with the following from %s:" % variant_cfg_path)

Please use ''.format()

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74194/diff/1-2/

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784885 [details]
Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74196/diff/1-2/

Comment 14

[Chris Manchester (limited bugmail, email directly)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #10)
> (In reply to Chris Manchester (:chmanchester) from comment #5)
> > It looks like the TC build is running the check tests and one is failing...
> > this is one of the "actions" according to mozharness. We don't have this
> > specified in the mozharness config, but I guess TaskCluster does this
> > separately.
> 
> Yes, I found that 'check-test' is included in the task config:
> https://dxr.mozilla.org/mozilla-central/rev/
> 01748a2b1a463f24efd9cd8abad9ccfd76b037b8/taskcluster/ci/legacy/tasks/builds/
> base_linux64.yml#24
> 
> Clarification: In the case of an artifact build, can we skip check-test
> completely, or do we prefer to keep that action and just disable some tests?

Either is acceptable for now. The check tests aren't doing that much interesting with respect to the build, but it occurred to me that this would be a reasonable way to run (for instance) the build system python tests without waiting on a full build.

> 
> > 
> > We can either skip this test for artifact builds (it's failing because it's
> > trying to find js/src/config.status, which we don't generate during artifact
> > builds), or we can make taskcluster detect this and do the right thing when
> > artifact builds are requested.
> 
> I could also force the mozharness script remove the check-test action as
> part of updating the config for artifact builds. 
> 
> How would I go about disabling specific tests in the artifact case? (Where
> do we decide which tests run?)

We can modify this bit: http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/config/tests/test_mozbuild_reading.py#87

So it also skips the test when config.substs['MOZ_ARTIFACT_BUILDS'] is set.

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Ok, I'll try that.

In the mean time I also tried having the mozharness script ignore the actions set by TaskCluster in favour of using 'default_actions' from the config (only if `-artifact` flag is present in try syntax) and that works too: https://hg.mozilla.org/try/rev/78c7fa334d7560d5256f7d7143646502c904802e#l2.12 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=d373bc421f04858f6a7094e0819d785a581e38ca -- so nice to see 10-minute build jobs!

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Having disabled test_orphan_file patterns, it looks to me like all the python tests pass (so far), but then we hit errors in xpcom/tests: 

>15:51:25     INFO - make[3]: Leaving directory `/home/worker/workspace/build/src/obj-firefox/xpcom/tests/gtest'
>15:51:25     INFO - XPCOM_DEBUG_BREAK=stack-and-abort ../../dist/bin/run-mozilla.sh \
>15:51:25     INFO - 	  ../../dist/bin/TestRegistrationOrder '/home/worker/workspace/build/src/xpcom/tests/regorder'
>15:51:25     INFO - run-mozilla.sh: Cannot execute ../../dist/bin/TestRegistrationOrder

Details: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=26520057

Can we skip these failures, too? How?

Getting check-tests working here may be scope bloat. Again, we can get the artifact build working on TaskCluster if we override the mozharness actions forced in the task definition (in the artifact case only), in favour of the default_actions specified by the config in 64_artifact.py. This works fine in my try push from Comment 15.

Comment 17

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I will look into getting this working on Windows and OS X next.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #16)
> Having disabled test_orphan_file patterns, it looks to me like all the
> python tests pass (so far), but then we hit errors in xpcom/tests: 
> 
> >15:51:25     INFO - make[3]: Leaving directory `/home/worker/workspace/build/src/obj-firefox/xpcom/tests/gtest'
> >15:51:25     INFO - XPCOM_DEBUG_BREAK=stack-and-abort ../../dist/bin/run-mozilla.sh \
> >15:51:25     INFO - 	  ../../dist/bin/TestRegistrationOrder '/home/worker/workspace/build/src/xpcom/tests/regorder'
> >15:51:25     INFO - run-mozilla.sh: Cannot execute ../../dist/bin/TestRegistrationOrder
> 
> Details:
> https://treeherder.mozilla.org/#/jobs?repo=try&author=mjzffr@gmail.
> com&selectedJob=26520057
> 
> Can we skip these failures, too? How?
> 
> Getting check-tests working here may be scope bloat. Again, we can get the
> artifact build working on TaskCluster if we override the mozharness actions
> forced in the task definition (in the artifact case only), in favour of the
> default_actions specified by the config in 64_artifact.py. This works fine
> in my try push from Comment 15.

It looks like we could guard the relevant target in xpcom/tests/Makefile.in (with ifdef COMPILE_ENVIRONMENT), but I agree if it seems like less work to skip check tests entirely let's go with that.

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74194/diff/2-3/

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784885 [details]
Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74196/diff/2-3/

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

This patch series yields successful forced artifact builds (--artifact flag) for linux64 on buildbot and taskcluster. I'd like to land this now, then add configs for other platforms in Bug 1299702.

More follow-up - We will also have to make sure the resulting builds are made available for tests: for example, the build isn't being downloaded by a dependent test on linux64 debug on taskcluster: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f695235fd34b6033d3e6b42eb17f3d7fd2f91e&selectedJob=27105049.

Comment 22

[Armen [:armenzg]]

Comment on attachment 8784885 [details]
Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

https://reviewboard.mozilla.org/r/74196/#review75886

::: testing/mozharness/configs/builds/releng_sub_linux_configs/64_artifact.py:20
(Diff revision 3)
>          "buildbot": "/tools/buildbot/bin/buildbot",
>      },
>      'app_ini_path': '%(obj_dir)s/dist/bin/application.ini',
>      # decides whether we want to use moz_sign_cmd in env
> -    'enable_signing': False,
> +    # XXX setting to True as in https://bugzilla.mozilla.org/show_bug.cgi?id=1278700#c8
> +    'enable_signing': True,

Would you mind enquirying about this?
I don't like dragging a setting around without understanding it.

If it makes your life easier to land it now and deal it with it later by all means.

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Regarding 'enable_signing' in the 64_artifact.py config: if we set it to False, forced artifact builds fail on linux64 opt in buildbot only. For some reason MOZ_SIGN_CMD is always defined in that environment and not elsewhere, which is why we're seeing this broken behaviour: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=27160825

I have a new try push that just wipes out MOZ_SIGN_CMD if enable_signing is False in buildbase.py. (See https://hg.mozilla.org/try/rev/1f46d70c9870191e7ecd61b6ac6ce7487652071b). mshal, I think you were the last person to review this part of buildbase.py -- does this look like a reasonable workaround for the situation I describe above? Do you know why the buildbot behaviour differs on this platform?

Comment 24

[Michael Shal [:mshal]]

I think that's a reasonable thing to do, though a comment explaining that we can inherit MOZ_SIGN_CMD from buildbot would be helpful. I believe it's coming from this line in buildbotcustom: http://hg.mozilla.org/build/buildbotcustom/file/0ba34e9310e9/process/factory.py#l460

You should probably set enable_signing=False in 32_artifact.py as well so the 32 & 64-bit variants are consistent with each other.

Comment 25

[Michael Shal [:mshal]]

(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #11)
> Comment on attachment 8784884 [details]
> > +        self.info("Updating self.config with the following from %s:" % variant_cfg_path)
> 
> Please use ''.format()

I'm curious, why is .format() preferable? Looking through python/mozbuild code as an example, we seem to have a healthy mix of both, though '... %s' % foo is favored 6:1 over ''.format() if my greps are to be believed.

Comment 26

[Michael Shal [:mshal]]

Also, a log message saying that MOZ_SIGN_CMD was cleared because enable_signing=False in the mozharness config would be helpful :)

Comment 27

[Armen [:armenzg]]

Hi mshal, here's what I've got from asking around:

11:15 AM <armenzg_brb> why is '{}'.format(variable) preferred over '%s' % variable?
11:20 AM <co60ca> armenzg: It's the new format. That would be my guess.
11:29 AM <@ted> armenzg: yeah, i dunno that it makes much difference in that case
11:29 AM <armenzg> I recall somebody recommending it to me on a review and I have been using it since
11:29 AM <@ted> armenzg: if you've got a more complicated string .format is nicer, like 'Hello {name}, it is {day}'.format(name='Armen', day='Friday')
11:30 AM <@ted> or you can just use positionals like {0} {1}
11:30 AM <armenzg> I always forget the value of it
11:31 AM <maja_zf> is % going away in python 3? gotta be prepared ;)
11:32 AM <ato> Don’t worry, we’ll never make it to Python 3 (-:
11:33 AM <@ted> hah
11:34 AM <@ted> armenzg: i still use %s a lot :)
11:34 AM <@ted> just habit
11:35 AM <ato> Same here. Don’t think I’ve ever used .format().
11:36 AM <wlach> we mostly use format in treeherder
11:36 AM <ato> Which is fine. Because the default /usr/bin/env python is going to be 2.7 into the foreseeable future unless OSes want to break the entire world.
11:36 AM <wlach> it's actually a bit better once you get used to it
11:36 AM <wlach> I believe fedora's default python is now 3
11:37 AM <ato> How’s that going for them? I would’ve thought that to be impossible.
11:37 AM <co60ca> You can also do "{0} {0}".format("A") which is useful. Arch's default is python3 as well.
11:38 AM <armenzg> ato: I'm finding more and more cases where I would prefer having Python3 available
11:38 AM <armenzg> I'm trying to make my code Python3 compatible as a habit
11:38 AM <armenzg> I want some of my tools to use parallelization from Python 3
11:39 AM <armenzg> since my tools are not consumed by others

Comment 28

[Michael Shal [:mshal]]

Thanks! I haven't made much use of .format() myself, perhaps it is time to learn...

Comment 29

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74194/diff/3-4/

Comment 30

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784885 [details]
Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74196/diff/3-4/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f770a12324d7
Make mozharness build scripts trigger artifact builds when specified via try syntax; r=armenzg
https://hg.mozilla.org/integration/autoland/rev/4468d50fd021
Get forced artifact build working in TaskCluster vs Buildbot; r=armenzg

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3344604&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/54d81bfbd901


12:37 PM <maja_zf> KWierso|afk: the stack trace points to my changes (mozharness/mozharness/mozilla/testing/try_tools.py", line 108, in _extract_try_message). that code path should not get hit outside of try

Comment 33

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784884 [details]
Bug 1278698 - Make mozharness build scripts trigger artifact builds when specified via try syntax;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74194/diff/4-5/

Comment 34

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784885 [details]
Bug 1278698 - Get forced artifact build working in TaskCluster vs Buildbot;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74196/diff/4-5/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd82d664a18c
Make mozharness build scripts trigger artifact builds when specified via try syntax; r=armenzg
https://hg.mozilla.org/integration/autoland/rev/ac7f8d395735
Get forced artifact build working in TaskCluster vs Buildbot; r=armenzg

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd82d664a18c
https://hg.mozilla.org/mozilla-central/rev/ac7f8d395735