1253110 - Produce zip files for artifact builds to avoid client-side processing

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #1250991 +++

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Implement DeflatedFile.read() and ManifestFile.read(); r?glandium

BaseFile.read() isn't implemented because each file class needs to do
intelligent things. We're about to introduce a consumer that wants
both DeflatedFile.read() and ManifestFile.read(), so implement them.

As part of this, I snuck in a better error message saying which class
doesn't implement read(), since each class likely has a different
implementation.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

Currently, artifact builds download DMGs and process them into JAR files
containing a subset of the files they care about. This adds overhead.

This commit unblocks future work to stop doing that.

We introduce an "artifact_archive" build action. Given an output
filename, it reads the build configuration and produces a zip file
containing files relevant to artifact builds. This file is uploaded
as part of the automation results. In the future, artifact builds
can search for this file, download it, and extract it verbatim.
A goal is to make the client-side of artifact builds as dumb as

I'm not very happy with the amount of hackery in artifact_archive.py
regarding searching for binary files. I really wish these things came
more from moz.build files. I would appreciate alternative solutions
so we don't have to hard-code lists of files. (It's worth noting that
the client side of artifact builds currently codes a similar files
list. Once we have the client side of artifacts doing a simple
archive extract, we will have effectively moved that files list
from mach to packaging, which is closer to where it should be,
but not actually in moz.build, which is where I think it should
belong.)

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

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

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

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

https://reviewboard.mozilla.org/r/47307/#review44407

::: python/mozbuild/mozbuild/action/artifact_archive.py:27
(Diff revision 2)
> +def get_binaries(objdir):
> +    """Obtain binaries that should be packaged in the archive."""
> +    with open(os.path.join(objdir, 'binaries.json'), 'rb') as fh:
> +        binaries = json.load(fh)
> +
> +    # Ideally the data below comes from moz.build or some other source.

... like package-manifest.in?

::: python/mozbuild/mozbuild/action/artifact_archive.py:74
(Diff revision 2)
> +        # This is a bit hacky. We should record the proper path for app
> +        # binaries in the binaries JSON file.
> +        target = program['install_target']
> +        if target == 'dist/bin/browser':
> +            target = 'dist/bin'

Please file a bug and mark it here.

::: python/mozbuild/mozbuild/action/artifact_archive.py:118
(Diff revision 2)
> +        'components/components.manifest',
> +        'components/interfaces.xpt',
> +    )
> +
> +    finder = unpack.UnpackFinder(source)
> +    for p, f in finder.find('*'):

You should be able to do:
for p, f in finder.find('**/components/*'):
    if p.endswith(('.xpt, '.manifest')):
        yield p, f

::: python/mozbuild/mozbuild/action/artifact_archive.py:137
(Diff revision 2)
> +        if dest == 'bin/firefox':
> +            yield b'bin/firefox-bin', File(os.path.join(objdir, source))

why?

::: python/mozbuild/mozbuild/action/artifact_archive.py:140
(Diff revision 2)
> +        yield dest.encode('utf-8'), File(os.path.join(objdir, source))
> +
> +        if dest == 'bin/firefox':
> +            yield b'bin/firefox-bin', File(os.path.join(objdir, source))
> +
> +    for source, f in get_xpidl_files(os.path.join(objdir, 'dist', app_dir)):

If you take the programs from dist/bin, why are you taking the xpt files from dist/Nightly.app?In fact, I'd argue it'd be better to have the same kind of dist/bin after an artifact build than after a normal build, which means it should contain the multiple xpt files, not the linked ones.

::: python/mozbuild/mozbuild/action/artifact_archive.py:153
(Diff revision 2)
> +
> +def create_archive(build, output_path):
> +    if build.defines.get('XP_MACOSX'):
> +        app_dir = os.path.join(build.substs['MOZ_APP_NAME'],
> +                               build.substs['MOZ_MACBUNDLE_NAME'])
> +        files = dict(get_osx_files(build.topobjdir, app_dir))

what's the point of actively creating a dict to then create a list out of it (file.items()), when you could just keep and use the generator directly?

::: python/mozbuild/mozbuild/backend/common.py:577
(Diff revision 2)
> +        Binaries from some 3rd party build systems aren't exposed to moz.build.
> +        This should eventually be fixed as we rewrite build systems for 3rd
> +        party projects or come up with a separate mechanism for declaring the
> +        binaries. For now, work around the problem by defining the binaries
> +        manually.

Please mention this in the bug about moz.build'ifying nss

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8742568 [details]
MozReview Request: Bug 1253110 - Implement DeflatedFile.read() and ManifestFile.read(); r?glandium

https://reviewboard.mozilla.org/r/47305/#review44415

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Make package_re optional in artifact builds; r?mshal

Upcoming commits will introduce support for consuming a single artifact
containing all binary related files. We prepare for this by making the
package_re argument optional.

While we were here, we make the arguments named so we don't always have
to pass them.

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Construct artifact classes using named arguments; r?mshal

This will allow us to define any combination of arguments we wish.

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Pass binary artifacts regular expression for OS X builds; r?mshal

We don't do anything with it yet, including emitting the URL.

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Directly use binary artifact archives; r?mshal

The existing code for performing an artifact build downloads a package
and test archive and processes them into a new archive containing the
subset of relevant files.

Now that we produce artifact archives containing the final set of files
that would be in the processed archive, we can skip the client-side
processing and extract the binary artifact archive as is.

This commit leaves the artifact code in kind of a wonky state because
we still support the old processing mode because we only generate
binary artifact archives for OS X currently. Once we generate these
binary artifact archives for other platforms, a lot of code in
artifacts.py can be deleted.

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

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

Comment 11

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/47307/#review44407

> ... like package-manifest.in?

Are you suggesting I integrate the generation of this artifact archive with the packager [someday]?

Comment 12

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/47307/#review44407

> why?

This is for parity with the existing artifact filesystem layout. We /could/ do this on the client, but then the extract code wouldn't be as simple as "unzip to dist."

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

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

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8743424 [details]
MozReview Request: Bug 1253110 - Make package_re optional in artifact builds; r?mshal

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8743425 [details]
MozReview Request: Bug 1253110 - Construct artifact classes using named arguments; r?mshal

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8743426 [details]
MozReview Request: Bug 1253110 - Pass binary artifacts regular expression for OS X builds; r?mshal

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

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8743427 [details]
MozReview Request: Bug 1253110 - Directly use binary artifact archives; r?mshal

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

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8743424 [details]
MozReview Request: Bug 1253110 - Make package_re optional in artifact builds; r?mshal

https://reviewboard.mozilla.org/r/47745/#review44955

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8743425 [details]
MozReview Request: Bug 1253110 - Construct artifact classes using named arguments; r?mshal

https://reviewboard.mozilla.org/r/47747/#review44957

Comment 20

[Michael Shal [:mshal]]

Comment on attachment 8743426 [details]
MozReview Request: Bug 1253110 - Pass binary artifacts regular expression for OS X builds; r?mshal

https://reviewboard.mozilla.org/r/47749/#review44959

::: python/mozbuild/mozbuild/artifacts.py:406
(Diff revision 2)
>          'tests_re': 'public/build/firefox-(.*)\.common\.tests\.zip'}),
>      'linux64': (LinuxArtifactJob, {
>          'package_re': 'public/build/firefox-(.*)\.linux-x86_64\.tar\.bz2',
>          'tests_re': 'public/build/firefox-(.*)\.common\.tests\.zip'}),
>      'macosx64': (MacArtifactJob, {
> +        'binary_artifacts_re': 'public/build/firefox-(.*)\.binary-artifacts.zip',

\.zip for consistency (though it's unlikely to ever matter).

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 8743427 [details]
MozReview Request: Bug 1253110 - Directly use binary artifact archives; r?mshal

https://reviewboard.mozilla.org/r/47751/#review44971

::: python/mozbuild/mozbuild/artifacts.py:156
(Diff revision 2)
>          tests_artifact = None
>          for artifact in artifacts:
>              name = artifact['name']
> +            # Binary artifacts take precedence over package and tests artifacts.
> +            # Only emit binary artifact if wanted.
> +            if self._binary_artifacts_re:

Do we want to fallback to package_re if none of the artifacts match binary_artifacts_re? Though I see later that we only have one or the other set, so this is probably a non-issue.

::: python/mozbuild/mozbuild/artifacts.py:179
(Diff revision 2)
>              raise ValueError('Expected tests archive matching "{re}", but '
>                               'found none!'.format(re=self._tests_re))
>  
>      def process_artifact(self, filename, processed_filename):
> +        # Binary artifacts don't require any processing. Just return.
> +        if filename.endswith(BINARY_ARTIFACT_SUFFIX):

These kinds of checks tend to bite us as we add new outputs to the build process. Is there a way we can return both the artifact name and the type of artifact when the RE matches instead of having to look at the suffix?

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

https://reviewboard.mozilla.org/r/47307/#review44979

The packaging bits look fine, and I don't have anything to add for artifact_archive.py on top of what glandium suggested.

::: python/mozbuild/mozbuild/backend/common.py:588
(Diff revision 4)
> +        # No-op if no binaries defined. This handles cases where we aren't
> +        # building binaries, which includes some test cases.
> +        if not self._binaries.programs or not self._binaries.shared_libraries:
> +            return
> +
> +        NEW_PROGRAMS = [

Oof, adding this to the mozbuild backend seems way out of place. How difficult do you think it would be to add some stub variables to moz.build so that we could add 'certutil' and such to self.binaries? Then we could have a moz.build file for nss like:

THIRD_PARTY_PROGRAMS += ['certutil', ...]
THIRD_PARTY_LIBS += ['freebl3', ...]

and the backend could add it to the appropriate variables, but not actually emit any rules to compile them. Once we convert them to full moz.build, the backend mozbuild code shouldn't need to change.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

https://reviewboard.mozilla.org/r/47307/#review45659

::: python/mozbuild/mozbuild/action/artifact_archive.py:136
(Diff revision 4)
> +
> +    for dest, source in all_files.items():
> +        yield dest.encode('utf-8'), File(os.path.join(objdir, source))
> +
> +        if dest == 'bin/firefox':
> +            yield b'bin/firefox-bin', File(os.path.join(objdir, source))

You should add a comment as to why this is necessary, which you answered in comment 12, but that's better to put the reason in the code than having to dig bug comments.

::: python/mozbuild/mozbuild/action/artifact_archive.py:138
(Diff revision 4)
> +    for source, f in get_xpidl_files(os.path.join(objdir, 'dist', app_dir)):
> +        assert source.startswith('Contents/Resources/')
> +        dest = os.path.join('bin', source[len('Contents/Resources/'):])
> +        yield dest.encode('utf-8'), f

Same comment as before, you should take the xpidls from dist/bin, not dist/Nightly.app.

::: python/mozbuild/mozbuild/action/artifact_archive.py:152
(Diff revision 4)
> +def create_archive(build, output_path):
> +    if build.defines.get('XP_MACOSX'):
> +        build_app = build.substs['MOZ_BUILD_APP']
> +        app_dir = os.path.join(build.substs['MOZ_APP_NAME'],
> +                               build.substs['MOZ_MACBUNDLE_NAME'])
> +        files = dict(get_osx_files(build.topobjdir, build_app, app_dir))

Same comment as before, just remove the dict here and just do for p, f in sorted(files) below.

Comment 24

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Implement GeneratedFile.read(); r?glandium

This can get merged with the previous commit before landing.

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

Comment 25

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1253110 - Don't process XPIDLs during artifact builds on OS X; r?mshal

The new binary artifact archives on OS X contain the XPT and manifest
files derived from XPIDL processing. So we no longer need to process
XPIDL in this configuration.

This fixes a class of artifact build errors where the binaries could be
out of sync with in-tree XPIDL changes, leading to startup failures or
crashes. It also makes artifact builds several seconds faster due to not
having to process XPIDL files and write .xpt files.

The Makefile.in changes to exclude XPIDL processing are somewhat hacky.
But so is nearly everything related to XPIDL processing in make land.
I'm inclined to fix this in follow-up bugs.

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

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8742568 [details]
MozReview Request: Bug 1253110 - Implement DeflatedFile.read() and ManifestFile.read(); r?glandium

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

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

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

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8743424 [details]
MozReview Request: Bug 1253110 - Make package_re optional in artifact builds; r?mshal

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

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8743425 [details]
MozReview Request: Bug 1253110 - Construct artifact classes using named arguments; r?mshal

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

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8743426 [details]
MozReview Request: Bug 1253110 - Pass binary artifacts regular expression for OS X builds; r?mshal

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

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8743427 [details]
MozReview Request: Bug 1253110 - Directly use binary artifact archives; r?mshal

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

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 8748436 [details]
MozReview Request: Bug 1253110 - Implement GeneratedFile.read(); r?glandium

https://reviewboard.mozilla.org/r/50279/#review47461

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

https://reviewboard.mozilla.org/r/47307/#review47463

::: python/mozbuild/mozbuild/action/artifact_archive.py:41
(Diff revision 5)
> +    }
> +
> +    # This binary is quite large (~10 MB) and doesn't provide much value.
> +    # Ignore it for now.
> +    IGNORE_PROGRAMS = {
> +        'ReadNTLM',

If it were called TestReadNTLM, like other tests, you wouldn't have to exclude it manually.

::: python/mozbuild/mozbuild/action/artifact_archive.py:103
(Diff revision 5)
> +    finder = FileFinder(os.path.join(objdir, 'dist', 'plugins'), find_executables=False)
> +    for p, f in finder.find('*'):
> +        yield 'dist/plugins/%s' % p, 'dist/plugins/%s' % p
> +
> +    # Copy all files in gmp-* directories.
> +    finder = FileFinder(os.path.join(objdir, 'dist', 'bin'), find_executables=False)
> +    for p, f in finder.find('*'):
> +        if p.startswith('gmp-'):
> +            yield 'dist/bin/%s' % p, 'dist/bin/%s' % p

You could use the same finder for both. That would even simplify things:

finder = FileFinder(objdir, find_executables=False)
for p, f in finder.find('dist/plugins/*'):
    yield p, p

for p, f in finder.find('dist/bin/**/gmp-*'):
    yield p, p

::: python/mozbuild/mozbuild/action/artifact_archive.py:118
(Diff revision 5)
> +    # Find XPIDL files. These are .xpt and .manifest files in components/
> +    # directories. We need to also emit manifest files containing references
> +    # to these component manifests.

Come to think of it, why do you need the manifest files? The only thing relevant to xpt files in there are the interfaces entries, and the binary-component entries for the binary components that you may have copied. Everything else is irrelevant, and will come from the local build.
In fact, I'd argue the build itself should be adding the right manifest entries for the binary component and xpt it pulls from artifacts.

::: python/mozbuild/mozbuild/action/artifact_archive.py:163
(Diff revision 5)
> +        if dest == 'bin/firefox':
> +            yield b'bin/firefox-bin', File(os.path.join(objdir, source))

firefox-bin being a copy of firefox, why should it be in the artifacts, instead of the build copying/symlinking it?

::: python/mozbuild/mozbuild/action/artifact_archive.py:173
(Diff revision 5)
> +
> +
> +def create_archive(build, output_path):
> +    if build.defines.get('XP_MACOSX'):
> +        build_app = build.substs['MOZ_BUILD_APP']
> +        files = dict(get_osx_files(build.topobjdir, build_app))

Same comment as before, just remove the dict here and just do for p, f in sorted(files) below.

Comment 34

[Michael Shal [:mshal]]

Comment on attachment 8743427 [details]
MozReview Request: Bug 1253110 - Directly use binary artifact archives; r?mshal

https://reviewboard.mozilla.org/r/47751/#review47531

No new comments for this one. I'm still curious about the one open issue.

Comment 35

[Michael Shal [:mshal]]

Comment on attachment 8748437 [details]
MozReview Request: Bug 1253110 - Don't process XPIDLs during artifact builds on OS X; r?mshal

https://reviewboard.mozilla.org/r/50281/#review47559

Looks reasonable to me overall.

::: Makefile.in:133
(Diff revision 1)
>  $(foreach file,$(BUILD_BACKEND_FILES),$(eval $(call build_backend_rule,$(file))))
>  
>  default:: $(BUILD_BACKEND_FILES)
>  endif
>  
> +# XPIDLs aren't processes for OS X artifact builds.

What are your thoughts on putting the definition of no_xpidl into baseconfig.mk? Then it could be shared between here and xpcom/xpidl/Makefile.in. I'd recommend an all-caps variable name in this case.

::: Makefile.in:146
(Diff revision 1)
> -  $(addprefix dist/,branding idl include public private sdk xpi-stage) \
> +  $(addprefix dist/,branding include public private sdk xpi-stage) \
>    _tests \
>    $(NULL)
> +
> +ifndef no_xpidl
> +	install_manifests += dist/idl

You don't want to indent with a tab here. If the lines above it are part of a rule, the "install_manifests += dist/idl" would be treated as commands for that rule. This could be confusing if some lines are deleted or moved around.

Comment 36

[Michael Shal [:mshal]]

Comment on attachment 8742569 [details]
MozReview Request: Bug 1253110 - Produce binary artifact zip files for OS X builds; r?mshal, glandium

https://reviewboard.mozilla.org/r/47307/#review47571

I don't have anything new to add, but I'm still concerned about common.py

Comment 37

[Gregory Szorc [:gps]]

I'm not actively working on this. I'm guessing the patches have not aged well. It's probably a good idea to brush off the bit rot and land this so artifact builds become faster.