Closed Bug 1304998 Opened 3 years ago Closed 3 years ago

enable taskcluster windows opt pgo builds (tier 2)

Categories

(Release Engineering :: General, defect)

Unspecified
Windows
defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
Tracking Status
firefox52 --- fixed

People

(Reporter: grenade, Assigned: grenade)

Details

Attachments

(2 files, 1 obsolete file)

somehow we neglected to add pgo builds when enabling debug and opt on taskcluster windows. add support for pgo.
Assignee: nobody → rthijssen
note: in order to test the changes above on try (where we haven't got syntax for pgo), i just added the "enable-pgo" option to the opt builds which are identical to pgo in how they are triggered with the exception of that command line switch being added to the mozharness call. the commit in the review only differs (from the one on try) by not having the enable-pgo options set for the win32-opt and win64-opt builds, only win32-pgo and win64-pgo.
the try test is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b055e05fa53205e842989bd0da3317d0565a9d5
Comment on attachment 8794130 [details]
Bug 1304998 - enable tc win pgo;

https://reviewboard.mozilla.org/r/80714/#review79424

::: taskcluster/ci/build/windows.yml:41
(Diff revision 1)
>          using: mozharness
>          script: mozharness/scripts/fx_desktop_build.py
>          config:
>              - builds/taskcluster_firefox_win32_opt.py
>  
> +win32/pgo:

We fix this issue for Linux builds with

https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/build_attrs.py#22
        if build_type == 'pgo':
            build_platform = build_platform + '-pgo'
            build_type = 'opt'

        attributes = job.setdefault('attributes', {})
        attributes.update({
            'build_platform': build_platform,
            'build_type': build_type,
        })
        
The try syntax is matching on the build_platform and build_type (which only allows "d"ebug and "o"pt).  But the treeherder platform is linux64/pgo.  It's confusing, that's why there's bug 1286086

::: taskcluster/taskgraph/transforms/gecko_v2_whitelist.py:69
(Diff revision 1)
>      'win32-debug',
>      'win32-opt',
> +    'win32-pgo',
>      'win64-debug',
>      'win64-opt',
> +    'win64-pgo',

These match the index routes used in Buildbot?  I'm contractually obligated to ask ;)

::: taskcluster/taskgraph/transforms/job/mozharness.py:190
(Diff revision 1)
> +    for option in run.get('options', []):
> +        mh_command.append('--' + option)

++
Attachment #8794130 - Flags: review?(dustin) → review+
Comment on attachment 8794130 [details]
Bug 1304998 - enable tc win pgo;

https://reviewboard.mozilla.org/r/80714/#review79424

> We fix this issue for Linux builds with
> 
> https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/build_attrs.py#22
>         if build_type == 'pgo':
>             build_platform = build_platform + '-pgo'
>             build_type = 'opt'
> 
>         attributes = job.setdefault('attributes', {})
>         attributes.update({
>             'build_platform': build_platform,
>             'build_type': build_type,
>         })
>         
> The try syntax is matching on the build_platform and build_type (which only allows "d"ebug and "o"pt).  But the treeherder platform is linux64/pgo.  It's confusing, that's why there's bug 1286086

I believe the same transform is working for the Windows builds (build_platform should just be win32, win64)

> These match the index routes used in Buildbot?  I'm contractually obligated to ask ;)

I think we're safe:
https://tools.taskcluster.net/index/#gecko.v2.mozilla-central.latest.firefox/gecko.v2.mozilla-central.latest.firefox.win32-pgo
https://tools.taskcluster.net/index/#gecko.v2.mozilla-central.latest.firefox/gecko.v2.mozilla-central.latest.firefox.win64-pgo
> I believe the same transform is working for the Windows builds (build_platform should just be win32, win64)

Silly me, you're right of course :)
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attached patch correction to TH platform name (obsolete) — Splinter Review
I managed to miss an important platform distinction in my earlier patch. This corrects that error.
Attachment #8796419 - Flags: review?(dustin)
Attachment #8796419 - Attachment is patch: true
Attachment #8796419 - Flags: review?(dustin) → review+
Attachment #8796419 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f0aae2c2bdd0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
please also apply the second patch in this bug, which adds a correction to the first patch
https://bug1304998.bmoattachments.org/attachment.cgi?id=8796434
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c95282b2888
correct TH pgo platform name; r=dustin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c95282b2888
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.