1250991 - Upload make file cleanup

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Currently, artifact builds download a package file from automation and post-process it into a jar/zip file containing only the bits that are needed. This takes several seconds and makes artifact builds slower than they could be.

Let's produce the jar/zip that would be produced locally as part of automation and have artifact builds download it directly. Artifact builds will then literally uncompress a jar/zip and do the partial build.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1250991 - Indent code; r=mshal

This file is so hard to read. Add some indentation to make it easier to
grok.

I also converted some useless tabs to spaces.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1250991 - Move APK upload files code to own file; r=mshal

This is several hundred lines of make goo that makes upload-files.mk
even harder to read than it actually is. Extract it to its own file.

I performed a `hg cp` to preserve file history so blame continues to
work.

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

Comment 3

[Gregory Szorc [:gps]]

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

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

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

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8723291 [details]
MozReview Request: Bug 1250991 - Move APK upload files code to own file; r=mshal

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

Comment 5

[Gregory Szorc [:gps]]

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

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

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8723290 [details]
MozReview Request: Bug 1250991 - Indent code; r=mshal

https://reviewboard.mozilla.org/r/36453/#review33167

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 8723291 [details]
MozReview Request: Bug 1250991 - Move APK upload files code to own file; r=mshal

https://reviewboard.mozilla.org/r/36455/#review33031

::: toolkit/mozapps/installer/upload-files-apk.mk:292
(Diff revision 1)
>  endif

I think this endif is the ifeq ($MOZ_PKG_FORMAT),APK) one, and doesn't belong in the new upload-files-apk.mk file.

::: toolkit/mozapps/installer/upload-files-apk.mk:292
(Diff revision 2)
>  

nit: empty line at the end of upload-files-apk.mk

::: toolkit/mozapps/installer/upload-files.mk:255
(Diff revision 2)
> -
> +include $(MOZILLA_DIR)/toolkit/mozapps/installer/upload-files-apk.mk

We could probably do something like 'include $(MOZILLA_DIR)/toolkit/mozapps/installer/upload-files-$(MOZ_PKG_FORMAT).mk' if we move the other formats into their own files as well. You don't need to block on this, though.

Might make sense to use upload-files-APK.mk as the new filename if you intend to do this in the future.

Comment 8

[Michael Shal [:mshal]]

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

https://reviewboard.mozilla.org/r/36457/#review33175

Overall this looks pretty good. The file list issue is the main concern, though I'm not sure how feasible it is to address that for this bug.

::: python/mozbuild/mozbuild/action/artifact_archive.py:48
(Diff revision 2)
> +    all_files = {p: 'bin/%s' % p for p in bin_files}

Is this list something we can potentially annotate in moz.build files somehow and export it from the build system? It seems like it will be annoying to keep bin_files and IGNORE_BIN_DYLIBS (and their equivalents on other platforms) up to date.

::: toolkit/mozapps/installer/packager.mk:89
(Diff revision 2)
> +ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))

Similar to the other platform check, this could be removed if the artifact_archive action returns successfully instead of raising an exception. Other platforms could then be added by just changing artifact_archive.py rather than also fiddling with Makefile checks.

::: toolkit/mozapps/installer/packager.mk:90
(Diff revision 2)
> +	$(call py_action,artifact_archive,$(DIST)/$(BINARY_ARTIFACT_PACKAGE))

Is there any reason the binary artifact package can't be parallelized with creating the regular package? If you put this in a separate rule, you might get some parallelization wins. That might be more effort than it's worth though, so feel free to keep as is.

::: toolkit/mozapps/installer/upload-files.mk:526
(Diff revision 2)
> +# Upload binary artifact archive.

Since we use QUOTED_WILDCARD, you could just stick this UPLOAD_FILES addition in the main list, and it will get uploaded if it's built and ignored if not. That way you don't have to update the platform checks as more platforms are supported.

Comment 9

[Gregory Szorc [:gps]]

Repurposing this bug to land the commits that are ready to land. Will clone to track the follow-up work.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8723290 [details]
MozReview Request: Bug 1250991 - Indent code; r=mshal

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

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8723291 [details]
MozReview Request: Bug 1250991 - Move APK upload files code to own file; r=mshal

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba91fdbffe1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6afd71bafd7

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ba91fdbffe1
https://hg.mozilla.org/mozilla-central/rev/d6afd71bafd7