Closed Bug 1455570 Opened Last year Closed Last year

Build and publish TPS add-on

Categories

(Firefox :: Sync, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

Details

Attachments

(1 file)

I'd like to use the TPS add-on for testing sync integration between multiple clients. I have a prototype in the tests for iOS that combines XCUITests and TPS to test syncing bookmarks between Firefox for iOS and Firefox desktop. This prototype is currently mirroring the TPS add-on source code.

To avoid maintainig the source code in multiple places, I'd like to build and publish the TPS add-on so that my integration tests can simply download and use the latest version.

I'd like to achieve this by writing a mach command for building the add-on, and then creating a TaskCluster task to execute the command and publish the artifact to a location that can be accessed via some sort of 'latest' symlink.
(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #0)
> I'd like to use the TPS add-on for testing sync integration between multiple
> clients.

\o/
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

:dustin would you be able to provide some feedback on this patch? I'd like to have a tps.xpi artifact for every push to mozilla-central. I've created a TPS group in Treeherder because we may ultimately also have the TPS test suite running in TaskCluster.

The idea is that a test suite outside of mozilla-central can install the TPS add-on by downloading the latest artifact from TaskCluster. Ideally we'll have a URL such as https://tools.taskcluster.net/index/gecko.v2.mozilla-central.latest...tps.xpi

I've taken a few guesses at the yml file, and have been able to test this on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=343b1e48ec5343dc8e66d2af8309842e986f7e7b&selectedJob=174762137

I'm not sure about the location of tps.yml... If you can provide your thoughts, or bounce this to someone who could help, that would be great!
Attachment #8969634 - Flags: feedback?(dustin)
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

As written, this will "build" TPS for every platform on every push to every tree (whever any file changes).  Which is probably a little much for a zipfile with two files in it!

This doesn't make a lot of sense as a test, since it's not testing anything.

I'm not sure what TPS means.  I'm betting it's not https://addons.mozilla.org/en-US/android/addon/tps-firefox/ nor is it a report I'm going to need you to go ahead and come in on Saturday for..

One option here may be to build the extension entirely out-of-tree, e.g., from a github repo.  But if the extension's source is in-tree for some other reason, then it may make sense to add a new kind for this purpose, similar to the searchfox kind [1].  I'm not sure if we have other extension-building code in-tree, to be honest.  Maybe someone in releng does?

[1] http://gecko-docs.mozilla.org.s3.amazonaws.com/taskcluster/taskcluster/kinds.html and see the surrounding documentation, too
Attachment #8969634 - Flags: feedback?(dustin) → feedback-
TPS is a test suite used by sync. Some background is here https://developer.mozilla.org/en-US/docs/Mozilla/Projects/TPS_Tests, and the source is https://searchfox.org/mozilla-central/source/services/sync/tps/extensions/tps (for the extension) and here https://searchfox.org/mozilla-central/source/testing/tps (for the python driver).

Currently it is run in some automation using https://github.com/mozilla-services/sync-tps-setup every night, but it would be nice if there were a better setup that were easier to use on patches that have not landed yet.
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #4)
> Comment on attachment 8969634 [details]
> Bug 1455570 - Build and publish TPS add-on
> 
> As written, this will "build" TPS for every platform on every push to every
> tree (whever any file changes).  Which is probably a little much for a
> zipfile with two files in it!

Ah, I thought "platform: linux64/opt" would limit this to just that platform. We certainly don't need to build this for multiple platforms, and linux64 should be sufficient.

> This doesn't make a lot of sense as a test, since it's not testing anything.

There are TPS tests that use this extension, but they haven't been added to mach or TaskCluster yet. This is just packaging the extension for use outside of mozilla-central. As you say, it probably doesn't make sense as a test.

> I'm not sure what TPS means.  I'm betting it's not
> https://addons.mozilla.org/en-US/android/addon/tps-firefox/ nor is it a
> report I'm going to need you to go ahead and come in on Saturday for..

You mean you didn't get the memo? Yeah.. sorry for not providing more context.

> One option here may be to build the extension entirely out-of-tree, e.g.,
> from a github repo.  But if the extension's source is in-tree for some other
> reason, then it may make sense to add a new kind for this purpose, similar
> to the searchfox kind [1].  I'm not sure if we have other extension-building
> code in-tree, to be honest.  Maybe someone in releng does?

The source of the extenstion is in-tree. Do you think we should have a 'TPS' kind, or a more generic kind? As you've seen, the mach command is really just archiving a directory as a file with an xpi extension.

> 
> [1]
> http://gecko-docs.mozilla.org.s3.amazonaws.com/taskcluster/taskcluster/kinds.
> html and see the surrounding documentation, too

Thanks, I'll have a read through.
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

I've created a new kind (named xpi for now), but it's not building when I push to try. Could you take a look and let me know what I'm missing?
Attachment #8969634 - Flags: feedback?(dustin)
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review244744

This looks good.  A few possible tweaks below, but nothing major.

Ted, I don't feel qualified to say that this is the right way to get started building add-ons in the taskgraph, or even that it's a good idea.  What do you think?

::: taskcluster/ci/addon/kind.yml:13
(Diff revision 3)
> +    - taskgraph.transforms.build_attrs:transforms
> +    - taskgraph.transforms.job:transforms
> +    - taskgraph.transforms.task:transforms
> +
> +jobs:
> +    linux64-tps/opt:

Although treeherder needs a platform (and linux64/opt is a good "dumping ground" for non-platform tasks like this), the task itself need not be so named.  In particular, this appears to introduce a new platform "linux64-tps", which we definitely don't want to do!  The property name here will become job['name'] and be prefixed with the kind name, so something like 'tps-xpi' is a good choice.  Then the task label will be `addon-tps-xpi`.

::: taskcluster/ci/addon/kind.yml:26
(Diff revision 3)
> +            kind: build
> +            tier: 1
> +        run-on-projects: [mozilla-central]
> +        worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> +        worker:
> +            docker-image: {in-tree: lint}

Let's switch this to `in-tree: desktop-build`.  It won't make much difference to this task, but it's an image that's more likely to be already cached on this sort of worker.

::: taskcluster/ci/config.yml:63
(Diff revision 3)
>          'p': 'Partial generation'
>          'ps': 'Partials signing'
>          'Rel': 'Release promotion'
>          'Snap': 'Snap image generation'
>          'langpack': 'Langpack sigatures and uploads'
> +        'TPS': 'Sync tests'

This seems an odd group name, since the task in that group is not performing tests.  Is the idea that it will eventually also contain test jobs?
Attachment #8969634 - Flags: review?(dustin) → review+
Priority: -- → P3
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review244744

> Let's switch this to `in-tree: desktop-build`.  It won't make much difference to this task, but it's an image that's more likely to be already cached on this sort of worker.

I couldn't find a desktop-build docker image, and changing to this caused an exception:

IOError: [Errno 2] No such file or directory: u'/Users/dhunt/workspace/firefox/taskcluster/docker/desktop-build/Dockerfile'

> This seems an odd group name, since the task in that group is not performing tests.  Is the idea that it will eventually also contain test jobs?

Yes, the TPS tests are currently run in a Jenkins instance, but I'm interested in moving them into TaskCluster and Treeherder.
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review244744

> I couldn't find a desktop-build docker image, and changing to this caused an exception:
> 
> IOError: [Errno 2] No such file or directory: u'/Users/dhunt/workspace/firefox/taskcluster/docker/desktop-build/Dockerfile'

Indeed, I was looking at an old checkout.  It looks like debian7-amd64-build is the right choice, based on taskcluster/ci/docker-image/kind.yml.  Sorry about that!
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review244744

> Indeed, I was looking at an old checkout.  It looks like debian7-amd64-build is the right choice, based on taskcluster/ci/docker-image/kind.yml.  Sorry about that!

No worries, fixed!
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review247350

A few nits but overall I think this is OK. We don't have anything else like this currently--we build addons as part of the full build but that's mostly done with Makefile rules still. I know other teams have CI building addons, but I think they're mostly working on GitHub still. Maybe when the GitHub->m-c vendoring flow is up and running we could leverage this for building other addons. It should be straightforward to add your tests atop this so they test the latest Firefox with the latest version of the addon, which will be nice! We had a number of discussions on how to do that for folks building addons in other CI on GitHub but we didn't have great answers.

::: taskcluster/ci/addon/kind.yml:12
(Diff revision 5)
> +transforms:
> +    - taskgraph.transforms.job:transforms
> +    - taskgraph.transforms.task:transforms
> +
> +jobs:
> +    tps-xpi:

You should probably put a `when: files-changed:` block in this job definition so it doesn't have to run for every push. Here's an example:
https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/taskcluster/ci/source-test/python.yml#39

I'm sure it's quick to run but tasks still have nontrivial overhead and take up worker time.

::: taskcluster/ci/addon/kind.yml:32
(Diff revision 5)
> +            artifacts:
> +                - type: file
> +                  name: public/tps.xpi
> +                  path: /builds/worker/checkouts/gecko/tps-out/tps.xpi
> +        run:
> +            using: mach

You might want to make this use a sparse profile because otherwise each task run has to checkout every file in the repo (which is a lot). You can see what that looks like in the upload-generated-sources task:
https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/taskcluster/ci/upload-generated-sources/kind.yml#34
https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/build/sparse-profiles/upload-generated-sources

::: testing/tps/mach_commands.py:23
(Diff revision 5)
> +    @CommandArgument('--dest', default=None, help='Where to write add-on.')
> +    def build(self, dest):
> +        src = os.path.join(self.topsrcdir, 'services', 'sync', 'tps', 'extensions', 'tps')
> +        dest = os.path.join(dest or os.path.join(self.topobjdir, 'services', 'sync'), 'tps.xpi')
> +        root = os.path.splitext(dest)[0]
> +        shutil.make_archive(root, 'zip', src)

It might be nicer to use the mozpack code we have for creating zip files, as it does some fiddling to ensure that the output is reproducible. There's a pretty simple example here:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/zip.py

(not critical, but nice to have)
Attachment #8969634 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> ::: taskcluster/ci/addon/kind.yml:32
> (Diff revision 5)
> > +            artifacts:
> > +                - type: file
> > +                  name: public/tps.xpi
> > +                  path: /builds/worker/checkouts/gecko/tps-out/tps.xpi
> > +        run:
> > +            using: mach
> 
> You might want to make this use a sparse profile because otherwise each task
> run has to checkout every file in the repo (which is a lot). You can see
> what that looks like in the upload-generated-sources task:
> https://dxr.mozilla.org/mozilla-central/rev/
> f877359308b17e691209e1afb7193b8e86f175ce/taskcluster/ci/upload-generated-
> sources/kind.yml#34
> https://dxr.mozilla.org/mozilla-central/rev/
> f877359308b17e691209e1afb7193b8e86f175ce/build/sparse-profiles/upload-
> generated-sources

This is failing, so I must have done something wrong here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab17a7aa7274004f7a584e239126c1855c2a9f0&selectedJob=176949909
Flags: needinfo?(ted)
(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #20)
> This is failing, so I must have done something wrong here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=eab17a7aa7274004f7a584e239126c1855c2a9f0&selectedJob=1
> 76949909

Nevermind, I fixed it by switching from "using: mach" to "using: run-task" as in the example you linked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f51fc3ce51d6e47bb852d476f27a49f79ebe18fd

Now I have an issue regarding the unknown mach command, but I think I know how to fix this.
Flags: needinfo?(ted)
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

I think this is ready for another review.
Attachment #8969634 - Flags: review+ → review?(ted)
Comment on attachment 8969634 [details]
Bug 1455570 - Build and publish TPS add-on;

https://reviewboard.mozilla.org/r/238434/#review247512

Looks good, thanks!
Attachment #8969634 - Flags: review+
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e44aec7efe71
Build and publish TPS add-on; r=dustin,ted
https://hg.mozilla.org/mozilla-central/rev/e44aec7efe71
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.