Closed Bug 1250991 Opened 9 years ago Closed 9 years ago

Upload make file cleanup

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

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.
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)
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)
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)
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 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/
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)
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
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
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
Attachment #8723292 - Attachment is obsolete: true
Blocks: 1253110
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: