1444220 - Extend artifact builds to include Thunderbird

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Geoff Lankow (:darktrojan)]

The build code can be made to download Thunderbird artifacts instead of Firefox artifacts by changing the relevant strings [1]. This bug is about doing so in a way that doesn't also break everything for Firefox.

[1] https://gist.github.com/darktrojan/4e24e58658a682a9d34be408dec7c852#file-m-c-diff

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1444220-artifact-mc-1.diff

Here's my current m-c patch. With this and some c-c changes hacks, I have a working artifact build.

Firefox people, do you have any suggestions as to how I might work this in, in a way that would be acceptable?

Comment 2

[Geoff Lankow (:darktrojan)]

CC'ing a bunch of people who might be able to help me here.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Lankow (:darktrojan) from comment #1)

Created attachment 9051612 [details] [diff] [review]
1444220-artifact-mc-1.diff

Here's my current m-c patch. With this and some c-c changes hacks, I have a working artifact build.

Firefox people, do you have any suggestions as to how I might work this in, in a way that would be acceptable?

The artifacts.py stuff is fine -- just generalize it such that the appname and the index is part of the configuration in mach artifact (which is where you have access to the build config). Default it to the Firefox case and I see no reason we can't land that.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1444220-artifact-mc-2.diff

I doubt this is quite what you had in mind, but it's taken me so long to get this far that I might as well post it. My changes feel a bit hacky, but they do work.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9060682 [details] [diff] [review]
1444220-artifact-mc-2.diff

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

Honestly, this looks way better than I expected it too; I'm pleased at how nicely the assumptions baked into this code -- and there are many -- generalize smoothly.  It's not particularly hacky but I would prefer to avoid the blanket substitution.

:glandium, do you want to see different things?  Would you take this more-or-less as is?

::: python/mozbuild/mozbuild/artifacts.py
@@ +125,2 @@
>  
>  class ArtifactJob(object):

A class comment about what `trust_domain` and `product` mean here would be helpful.

@@ +473,5 @@
>                  'plugin-container.app/Contents/MacOS/plugin-container',
>                  'updater.app/Contents/MacOS/org.mozilla.updater',
>                  # 'xpcshell',
>                  'XUL',
> +                'thunderbird',

It would be nice if this was in the Thunderbird* hierarchy you create below.

@@ +586,5 @@
> +class LinuxThunderbirdArtifactJob(LinuxArtifactJob):
> +    trust_domain = 'comm'
> +    product = 'thunderbird'
> +
> +    package_artifact_patterns = {

This isn't the end of the world, but it would also be nice to use the substitution `{product}/XUL.dylib` and then make
```
   @property
   def package_artifact_patterns(self) {
        p.format(product=self.product) for p in ...
   }
```
That is, rather than substitute a special keyword, bake the keyword into the definitions and then process the definitions depending on the product.

Comment 6

[Geoff Lankow (:darktrojan)]

(In reply to Nick Alexander :nalexander [he/him] from comment #5)

Comment on attachment 9060682 [details] [diff] [review]
1444220-artifact-mc-2.diff

Review of attachment 9060682 [details] [diff] [review]:

Honestly, this looks way better than I expected it too; I'm pleased at how
nicely the assumptions baked into this code -- and there are many --
generalize smoothly. It's not particularly hacky but I would prefer to
avoid the blanket substitution.

:glandium, do you want to see different things? Would you take this
more-or-less as is?

::: python/mozbuild/mozbuild/artifacts.py
@@ +125,2 @@

class ArtifactJob(object):

A class comment about what trust_domain and product mean here would be
helpful.

@@ +473,5 @@

             'plugin-container.app/Contents/MacOS/plugin-container',
             'updater.app/Contents/MacOS/org.mozilla.updater',
             # 'xpcshell',
             'XUL',

•            'thunderbird',
  

It would be nice if this was in the Thunderbird* hierarchy you create below.

Yes, I did that at first and then changed it to this way figuring it looked tidier. I guess since I've done Linux and Windows by overriding I should do Mac the same way.

@@ +586,5 @@

+class LinuxThunderbirdArtifactJob(LinuxArtifactJob):

• trust_domain = 'comm'
• product = 'thunderbird'
• package_artifact_patterns = {

This isn't the end of the world, but it would also be nice to use the
substitution {product}/XUL.dylib and then make

   @property
   def package_artifact_patterns(self) {
        p.format(product=self.product) for p in ...
   }

That is, rather than substitute a special keyword, bake the keyword into the
definitions and then process the definitions depending on the product.

Again, I thought about doing it that way but it made the for loop really ugly. Using a property is a good idea way to fix that, that I hadn't considered.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1444220-artifact-mc-3.diff

Updated to reflect :nalexander's comments.

Comment 8

[Geoff Lankow (:darktrojan)]

Oh, except for the bit about adding comments.

A class comment about what trust_domain and product mean here would be helpful.

I'm not actually sure what the trust domain is (or where I got the name from).

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 9061215 [details] [diff] [review]
1444220-artifact-mc-3.diff

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

::: python/mozbuild/mozbuild/artifacts.py
@@ +599,5 @@
> +    trust_domain = 'comm'
> +    product = 'thunderbird'
> +
> +    _package_artifact_patterns = {
> +        p.replace('firefox', 'thunderbird')

What was the point of the whole '{product}' thing if you just .replace()?

@@ +1351,5 @@
>                    'revision': revision},
>                   'Will only accept artifacts from a pushhead at {revision} '
>                   '(matched revset "{revset}").')
>          # Include try in our search to allow pulling from a specific push.
> +        pushheads = [(list(self._candidate_trees) + ['try'], revision)]

don't you want try-comm-central instead of try?

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1444220-artifact-mc-4.diff

As well as fixing the comments, I did some further refactoring.

Just to prove that it all still works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9039e6ddd5e0bb66e5bb2b9c4b6c30c5ec7027b9
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a18a425e6073ee4ad5bba48da5969d540d86d891

Comment 11

[Geoff Lankow (:darktrojan)]

Regarding the list of candidate trees, comm-beta and comm-esr68 will definitely be useful in the future, should I just add them now? I see that neither release or ESR trees are listed for Firefox builds, but I guess that's because there's not much going on there for 99.9% of Firefox developers.

Comment 12

[Geoff Lankow (:darktrojan)]

Adding .shippable is currently breaking the OS X Thunderbird builds, but I think we've still got issues with that on "real" builds, so I'm ignoring it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c64f610ccf76
Extend artifact builds to include Thunderbird. r=glandium

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c64f610ccf76