1238320 - Fetch test binaries necessary to run mochitests and xpcshell tests in artifact based builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

This is straightforward: take the necessary bits of the "bin/" directory from the common.tests.zip and put them in "dist/bin".

I have patches to get this working reasonably well on Linux. There are probably a few more things to sort out on OS X.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

OS X only has a wrinkle for xpcshell tests: the xpcshell harness expects the xpcshell binary to be in the app bundle, so you always need to run "./mach build" after "./mach artifact install" to copy it over. For whatever reason the mochitest harness is happy to use the xpcshell binary from dist/bin.

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Manchester [:chmanchester] from comment #1)
> OS X only has a wrinkle for xpcshell tests: the xpcshell harness expects the
> xpcshell binary to be in the app bundle, so you always need to run "./mach
> build" after "./mach artifact install" to copy it over. For whatever reason
> the mochitest harness is happy to use the xpcshell binary from dist/bin.

--enable-artifact-builds does the "mach build" automatically at the right times.  (Otherwise, yes -- you need to run |mach artifact install| at just the right times.)

My expectation: you'll need to generalize the file downloading and caching to take multiple files (binaries, tests) and produce the post-processed ZIP file, and then you'll be able to write to wherever in dist/bin/ you want.  (You could add the xpcshell binary to both the root of the post-processed ZIP and the subdirectory, so it's in both places.  These inconsistencies are why all the ArtifactJob concrete instances vary.)

If we need to write to dist/_tests, that can be arranged, but I hope we don't need to.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1238320 - Part 1 (Linux): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1238320 - Part 2 (Mac): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1238320 - Part 3 (Windows): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 6

[Chris Manchester (limited bugmail, email directly)]

:gijs, if you have an opportunity to test this, that would be much appreciated.

I did some minor refactoring since I last tested on OS X, I'll re-test there before landing.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Chris Manchester [:chmanchester] from comment #6)
> :gijs, if you have an opportunity to test this, that would be much
> appreciated.
> 
> I did some minor refactoring since I last tested on OS X, I'll re-test there
> before landing.

First thing tomorrow morning. Thanks for jumping on this.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8706572 [details]
MozReview Request: Bug 1238320 - Part 1 (Linux): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

https://reviewboard.mozilla.org/r/30393/#review27109

I think this is expedient, and I'm not really against landing it, but I think we should investigate using a generic Finder and to unify the matching/whitelisting logic.  (Finder's aren't great for turning X into Y, but perhaps that's not necessary, or perhaps we can use Manifest instances.)  I'm going to kick this back to you for some preliminary investigation and look at the other bits.

::: python/mozbuild/mozbuild/artifacts.py:96
(Diff revision 1)
> +        # A subset of TEST_HARNESS_BINS in testing/mochitest/Makefile.in

nit: full sentences.

::: python/mozbuild/mozbuild/artifacts.py:107
(Diff revision 1)
> +        if isinstance(regexp, tuple):

Is there an advantage to being loose here?  I'd prefer to be strict and clear -- perhaps even `binaries_regexp, tests_regexp`.

::: python/mozbuild/mozbuild/artifacts.py:137
(Diff revision 1)
> +                    yield f.name, lambda: reader.extractfile(f), f.mode

Hmm.  What's the guarantee for the second tuple entry?

Is it that you can evaluate it and then call `.read()`?  If so, add a comment to the function.  (It's weird 'cuz these two things look very different...)

::: python/mozbuild/mozbuild/artifacts.py:168
(Diff revision 1)
> -            prefix = 'firefox/'
> +            'firefox/': ({

nit: explain the mapping:

"Keys are prefixes, first set is ... and second set is."

Can you try using a list of patterns to match and using the existing pattern matching code to unify this?

In fact, I see a `ComposedFinder` at
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/files.py#953

Perhaps a `JarFinder` is the right approach... and the questions I had earlier about the guarantee on the second tuple item might be addressed by extracting a `BaseFile`-instance, unifying this all.

::: python/mozbuild/mozbuild/artifacts.py:179
(Diff revision 1)
> +                '.so'

nit: add trailing comma.

::: python/mozbuild/mozbuild/artifacts.py:702
(Diff revision 1)
> -        if url:
> +        if urls:

Right now, we turn some distribution into a post-processed file.  You're doing that twice (two post-processed files), right?  I intended this to be a one-time thing, turning two binary archives into a single post-processed file, but there's no strong requirement it happen that way.

I am concerned that we're hiding errors (if only one file is found) and that we lost the ability to do `mach artifact install URL_OF_DISTRIBUTION`.

I also worry that the caching is no longer valid, since it's a one-to-many cache now.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8706573 [details]
MozReview Request: Bug 1238320 - Part 2 (Mac): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

https://reviewboard.mozilla.org/r/30395/#review27113

I'd like to see changes to part 1 to make things more uniform, which will likely change this; but if we decide to keep part 1 mostly as is, this looks fine to me.

::: python/mozbuild/mozbuild/artifacts.py:228
(Diff revision 1)
> +            try:

This is fragile.  Rather than installing from two things, can we explicitly handle binaries and then test binaries, and have a flag or something less special?

Perhaps make process_artifact delegate to two functions, or lift the delegation up to the generic framework (from the previous commit) and be more explicit about binaries and test binaries.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8706574 [details]
MozReview Request: Bug 1238320 - Part 3 (Windows): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

https://reviewboard.mozilla.org/r/30397/#review27115

Similar to part 2, this looks fine contingent on part 1.  Thanks for digging into this so quickly!

::: python/mozbuild/mozbuild/artifacts.py:332
(Diff revision 1)
> +                'bin/': (set('%s.exe' % n for n in WinArtifactJob.test_artifacts),

Could we not make `WinArtifactJob.test_artifacts` a getter that performed this mapping?  Too precious?

Comment 11

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/30393/#review27109

Thank you for the speedy review. I have a few replies/questions below.

> Hmm.  What's the guarantee for the second tuple entry?
> 
> Is it that you can evaluate it and then call `.read()`?  If so, add a comment to the function.  (It's weird 'cuz these two things look very different...)

It's a callable that evaluates to something that can be passed to JarWriter.add (which is itself very general). This is quite awkward because all of this conforms quite well to mozpack apis except for the tarfile. We can avoid unpacking the entire archive with this. Locally it saves about half the time it takes to process the archive.

> nit: explain the mapping:
> 
> "Keys are prefixes, first set is ... and second set is."
> 
> Can you try using a list of patterns to match and using the existing pattern matching code to unify this?
> 
> In fact, I see a `ComposedFinder` at
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/files.py#953
> 
> Perhaps a `JarFinder` is the right approach... and the questions I had earlier about the guarantee on the second tuple item might be addressed by extracting a `BaseFile`-instance, unifying this all.

The trouble is the tar file, I don't see an equivalent of JarFinder for tar files. To get to a Finder instance from the linux package I would end up making this like the OS X case, and use mozinstall and a FileFinder.

> Right now, we turn some distribution into a post-processed file.  You're doing that twice (two post-processed files), right?  I intended this to be a one-time thing, turning two binary archives into a single post-processed file, but there's no strong requirement it happen that way.
> 
> I am concerned that we're hiding errors (if only one file is found) and that we lost the ability to do `mach artifact install URL_OF_DISTRIBUTION`.
> 
> I also worry that the caching is no longer valid, since it's a one-to-many cache now.

Making this produce a single post-processed file seems reasonable, although I don't think I understand the advantage of that clearly. Wouldn't that be the case we lose the ablity to do `mach artifact install URL_OF_DISTRIBUTION`, because we would need both urls at once? I'll take a closer look at this either way.

Can you say a bit more about how this invalidates caching? That a single tree/job/rev will always map to the same set of artifact urls is a sane assumption.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/30393/#review27109

> Making this produce a single post-processed file seems reasonable, although I don't think I understand the advantage of that clearly. Wouldn't that be the case we lose the ablity to do `mach artifact install URL_OF_DISTRIBUTION`, because we would need both urls at once? I'll take a closer look at this either way.
> 
> Can you say a bit more about how this invalidates caching? That a single tree/job/rev will always map to the same set of artifact urls is a sane assumption.

I guess existing caches would be invalidated, but I don't see a way around that.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706572 [details]
MozReview Request: Bug 1238320 - Part 1 (Linux): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706573 [details]
MozReview Request: Bug 1238320 - Part 2 (Mac): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706574 [details]
MozReview Request: Bug 1238320 - Part 3 (Windows): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Gijs, these patches still need to be re-based once review goes a little further, don't bother testing for now.

Nick, I revised part 1 significantly this afternoon, addressing most of the comments. My only remaining question is about the advantage of producing a single post-processed file. I verified locally that `mach artifact install URL_OF_DISTRIBUTION` still works with the original patch in that you can run it for the tests url and then the package url and get the expected outcome.

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8706572 [details]
MozReview Request: Bug 1238320 - Part 1 (Linux): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

https://reviewboard.mozilla.org/r/30393/#review27615

Sorry this took a while to get back to.  This is much cleaner in my opinion.

Worth noting -- I've come around to your "list of URLs".  I'm still happy to name that list (like we have here) but it's a clean approach.

::: python/mozbuild/mozbuild/artifacts.py:144
(Diff revisions 1 - 2)
> -                    yield f.name, lambda: reader.extractfile(f), f.mode
> +    def process_package_artifact(self, filename, processed_filename):

Nice, much better.

::: python/mozbuild/mozbuild/artifacts.py:209
(Diff revisions 1 - 2)
> -                whitelist, suffix_whitelist = artifact_prefixes[found_prefix]
> +                            any(mozpath.match(f.name, p) for p in self.artifact_patterns)):

Locally, `mozpath.match(f.name, f.name)` succeeds, so I don't think you need the filename check.

::: python/mozbuild/mozbuild/artifacts.py:357
(Diff revisions 1 - 2)
> -    'macosx64': (MacArtifactJob, 'public/build/firefox-(.*)\.mac\.dmg'),
> +    'macosx64': (MacArtifactJob, 'public/build/firefox-(.*)\.mac\.dmg', None),

Ah, to follow.  I get it.

::: python/mozbuild/mozbuild/artifacts.py:110
(Diff revision 2)
> +    def __init__(self, binaries_re, tests_re, log=None):

You were using package/tests quite consistently, and I prefer that to binaries/tests.

::: python/mozbuild/mozbuild/artifacts.py:186
(Diff revision 2)
> -        with JarWriter(file=processed_filename, optimize=False, compress_level=5) as writer:
> +    artifact_patterns = {

nit: `package_artifact_patterns`, to be consistent with `test_artifact_patterns`.

Comment 18

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/30395/#review27617

Comment 19

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/30397/#review27619

::: python/mozbuild/mozbuild/artifacts.py:342
(Diff revision 2)
> -                if not f.filename.startswith(prefix):
> +                if not (f.filename in self.artifact_patterns or

Again, is the filename check necessary?  I'd hope not.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706572 [details]
MozReview Request: Bug 1238320 - Part 1 (Linux): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706573 [details]
MozReview Request: Bug 1238320 - Part 2 (Mac): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8706574 [details]
MozReview Request: Bug 1238320 - Part 3 (Windows): Download test binaries necessary to run xpcshell tests and mochitests in artifact builds.

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

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25c2925382da
https://hg.mozilla.org/integration/mozilla-inbound/rev/daaa097b37b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15e1923f8e2

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25c2925382da
https://hg.mozilla.org/mozilla-central/rev/daaa097b37b3
https://hg.mozilla.org/mozilla-central/rev/b15e1923f8e2