If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Upload make file cleanup

RESOLVED FIXED in Firefox 47

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8723290 [details]
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/
Attachment #8723290 - Flags: review?(mshal)
(Assignee)

Comment 2

2 years ago
Created attachment 8723291 [details]
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/
Attachment #8723291 - Flags: review?(mshal)
(Assignee)

Comment 3

2 years ago
Created attachment 8723292 [details]
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/
Attachment #8723292 - Flags: review?(mshal)
(Assignee)

Comment 4

2 years ago
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/
(Assignee)

Comment 5

2 years ago
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 on attachment 8723290 [details]
MozReview Request: Bug 1250991 - Indent code; r=mshal

https://reviewboard.mozilla.org/r/36453/#review33167
Attachment #8723290 - Flags: review?(mshal) → review+
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.
Attachment #8723291 - Flags: review?(mshal) → review+
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.
Attachment #8723292 - Flags: review?(mshal)
(Assignee)

Comment 9

2 years ago
Repurposing this bug to land the commits that are ready to land. Will clone to track the follow-up work.
Summary: Produce zip files for artifact builds to avoid client-side processing → Upload make file cleanup
(Assignee)

Comment 10

2 years ago
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/
Attachment #8723290 - Attachment description: MozReview Request: Bug 1250991 - Indent code; r?mshal → MozReview Request: Bug 1250991 - Indent code; r=mshal
(Assignee)

Comment 11

2 years ago
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/
Attachment #8723291 - Attachment description: MozReview Request: Bug 1250991 - Move APK upload files code to own file; r?mshal → MozReview Request: Bug 1250991 - Move APK upload files code to own file; r=mshal
(Assignee)

Updated

2 years ago
Attachment #8723292 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1253110

Comment 12

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

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ba91fdbffe1
https://hg.mozilla.org/mozilla-central/rev/d6afd71bafd7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.