1407672 - Support docker-image and toolchains by-product in l10n kind

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Nick Alexander :nalexander [he/him]]

Bug 1396098 migrated the Android builds to the android-build Docker image, but the l10n tasks ignore the dependent-tasks Docker image.  For Bug 1352599, I'm introducing a requirement (proguard.jar) that will only be available as a toolchain dependency.

This ticket tracks adding support for these options in the l10n kind.

It would be best to inherit the `docker-image` and set of `toolchains` from the underlying `dependent-task`, but that appears to be difficult: by the time the l10n transform sees the dependent task, the docker image and set of toolchains have been processed.  The l10n transform would have to "reconstitute" the docker image changes and the set of toolchains; it would be very fragile.  I think it better to be explicit, reduce unexpected interactions, and repeat the information in the l10n leaf tasks.

Comment 1

[Nick Alexander :nalexander [he/him]]

Callek: #c0 talks about the use case I have in mind.  It's not hard to make this work (although it took a lot of trial and error to discover how to make it work!), but it's also not a pretty solution.  Patch incoming, but I'd really like to know if there's a way to pass through the data from the underlying dependent task.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1407672 - Pre: Add rsync for l10n repacks and interactive helpers in android-build image.

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

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

This approach allows to specify the `docker-image` and set of
`toolchains` to the l10n leaf jobs suing the `by-platform:` override
mechanism.

It would be better to inherit the `docker-image` and set of
`toolchains` from the underlying `dependent-task`, but that appears to
be difficult: by the time the l10n transform sees the dependent task,
the docker image and set of toolchains have been processed.  The l10n
transform would have to "reconstitute" the docker image changes and
the set of toolchains; it would be very fragile.  It's better to be
explicit, reduce unexpected interactions, and repeat the information
in the l10n leaf tasks.

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

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

https://reviewboard.mozilla.org/r/188424/#review193678

::: commit-message-540ab:14
(Diff revision 1)
> +be difficult: by the time the l10n transform sees the dependent task,
> +the docker image and set of toolchains have been processed.  The l10n
> +transform would have to "reconstitute" the docker image changes and
> +the set of toolchains; it would be very fragile.  It's better to be
> +explicit, reduce unexpected interactions, and repeat the information
> +in the l10n leaf tasks.

For reference, I personally prefer being explicit. I don't think the l10n repack process necessarily needs to happen on the same type of image/machine as does the build.

A case in point, I'd like to eventually get to the state where windows l10n repacks can happen on linux images, since they are faster/easier to spin up/down. But we certainly need windows built on windows instances.

::: taskcluster/ci/nightly-l10n/kind.yml:50
(Diff revision 1)
> +   docker-image:
> +      by-build-platform:
> +         default:
> +           in-tree: desktop-build
> +         android-api-16-l10n:
> +           in-tree: android-build

how about add `win.*: null` here and omit the explicit windows check below?

Additionally, I feel safe assuming "in-tree:" so we don't have to specify it.

::: taskcluster/ci/nightly-l10n/kind.yml:56
(Diff revision 1)
> +   toolchains:
> +      by-build-platform:
> +         default: []
> +         android-api-16-nightly:
> +            - android-sdk-linux
> +            - proguard-jar

As I said in IRC, I'm less happy about requiring an sdk/special toolchain to do a repack, since we explicitly mark this as --no-compile-environment, and if that flag doesn't work its probably a bigger issue in the job.

But I also don't admittedly understand what proguard-jar does, or if/why we need it in l10n repacks, so I'm stamping this piece for now.

::: taskcluster/taskgraph/transforms/l10n.py:104
(Diff revision 1)
>  
> +    # Docker image required for task.  We accept only in-tree images
> +    # -- generally desktop-build or android-build -- for now.
> +    Required('docker-image'): _by_platform(Any(
> +        # an in-tree generated docker image (from `taskcluster/docker/<name>`)
> +        {'in-tree': basestring},

If you take my suggestion this can be:

_by_platform(Any(basestring, None))

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8917435 [details]
Bug 1407672 - Pre: Add rsync for l10n repacks and interactive helpers in android-build image.

https://reviewboard.mozilla.org/r/188422/#review193680

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

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

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

https://reviewboard.mozilla.org/r/188424/#review193678

> For reference, I personally prefer being explicit. I don't think the l10n repack process necessarily needs to happen on the same type of image/machine as does the build.
> 
> A case in point, I'd like to eventually get to the state where windows l10n repacks can happen on linux images, since they are faster/easier to spin up/down. But we certainly need windows built on windows instances.

I updated the commit message with these considerations, and agree with the design choice.

> how about add `win.*: null` here and omit the explicit windows check below?
> 
> Additionally, I feel safe assuming "in-tree:" so we don't have to specify it.

I did the `win.*` thing -- thanks for that -- but didn't do the `in-tree:` thing, mostly 'cuz I didn't want to re-run the try pushes.

> As I said in IRC, I'm less happy about requiring an sdk/special toolchain to do a repack, since we explicitly mark this as --no-compile-environment, and if that flag doesn't work its probably a bigger issue in the job.
> 
> But I also don't admittedly understand what proguard-jar does, or if/why we need it in l10n repacks, so I'm stamping this piece for now.

I commented on this in the commit message.  Technically the SDK is required and the Proguard JAR is not, but I'd prefer to keep it for simplicity and consistency with the `--disable-compile-environment` flag.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917435 [details]
Bug 1407672 - Pre: Add rsync for l10n repacks and interactive helpers in android-build image.

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

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917435 [details]
Bug 1407672 - Pre: Add rsync for l10n repacks and interactive helpers in android-build image.

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

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

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

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

https://reviewboard.mozilla.org/r/188424/#review193750

::: taskcluster/ci/nightly-l10n/kind.yml:54
(Diff revision 4)
> +         android-api-16-nightly:
> +           in-tree: android-build
> +         win.*: null
> +   toolchains:
> +      by-build-platform:
> +         default: []

Callek: I realized that this is *blocking* the SDK and Proguard toolchain patches, so I moved the toolchain lines to those tickets.  That makes this smaller and allows it to land before those tickets.

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

https://reviewboard.mozilla.org/r/188424/#review194090

::: commit-message-540ab:4
(Diff revision 4)
> +Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs. r=Callek
> +
> +This approach allows to specify the `docker-image` and set of
> +`toolchains` to the l10n leaf jobs suing the `by-platform:` override

nit: using

::: taskcluster/ci/nightly-l10n/kind.yml:54
(Diff revision 4)
> +         android-api-16-nightly:
> +           in-tree: android-build
> +         win.*: null
> +   toolchains:
> +      by-build-platform:
> +         default: []

knowing that you're modifying this elsewhere and soon this is not blocking, however:

Generally I'd say leave out the `toolchains` call here, or at least don't do `by-build-platform` with just `default`.

I also warn that I *vaguely* recall adding a check that `default` isn't the only option, so please run this by a local `./mach taskgraph full` before landing to avoid a backout.

(That latter warning is why I marked this as an issue in reviewboard, feel free to resolve without any code change if it is fine)

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917435 [details]
Bug 1407672 - Pre: Add rsync for l10n repacks and interactive helpers in android-build image.

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

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

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

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

https://reviewboard.mozilla.org/r/188424/#review194090

> knowing that you're modifying this elsewhere and soon this is not blocking, however:
> 
> Generally I'd say leave out the `toolchains` call here, or at least don't do `by-build-platform` with just `default`.
> 
> I also warn that I *vaguely* recall adding a check that `default` isn't the only option, so please run this by a local `./mach taskgraph full` before landing to avoid a backout.
> 
> (That latter warning is why I marked this as an issue in reviewboard, feel free to resolve without any code change if it is fine)

Thanks, this is in fact was an issue.  I dropped the `toolchains:` block; it'll be part of the commits that use this functionality.

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8917436 [details]
Bug 1407672 - Add docker-image and toolchain support to l10n leaf jobs.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0138266bc70b
Pre: Add rsync for l10n repacks and interactive helpers in android-build image. r=Callek
https://hg.mozilla.org/integration/autoland/rev/7a93b841a26f
Add docker-image and toolchain support to l10n leaf jobs. r=Callek

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/0138266bc70b
https://hg.mozilla.org/mozilla-central/rev/7a93b841a26f