Closed
Bug 1250991
Opened 9 years ago
Closed 9 years ago
Upload make file cleanup
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 1 obsolete file)
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 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 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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•9 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•9 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•9 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•9 years ago
|
Attachment #8723292 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4ba91fdbffe1
https://hg.mozilla.org/mozilla-central/rev/d6afd71bafd7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•