1503366 - add an aarch64 windows build to automation

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

...so people don't break the build and so that anybody adding features can at least get builds through try.  (Local builds may require a little extra effort, due to cross-compiling Windows builds being wonky.)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add a windows rust toolchain with aarch64 support

We could have added another target to the current nightly toolchain, but
I don't know what updating the date on that toolchain entails.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - enable WIN64_* for cargo in aarch64 cross-compiles

We need these bits for generic cross-compiles, so make sure they are
enabled on both x86 and aarch64.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - tooltool manifest for aarch64 windows builds

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add mozconfigs for aarch64 windows

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - mozharness configs for aarch64 windows

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 6 - taskcluster definitions for aarch64 windows

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 9024104 [details] [diff] [review]
part 1 - add a windows rust toolchain with aarch64 support

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

::: taskcluster/ci/toolchain/windows.yml
@@ +167,5 @@
> +        script: repack_rust.py
> +        arguments: [
> +            '--channel', 'nightly-2018-11-08',
> +            '--host', 'x86_64-pc-windows-msvc',
> +            '--target', 'x86_64-pc-windows-msvc',

You're creating the same as win64-rust-nightly, here, with the aarch64 target added. Why not just add that target to win64-rust-nightly?

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #7)
> You're creating the same as win64-rust-nightly, here, with the aarch64
> target added. Why not just add that target to win64-rust-nightly?

We could, and comment 1 notes that fact.  I wasn't sure what's involved in updating the date on the existing nightly repack, and it seemed simpler to keep the two separate for now.  Merging them would be a reasonable thing to do if desired.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024104 [details] [diff] [review]
part 1 - add a windows rust toolchain with aarch64 support

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

::: taskcluster/ci/toolchain/windows.yml
@@ +165,5 @@
> +    run:
> +        using: toolchain-script
> +        script: repack_rust.py
> +        arguments: [
> +            '--channel', 'nightly-2018-11-08',

I guess this is alright... does anything break if we try bumping win64-rust-nightly?

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024106 [details] [diff] [review]
part 3 - tooltool manifest for aarch64 windows builds

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

This would probably be better off in the directory with the rest of the win64 tooltool manifests,  browser/config/tooltool-manifests/win64. It's sort of confusing because this manifest doesn't seem to contain things truly specific to aarch64, just the updated vs version.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024107 [details] [diff] [review]
part 4 - add mozconfigs for aarch64 windows

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

Wow, this amount of near duplication makes me think we did something wrong here, maybe long before this patch. Other build types get by on including certain files while setting/overriding certain values, did we try that here and conclude it was worse than all this? The actual difference between these and regular msvc builds are setting the target, disabling some features, and fiddling with some toolchain paths, right?

::: browser/config/mozconfigs/win64-aarch64/common-win64
@@ +3,5 @@
> +ac_add_options --target=aarch64-windows-mingw32
> +ac_add_options --host=x86_64-pc-mingw32
> +
> +# No gn configs for webrtc yet.
> +ac_add_options --disable-webrtc

These landed earlier in the week.

Comment 12

[(Away)]

> > +# No gn configs for webrtc yet.
> > +ac_add_options --disable-webrtc
> 
> These landed earlier in the week.

There is still bustage for other reasons, so I guess it just needs a comment clarification.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024108 [details] [diff] [review]
part 5 - mozharness configs for aarch64 windows

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

::: testing/mozharness/configs/builds/taskcluster_base_win64_aarch64.py
@@ +8,5 @@
> +    "check_test_env": {
> +        'MINIDUMP_STACKWALK': '%(abs_src_dir)s\\win32-minidump_stackwalk.exe',
> +        'MINIDUMP_SAVE_PATH': os.path.join(os.getcwd(), 'public', 'build'),
> +    },
> +    'mozconfig_platform': 'win64-aarch64',

I believe there's a way to set this from taskcluster, either through "extra-config" or "mozconfig-variant" as seen in windows.yml, so we shouldn't need this patch.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024109 [details] [diff] [review]
part 6 - taskcluster definitions for aarch64 windows

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

I skimmed this, it looks fine. It might be a good idea to get a taskgraph peer to look at it as well.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #10)
> This would probably be better off in the directory with the rest of the
> win64 tooltool manifests,  browser/config/tooltool-manifests/win64. It's
> sort of confusing because this manifest doesn't seem to contain things truly
> specific to aarch64, just the updated vs version.

I guess I can do that, but I don't know how to make the build use a non-releng.manifest filename.  Do you have pointers on how to do that?  (We're not updating the MSVC version we use for the x86ish builds.)

(In reply to Chris Manchester (:chmanchester) from comment #11)
> Wow, this amount of near duplication makes me think we did something wrong
> here, maybe long before this patch. Other build types get by on including
> certain files while setting/overriding certain values, did we try that here
> and conclude it was worse than all this? The actual difference between these
> and regular msvc builds are setting the target, disabling some features, and
> fiddling with some toolchain paths, right?

I went with this because it obviously wasn't breaking any of the regular Windows builds.  I'm not really sure how much can be shared with the regular mozconfigs: common-{win64,opt} need different bits than the regular Windows builds, and so everything downstream of those needs to use different includes.  The MSVC configuration is completely different because it's a real cross-compilation scenario, not the x86 case where we can run target binaries on the host system.  What would you suggest sharing between our regular win64 configs and the aarch64 ones?

Comment 16

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> (In reply to Chris Manchester (:chmanchester) from comment #10)
> > This would probably be better off in the directory with the rest of the
> > win64 tooltool manifests,  browser/config/tooltool-manifests/win64. It's
> > sort of confusing because this manifest doesn't seem to contain things truly
> > specific to aarch64, just the updated vs version.
> 
> I guess I can do that, but I don't know how to make the build use a
> non-releng.manifest filename.  Do you have pointers on how to do that? 
> (We're not updating the MSVC version we use for the x86ish builds.)

Is that TOOLTOOL_MANIFEST set by taskcluster in windows.yml?

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9024107 [details] [diff] [review]
part 4 - add mozconfigs for aarch64 windows

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

::: browser/config/mozconfigs/win64-aarch64/debug
@@ +1,4 @@
> +. "$topsrcdir/build/mozconfig.win-common"
> +MOZ_AUTOMATION_L10N_CHECK=0
> +. "$topsrcdir/browser/config/mozconfigs/win64-aarch64/common-win64"
> +. "$topsrcdir/browser/config/mozconfigs/win64-aarch64/common-opt"

Why are we including common-opt in debug builds, unlike the other windows mozconfigs?

Comment 18

[Chris Manchester (limited bugmail, email directly)]

> (In reply to Chris Manchester (:chmanchester) from comment #11)
> > Wow, this amount of near duplication makes me think we did something wrong
> > here, maybe long before this patch. Other build types get by on including
> > certain files while setting/overriding certain values, did we try that here
> > and conclude it was worse than all this? The actual difference between these
> > and regular msvc builds are setting the target, disabling some features, and
> > fiddling with some toolchain paths, right?
> 
> I went with this because it obviously wasn't breaking any of the regular
> Windows builds.  I'm not really sure how much can be shared with the regular
> mozconfigs: common-{win64,opt} need different bits than the regular Windows
> builds, and so everything downstream of those needs to use different
> includes.  The MSVC configuration is completely different because it's a
> real cross-compilation scenario, not the x86 case where we can run target
> binaries on the host system.  What would you suggest sharing between our
> regular win64 configs and the aarch64 ones?

browser/config/mozconfigs/win64-aarch64/debug and browser/config/mozconfigs/win64-aarch64/common-opt seem like they're mostly shared. Maybe we can set some variable in the top-included mozconfig to trigger swapping out mozconfig.vs2017? Or maybe these should look a little more like the asan builds?

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #13)
> Comment on attachment 9024108 [details] [diff] [review]
> part 5 - mozharness configs for aarch64 windows
> 
> Review of attachment 9024108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozharness/configs/builds/taskcluster_base_win64_aarch64.py
> @@ +8,5 @@
> > +    "check_test_env": {
> > +        'MINIDUMP_STACKWALK': '%(abs_src_dir)s\\win32-minidump_stackwalk.exe',
> > +        'MINIDUMP_SAVE_PATH': os.path.join(os.getcwd(), 'public', 'build'),
> > +    },
> > +    'mozconfig_platform': 'win64-aarch64',
> 
> I believe there's a way to set this from taskcluster, either through
> "extra-config" or "mozconfig-variant" as seen in windows.yml, so we
> shouldn't need this patch.

Tom, do you know whether this is possible to set this variable from taskcluster itself, or do we have to continue to specify it in the mozharness config?  (I believe this is the only difference between our win64 and win64-aarch configs, so if we could set it from the taskcluster job definition, that would be one less piece to include in mozharness.)

Comment 20

[Tom Prince [:tomprince]]

Comment on attachment 9024109 [details] [diff] [review]
part 6 - taskcluster definitions for aarch64 windows

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

::: taskcluster/ci/build/windows.yml
@@ +981,5 @@
> +            - builds/releng_base_firefox.py
> +            - builds/taskcluster_base_windows.py
> +            - builds/taskcluster_base_win64_aarch64.py
> +        extra-config:
> +            stage_platform: win64

You can add `mozconfig_platform` here (and add a `extra-config` stanza with `mozconfig_platform` above) you shouldn't need to modify the mozharness configs. The stanza is passed via `EXTRA_MOZHARNESS_CONFIG` as json, and overrides everything but the branch_specifics.py. 

Note that mozconfig-variant is passed the same way, but handled slightly specially, so the `by-release-type` logic needed for https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/taskcluster/ci/build/kind.yml#28-36 can be handled first.

Comment 21

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add mozconfigs for aarch64 windows

I looked into trying to common up the mozconfigs between aarch64 windows and
x86-64 windows and I honestly think it's not worth the trouble to separate out
the common parts.  We'd have to set up some sort of magic variable in each
common-win64 file, and then we'd have to carefully orchestrate what gets
included where...and at that point, you look at all the other duplication we
have across *all* of our mozconfigs and you wonder whether it's really worth
it.  I don't think it is.

I have reorganized things to minimize the diff versus the regular win64 files;
you might be able to get an idea of the scope of changes necessary by
eyeballing the diffs, and then thinking about what variables need to be
threaded where from where.

Comment 22

[Nathan Froyd [:froydnj]]

Attachment: part 5 - taskcluster definitions for aarch64 windows

With some of the changes chmanchester suggested for tooltool and setting up
mozconfig_platform.  We no longer need the mozharness changes, so this is part 5.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9026549 [details] [diff] [review]
part 4 - add mozconfigs for aarch64 windows

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

I'm still not quite convinced this needs its own directory, but like you say the mozconfigs we have aren't exactly well-factored. If we're confident this is doing what we need it to I guess it's fine.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5811255ebed4
part 1 - add a windows rust toolchain with aarch64 support; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef1c065654f
part 2 - enable WIN64_* for cargo in aarch64 cross-compiles; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/2526f6735ef5
part 3 - tooltool manifest for aarch64 windows builds; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/692342801503
part 4 - add mozconfigs for aarch64 windows builds; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/92c3a891c4f2
part 5 - taskcluster definitions for aarch64 windows builds; r=chmanchester

Comment 25

[(Away)]

Shoot, I am late to the party, but it might have been helpful to mark the mozconfigs as `hg cp` so that the changes stand out better (and the non-changes keep their blame).

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5811255ebed4
https://hg.mozilla.org/mozilla-central/rev/3ef1c065654f
https://hg.mozilla.org/mozilla-central/rev/2526f6735ef5
https://hg.mozilla.org/mozilla-central/rev/692342801503
https://hg.mozilla.org/mozilla-central/rev/92c3a891c4f2