1362148 - Consolidate generate-build-stats into build mozharness action

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

Currently generate-build-stats is a separate mozharness action from build. This means that each mozharness script/config has to opt into generate-build-stats.

Metrics collection should be enabled by default. Let's consolidate that mozharness action into build so it is always called. We can provide a config option or write other logic to disable metrics collection when appropriate.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1362148 - Always collect build metrics;

Previously, mozharness defined a separate action to collect build
metrics. This required the script and/or config to define that
action.

Metrics collection for CI is important. So it should be enabled by
default.

This commit changes the "build" action/method to always call the
metrics collection function after successful build. References to
the "generate-build-stats" action have been removed because it is
redundant.

A side-effect of this change is we may generate build metrics where
we weren't before. This could lead to e.g. duplicate entries in some
Perfherder series. Let's see what breaks ;)

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

https://reviewboard.mozilla.org/r/136294/#review139408

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:1991
(Diff revision 1)
>                      firefox_version, c.get('platform'), ext)
>                  if os.path.exists(os.path.join(dist_dir, name)):
>                      packageName = name
>                      break
>              else:
>                  self.fatal("could not determine packageName")

I hit this line on OS X opt BB builds: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=96708632

This is due to packageFilename and MOZ_SIMPLE_PACKAGE_NAME not being set.

I'm, uh, not sure how to fix this. I think if I set MOZ_SIMPLE_PACKAGE_NAME that other things will break due to renaming the .dmg.

Since the failure is isolated to this one build, I'm tempted to add a config flag to disable metrics and just exclude this one build configuration.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1362148 - Call generate_build_props() as part of build();

Previously, this ran during postflight_build(). The magic postflight_*
methods are called automagically by BaseScript.run_action() and are
only called if the main action method didn't raise. So there should
be no functional difference with this commit.

The reason I changed this is that a subsequent commit will perform
metrics generation from build() and without the build properties
file loaded, at least the OS X 64 opt buildbot build doesn't have
packageFilename defines, which breaks metrics collection.

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

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

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

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

https://reviewboard.mozilla.org/r/136294/#review139408

> I hit this line on OS X opt BB builds: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=96708632
> 
> This is due to packageFilename and MOZ_SIMPLE_PACKAGE_NAME not being set.
> 
> I'm, uh, not sure how to fix this. I think if I set MOZ_SIMPLE_PACKAGE_NAME that other things will break due to renaming the .dmg.
> 
> Since the failure is isolated to this one build, I'm tempted to add a config flag to disable metrics and just exclude this one build configuration.

I think I fixed this in revision 2 by forcing the build properties file to be read as part of build(). The try push should confirm or deny.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

https://reviewboard.mozilla.org/r/136294/#review141756

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

https://reviewboard.mozilla.org/r/136342/#review141758

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c302dc40f24c -d 508966268fab: rebasing 395245:c302dc40f24c "Bug 1362148 - Call generate_build_props() as part of build(); r=ted"
merging testing/mozharness/mozharness/mozilla/building/buildbase.py
rebasing 395246:d54eb59af273 "Bug 1362148 - Always collect build metrics; r=ted" (tip)
merging taskcluster/ci/build/android.yml
merging taskcluster/ci/build/linux.yml
merging testing/mozharness/configs/builds/releng_base_android_64_builds.py
merging testing/mozharness/configs/builds/releng_base_linux_32_builds.py
merging testing/mozharness/configs/builds/releng_base_linux_64_builds.py
merging testing/mozharness/configs/builds/releng_base_mac_64_builds.py
merging testing/mozharness/configs/builds/releng_base_mac_64_cross_builds.py
merging testing/mozharness/configs/builds/releng_base_windows_32_builds.py
merging testing/mozharness/configs/builds/releng_base_windows_64_builds.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/32_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/32_qr_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_add-on-devel.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_artifact.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_asan.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_asan_and_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_asan_tc.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_asan_tc_and_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_code_coverage.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_debug_artifact.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_qr_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_stat_and_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_stat_and_opt.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_stylo.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_stylo_debug.py
merging testing/mozharness/configs/builds/releng_sub_linux_configs/64_valgrind.py
merging testing/mozharness/configs/builds/releng_sub_mac_configs/64_add-on-devel.py
merging testing/mozharness/configs/builds/releng_sub_mac_configs/64_cross_debug.py
merging testing/mozharness/configs/builds/releng_sub_mac_configs/64_cross_qr_debug.py
merging testing/mozharness/configs/builds/releng_sub_mac_configs/64_debug.py
merging testing/mozharness/configs/builds/releng_sub_mac_configs/64_stat_and_debug.py
merging testing/mozharness/configs/builds/releng_sub_windows_configs/32_add-on-devel.py
merging testing/mozharness/configs/builds/releng_sub_windows_configs/32_debug.py
merging testing/mozharness/configs/builds/releng_sub_windows_configs/32_stat_and_debug.py
merging testing/mozharness/configs/builds/releng_sub_windows_configs/64_add-on-devel.py
merging testing/mozharness/configs/builds/releng_sub_windows_configs/64_debug.py
merging testing/mozharness/configs/builds/taskcluster_firefox_win32_qr_debug.py
merging testing/mozharness/configs/builds/taskcluster_firefox_win32_qr_opt.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_32_addondevel.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_32_debug.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_32_opt.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_64_addondevel.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_64_debug.py
merging testing/mozharness/configs/builds/taskcluster_firefox_windows_64_opt.py
merging testing/mozharness/mozharness/mozilla/building/buildbase.py
merging testing/mozharness/scripts/fx_desktop_build.py
warning: conflicts while merging testing/mozharness/mozharness/mozilla/building/buildbase.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

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

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

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

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/387d8415d05e
Call generate_build_props() as part of build(); r=ted
https://hg.mozilla.org/integration/autoland/rev/c9812dd7d27a
Always collect build metrics; r=ted

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5b0bd6714d9f -d c9812dd7d27a: rebasing 395307:5b0bd6714d9f "Bug 1362148 - Call generate_build_props() as part of build(); r=ted"
merging testing/mozharness/mozharness/mozilla/building/buildbase.py
warning: conflicts while merging testing/mozharness/mozharness/mozilla/building/buildbase.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/387d8415d05e
https://hg.mozilla.org/mozilla-central/rev/c9812dd7d27a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for breaking a bunch of Android jobs like checkstyle.
https://treeherder.mozilla.org/logviewer.html#?job_id=98727326&repo=autoland

https://hg.mozilla.org/mozilla-central/rev/1e2fe13035e13b7b4001ade3b48f226957cef5fc

Comment 17

[Gregory Szorc [:gps]]

Attachment: Bug 1362148 - Extract package metrics collection to own function;

A subsequent commit will make all this code conditional. Rather than
indent the world, it is easier to conditionally call a function.

A benefit of the new code is that we skip some code for debug builds,
which is one less thing that can break.

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

Comment 18

[Gregory Szorc [:gps]]

Attachment: Bug 1362148 - Don't collect package metrics for some Android builds;

We're about to enable metrics collection for all builds. There are some
Android build configurations that use buildbase.py but don't create a
package. So we need a way to conditionally obtain package metrics.

We could change package metrics collection to no-op if a package file
can't be found. However, that has a risk that a future change could
break metrics collection and we wouldn't necessarily find out. I like
things that fail fast.

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

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

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

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

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8867340 [details]
Bug 1362148 - Extract package metrics collection to own function;

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

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8867341 [details]
Bug 1362148 - Don't collect package metrics for some Android builds;

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

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

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

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 8867340 [details]
Bug 1362148 - Extract package metrics collection to own function;

https://reviewboard.mozilla.org/r/138862/#review149902

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8867341 [details]
Bug 1362148 - Don't collect package metrics for some Android builds;

https://reviewboard.mozilla.org/r/138864/#review149904

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8864699 [details]
Bug 1362148 - Call generate_build_props() as part of build();

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

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8867340 [details]
Bug 1362148 - Extract package metrics collection to own function;

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

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8867341 [details]
Bug 1362148 - Don't collect package metrics for some Android builds;

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

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8864620 [details]
Bug 1362148 - Always collect build metrics;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136294/diff/6-7/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f5e82dbe1b6
Call generate_build_props() as part of build(); r=ted
https://hg.mozilla.org/integration/autoland/rev/e958856b6bfc
Extract package metrics collection to own function; r=ted
https://hg.mozilla.org/integration/autoland/rev/b227363d66bb
Don't collect package metrics for some Android builds; r=ted
https://hg.mozilla.org/integration/autoland/rev/cb99c52cf386
Always collect build metrics; r=ted

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4f5e82dbe1b6
https://hg.mozilla.org/mozilla-central/rev/e958856b6bfc
https://hg.mozilla.org/mozilla-central/rev/b227363d66bb
https://hg.mozilla.org/mozilla-central/rev/cb99c52cf386

Comment 34

[Ryan VanderMeulen [:RyanVM]]

As far as I can tell, this again broke all the same Android jobs it broke in comment 16.

https://treeherder.mozilla.org/logviewer.html#?job_id=104854887&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=104854881&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=104854873&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=104854871&repo=mozilla-inbound

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Nevermind, I filed bug 1370539 to track that.