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

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/

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 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 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 on attachment 8742568 [details] MozReview Request: Bug 1253110 - Implement DeflatedFile.read() and ManifestFile.read(); r?glandium https://reviewboard.mozilla.org/r/47305/#review44415

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/

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/

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/

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 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/

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]?

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 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 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 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 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 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 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 on attachment 8743425 [details] MozReview Request: Bug 1253110 - Construct artifact classes using named arguments; r?mshal https://reviewboard.mozilla.org/r/47747/#review44957

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 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 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 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.

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/

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 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 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 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 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 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 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 on attachment 8748436 [details] MozReview Request: Bug 1253110 - Implement GeneratedFile.read(); r?glandium https://reviewboard.mozilla.org/r/50279/#review47461

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 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 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 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

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.