Closed
Bug 641212
Opened 13 years ago
Closed 7 years ago
LZMA support for updater
Categories
(Toolkit :: Application Update, enhancement, P1)
Toolkit
Application Update
Tracking
()
VERIFIED
FIXED
mozilla56
People
(Reporter: catlee, Assigned: robert.strong.bugs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxgrowth][fce-active-legacy])
Attachments
(4 files, 43 obsolete files)
2.41 KB,
patch
|
molly
:
review+
|
Details | Diff | Splinter Review |
8.87 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
205.67 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
lzma compression gives at least a 10% benefit over the bzip2 compression we're currently using for our complete and partial mars. From today's partial update for linux64: ls -l update*.mar -rw-r--r-- 1 catlee catlee 1009034 Mar 12 09:07 update-lzma.mar -rw------- 1 catlee catlee 1164683 Mar 12 09:05 update.mar
Comment 1•13 years ago
|
||
Out of curiosity, was lzma2 also looked at? Also, was this with default compression settings?
Reporter | ||
Comment 2•13 years ago
|
||
(In reply to comment #1) > Out of curiosity, was lzma2 also looked at? Also, was this with default > compression settings? This was using: LZMA command line tool 4.32.0beta3 LZMA SDK 4.43 I used the default compression settings above. Using --best I get 952339 bytes.
Comment 3•13 years ago
|
||
Might be interesting to try out the LZMA SDK 9.20 with LZMA2 compression.
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to comment #3) > Might be interesting to try out the LZMA SDK 9.20 with LZMA2 compression. So using xz -9e, (which I think is using LZMA2 (liblzma 5.0.0)), I get 945010 bytes. This is an 18.9% reduction from the original partial update. lzma -9 gave an 18.2% reduction.
Comment 5•13 years ago
|
||
Yeah, xz and lzma2 support go together. Any differences in compression speed (or probably more important to end users, memory for decompression).
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > Yeah, xz and lzma2 support go together. Any differences in compression speed > (or probably more important to end users, memory for decompression). Decompressing libxul.so.patch (1,130,991 bytes with bz2) under valgrind reports 3,670,444 bytes allocated. Decompressing libxul.so.patch (921,198 bytes with lzma) under valgrind reports 34,635,428 bytes allocated. Decompressing libxul.so.patch (913,064 bytes with xz) under valgrind reports 67,148,059 bytes allocated.
Reporter | ||
Comment 7•13 years ago
|
||
(In reply to comment #5) > Yeah, xz and lzma2 support go together. Any differences in compression speed > (or probably more important to end users, memory for decompression). compression times: bz2: 4.0s lzma: 38.8s xz: 44.6s decompression times: bz2: 0.6s lzma: 0.3s xz: 0.3s For each case I've unpacked the MAR file, and shelled out to each compression tool to compress/decompress. I would assume things would be slightly faster for users since the updater has built-in compression support.
Comment 8•13 years ago
|
||
Sure looks like LZMA2 isn't worth the extra effort and memory footprint. Thanks for checking!
Reporter | ||
Comment 9•13 years ago
|
||
(In reply to comment #8) > Sure looks like LZMA2 isn't worth the extra effort and memory footprint. Thanks > for checking! fwiw the memory footprint doesn't change much even when decompressing the full libxul.so/xul.dll vs. the patch.
Assignee | ||
Comment 10•13 years ago
|
||
I looked at doing this around a year ago and got sidetracked by other work. I personally *think* that we should do this but other work takes priority at the moment.
Reporter | ||
Comment 11•12 years ago
|
||
The relevant part of this patch modifies ArchiveReader::ExtractItemToStream to do some file magic detection first. It will check for "BZh" at the beginning of a file (bz2's magic) and then hand off control to the original bz2 decompression code in ::ExtractBZ2ItemToStream. If it fails to find "BZh" it will then check for "\xfd7zXZ\x00" which is XZ's magic. The code in ::ExtractXZItemToStream is loosely based on xzdec.c. Building xz is another matter. The current implementation unpacks the tarball and hands off control to xz's configure and make. This works on linux only ATM. At the minimum we'd need to compile the static liblzma library. It would probably be nice to compile the xz cmdline tool so that you can then also create mars using xz compression. The source for XZ is available here: http://tukaani.org/xz/xz-5.0.3.tar.bz2
Attachment #614346 -
Flags: feedback?(robert.bugzilla)
Assignee | ||
Comment 12•12 years ago
|
||
Comment on attachment 614346 [details] [diff] [review] add xz support to updater Note: I talked with catlee offline about implementing bug 504624 instead since it appears that we can now use courgette... clearing feedback review due to this but I will likely go over this patch when there is time to do so.
Attachment #614346 -
Flags: feedback?(robert.bugzilla)
Comment 13•10 years ago
|
||
AFAIK, we currently use 7-zip compression in the Windows installer (and possibly other locations). Can we use the 7-zip file format, using either LZMA or LZMA2, instead of adding another compression format (xz)? Given that they use the same compression algorithms & therefore should produce the same level of compression savings (untested), why proliferate the number of compression formats we use?
Updated•9 years ago
|
Whiteboard: [fxgrowth]
Reporter | ||
Comment 15•9 years ago
|
||
refreshed WIP patch this works on Linux, compiles at least on OSX (don't have a local machine to test it on), but fails to build on Windows. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a15a4464cc the xz source should be unpacked into modules/xz/src, like this: https://hg.mozilla.org/try/rev/b2a258efcb93
Attachment #614346 -
Attachment is obsolete: true
Reporter | ||
Comment 16•8 years ago
|
||
This adds support for decompressing xz archives in the updater. Tested on linux.
Attachment #8583049 -
Attachment is obsolete: true
Comment 17•8 years ago
|
||
Slight update to catlee's moz.build to support Windows and to enable the BCJ filter for ARM binaries. I didn't know xz-embedded existed; that seems like a much better option than dealing with the full liblzma.
Assignee: nobody → mhowell
Attachment #8799743 -
Attachment is obsolete: true
Reporter | ||
Comment 18•8 years ago
|
||
Just FYI - the xz-embedded work was cherry picked from bug 1307570.
Updated•8 years ago
|
Whiteboard: [fxgrowth] → [fxgrowth][fce-active]
Comment 19•8 years ago
|
||
Bug 1307570 has landed, so the "add xz-embedded to the tree" part of this patch is now redundant. Over in bug 1303180 I was talking about using a new MAR additional section type to identify the compression instead of reading the magic number from the compressed stream as this patch does. I've changed my mind about that, I don't think it's necessary. The maintainability concern I had doesn't really make sense considering we're not extracting arbitrary streams, only ones produced by our own code. If anything ever breaks the magic number check, it would be our own change, and we'd probably have to be doing something else around there because of that change anyway. So I'm okay with leaving it this way.
Attachment #8801343 -
Attachment is obsolete: true
Reporter | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=252a2ae0f120
Assignee | ||
Comment 21•8 years ago
|
||
A couple of notes I discussed with mhowell today. 1. Compressing all files into a single archive instead of individually should significantly improve compression. 2. The majority of users should perform the extraction by staging the update while Firefox is running so the time it takes to extract should be less of an issue. The percentage of users that stage updates should be verified via telemetry. 3. If there are system resource issues when staging the updater process priority could be lowered.
Reporter | ||
Comment 22•8 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #21) > A couple of notes I discussed with mhowell today. > > 1. Compressing all files into a single archive instead of individually > should significantly improve compression. This was one of the approaches I suggested a few years ago: http://atlee.ca/blog/posts/firefox-update-sizes.html. Looks like it's still valid! The impact can be approximated by compressing a MAR file containing uncompressed contents. e.g. 46042752 firefox-43.0.1.complete-raw.xz # MAR file of uncompressed contents, then compressed with xz 46999097 firefox-43.0.1.complete-xz.mar # MAR file of xz-compressed contents 53665068 firefox-43.0.1.complete.mar # original This approach doesn't help as much with partial MARs though. I imagine this is because the bsdiff files have very little in common between them. 20212852 firefox-42.0-43.0.1.partial-raw.mar.xz 20283661 firefox-42.0-43.0.1.partial-xz.mar 21358088 firefox-42.0-43.0.1.partial.mar
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8804912 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8815464 [details] Bug 641212 - Part 4: Support for creating MAR files compressed using XZ; https://reviewboard.mozilla.org/r/96364/#review96768 ::: tools/update-packaging/Makefile.in:69 (Diff revision 2) > '$(DIST)/$(COMPLETE_MAR)' \ > '$(PACKAGE_DIR)' > ifdef MOZ_SIGN_CMD > $(MOZ_SIGN_CMD) -f mar '$(DIST)/$(COMPLETE_MAR)' > endif > + xz --compress --extreme --format=xz --check=crc64 '$(DIST)/$(COMPLETE_MAR)' Do we want to use the x86 filter here? It gives substantial improvements with complete MARs. Something like xz --compress --x86 --lzma2=preset=9e --format=xz --check=crc64
Reporter | ||
Comment 32•7 years ago
|
||
Are there security implications of compressing the entire archive with XZ? That would mean the MAR file must first be completely decompressed before it can be verified.
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8815461 [details] Bug 641212 - Part 1: Enable the updater to extract MAR files compressed using XZ; https://reviewboard.mozilla.org/r/96358/#review96900 Discussed verifying the mar file before acting on it (e.g. extracting a compressed mar file) with mhowell over irc and we want to verify first so clearing review.
Attachment #8815461 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8815462 [details] Bug 641212 - Part 2: Add a test for extracting XZ-compressed MAR files; https://reviewboard.mozilla.org/r/96360/#review96902
Attachment #8815462 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8815463 [details] Bug 641212 - Part 3: Support XZ-compressed MAR files in the mar command-line tools; https://reviewboard.mozilla.org/r/96362/#review96904
Attachment #8815463 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8815464 [details] Bug 641212 - Part 4: Support for creating MAR files compressed using XZ; https://reviewboard.mozilla.org/r/96364/#review96906
Attachment #8815464 -
Flags: review?(robert.strong.bugs)
Updated•7 years ago
|
Attachment #8815461 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8815462 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8815463 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8815464 -
Attachment is obsolete: true
Comment 37•7 years ago
|
||
This version won't land in light of comment 33, so I withdrew the review, but here's my last version of that patch anyway, just for the record.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
One thing that makes me hesitant is moving the compression to calling a command from within libmar. Would it be possible to keep the compression within the update mar creation scripts as it is currently?
Comment 42•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #41) > One thing that makes me hesitant is moving the compression to calling a > command from within libmar. Would it be possible to keep the compression > within the update mar creation scripts as it is currently? Compressing part of a file instead of an entire file isn't really something that's easy to do from a shell script. Plus the MAR file size and offset to index fields have to be modified once the compression is done, which sounds like an awful lot of trouble. Another way to avoid having to call the XZ command would be to import a more complete version of liblzma; the only reason the command is needed at all is that the xz-embedded version we're using (already in the tree) only implements decompression.
Assignee | ||
Comment 43•7 years ago
|
||
Agreed... without thinking about it deeply I thought the script could just add the files to the archive and the archive could then be added to the mar. After thinking about it a little bit more (likely no where near enough) how about putting the command in the environment? That way it is simple to modify the values used to create the archive?
Updated•7 years ago
|
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8820922 -
Attachment is obsolete: true
Attachment #8820922 -
Flags: review?(robert.strong.bugs)
Updated•7 years ago
|
Attachment #8825958 -
Attachment is obsolete: true
Attachment #8825958 -
Flags: review?(robert.strong.bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•7 years ago
|
||
I pushed the patches to oak. I unbitrotted them and added the diff of wrong_product_channel.mar to part 9 which for some unknown reason was missing when I downloaded it (multiple times even).
Assignee | ||
Comment 54•7 years ago
|
||
mozreview-review |
Comment on attachment 8832668 [details] Bug 641212 Part 0 - Copy liblzma into the tree. , f?gerv https://reviewboard.mozilla.org/r/108856/#review160146 You will need to get a build peer to review this as well.
Assignee | ||
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8832669 [details] Bug 641212 Part 4 - Refactor updater headers to be usable from C. https://reviewboard.mozilla.org/r/108858/#review160148
Attachment #8832669 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8832668 [details] Bug 641212 Part 0 - Copy liblzma into the tree. , f?gerv https://reviewboard.mozilla.org/r/108856/#review160150 Forgot to r+ You will need to get a build peer to review this as well.
Attachment #8832668 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 57•7 years ago
|
||
mozreview-review |
Comment on attachment 8832670 [details] Bug 641212 Part 3 - Create MAR files using XZ compression. https://reviewboard.mozilla.org/r/108860/#review160154 ::: modules/libmar/src/mar_create.c:15 (Diff revision 1) > #include <stdlib.h> > #include <string.h> > #include "mar_private.h" > #include "mar_cmdline.h" > #include "mar.h" > +#include "updatedefines.h" Should be fine but I am tempted not to include this just to get MAXPATHLEN ::: modules/libmar/src/mar_create.c:295 (Diff revision 1) > * @param files The list of null-terminated file paths. Each file > * path must be compatible with fopen. > * @param infoBlock The information to store in the product information block. > * @return A non-zero value if an error occurs. > */ > int mar_create(const char *dest, int Please remove the trailing spaces here int mar_create(const char *dest, int num_files, char **files, and uint32_t offset_to_index = 0, size_of_index, Also int num_files on the same line ::: modules/libmar/src/mar_create.c:432 (Diff revision 1) > if (fwrite(&sizeOfEntireMAR, sizeof(sizeOfEntireMAR), 1, fp) != 1) > goto failure; > sizeOfEntireMAR = NETWORK_TO_HOST64(sizeOfEntireMAR); > > rv = 0; > failure: Please remove the trailing space here
Attachment #8832670 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 58•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #57) > Comment on attachment 8832670 [details] > Bug 641212 Part 3 - Create MAR files using XZ compression. > > https://reviewboard.mozilla.org/r/108860/#review160154 > > ::: modules/libmar/src/mar_create.c:15 > (Diff revision 1) > > #include <stdlib.h> > > #include <string.h> > > #include "mar_private.h" > > #include "mar_cmdline.h" > > #include "mar.h" > > +#include "updatedefines.h" > > Should be fine but I am tempted not to include this just to get MAXPATHLEN I see that it is used more extensively elsewhere so nevermind
Assignee | ||
Comment 59•7 years ago
|
||
test_create.js is failing with 'xz' is not recognized as an internal or external command,
Assignee | ||
Comment 60•7 years ago
|
||
Forgot to mention on Windows 7 VM debug
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 72•7 years ago
|
||
mozreview-review |
Comment on attachment 8884666 [details] Bug 641212 Part 1 - Incorporate liblzma into the build system. https://reviewboard.mozilla.org/r/155530/#review160528 Be sure to get a build peer to review this as well. ::: modules/xz/moz.build:104 (Diff revision 1) > +DEFINES['HAVE_INTTYPES_H'] = HOST_DEFINES['HAVE_INTTYPES_H'] = 1 > +DEFINES['HAVE_STDBOOL_H'] = HOST_DEFINES['HAVE_STDBOOL_H'] = 1 > +DEFINES['HAVE_CHECK_CRC32'] = HOST_DEFINES['HAVE_CHECK_CRC32'] = 1 > +DEFINES['HAVE_CHECK_CRC64'] = HOST_DEFINES['HAVE_CHECK_CRC64'] = 1 > +DEFINES['HAVE_DECODER_LZMA2'] = HOST_DEFINES['HAVE_DECODER_LZMA2'] = 1 > +DEFINES['HAVE_ENCODER_LZMA2'] = HOST_DEFINES['HAVE_ENCODER_LZMA2'] = 1 Duplicate of previous line ::: modules/xz/moz.build:110 (Diff revision 1) > +DEFINES['HAVE_MF_BT4'] = HOST_DEFINES['HAVE_MF_BT4'] = 1 > + > +if '86' in CONFIG['TARGET_CPU']: > + # Accept x86, x86_64, i386, i686, etc. > + DEFINES['HAVE_DECODER_X86'] = HOST_DEFINES['HAVE_DECODER_X86'] = 1 > + DEFINES['HAVE_ENCODER_X86'] = HOST_DEFINES['HAVE_ENCODER_X86'] = 1 Duplicate of previous line
Attachment #8884666 -
Flags: review?(robert.strong.bugs) → review-
Assignee | ||
Comment 73•7 years ago
|
||
mozreview-review |
Comment on attachment 8884667 [details] Bug 641212 Part 2 - Adjust mar/signmar/updater build files to link liblzma instead of bzip. https://reviewboard.mozilla.org/r/155532/#review160530 Would be good to get a build peer to review this as well
Attachment #8884667 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 74•7 years ago
|
||
mozreview-review |
Comment on attachment 8832672 [details] Bug 641212 Part 6 - Remove usage of bzip2 from the MAR builder scripts. https://reviewboard.mozilla.org/r/108864/#review160532 Please also ask releng (probably bhearsum) to review this since they own this file.
Attachment #8832672 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 75•7 years ago
|
||
mozreview-review |
Comment on attachment 8832673 [details] Bug 641212 Part 7 - Replace libbz2 with XZ in the mbsdiff tool. https://reviewboard.mozilla.org/r/108866/#review160534
Attachment #8832673 -
Flags: review?(robert.strong.bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 86•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8884666 [details] Bug 641212 Part 1 - Incorporate liblzma into the build system. https://reviewboard.mozilla.org/r/155530/#review160528 > Duplicate of previous line They're different; DEcoder vs. ENcoder. > Duplicate of previous line They're different; DEcoder vs. ENcoder.
Assignee | ||
Comment 87•7 years ago
|
||
Oops, eyes are tired. :)
Comment 88•7 years ago
|
||
About to add bhearsum and glandium as reviewers for specific patches per rstrong's request.
Updated•7 years ago
|
Attachment #8884666 -
Flags: review?(robert.strong.bugs) → review?(mh+mozilla)
Attachment #8884667 -
Flags: review?(mh+mozilla)
Attachment #8832672 -
Flags: review?(bhearsum)
Assignee | ||
Comment 89•7 years ago
|
||
mozreview-review |
Comment on attachment 8832674 [details] Bug 641212 Part 8 - Remove bz2 support from the updater. https://reviewboard.mozilla.org/r/108868/#review160536 Looks good except for a couple of nits ::: toolkit/mozapps/update/common/errors.h:80 (Diff revision 2) > #define SERVICE_INVALID_APPLYTO_DIR_ERROR 54 > #define SERVICE_INVALID_INSTALL_DIR_PATH_ERROR 55 > #define SERVICE_INVALID_WORKING_DIR_PATH_ERROR 56 > #define SERVICE_INSTALL_DIR_REG_ERROR 57 > > +#define UNEXPECTED_XZ_ERROR 52 Use 43 and place it where 43 used to be along with a comment that this used to be for gonk FILESYSTEM_MOUNT_READWRITE_ERROR ::: toolkit/mozapps/update/updater/archivereader.cpp:273 (Diff revision 2) > } > > int > ArchiveReader::ExtractItemToStream(const MarItem *item, FILE *fp) > { > - /* decompress the data chunk by chunk */ > + // We need to wait to do this until after the certificate has been checked, It isn't clear what is waiting to make sure this is true. ::: toolkit/mozapps/update/updater/archivereader.cpp:275 (Diff revision 2) > int > ArchiveReader::ExtractItemToStream(const MarItem *item, FILE *fp) > { > - /* decompress the data chunk by chunk */ > - > - bz_stream strm; > + // We need to wait to do this until after the certificate has been checked, > + // so that we don't do a bunch of processing on a file of unknown validity. > + // At this point, the check has to have been done if it's going to be. nit: Please reword the following so its meaning is clearer. "At this point, the check has to have been done if it's going to be."
Attachment #8832674 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 90•7 years ago
|
||
mozreview-review |
Comment on attachment 8832675 [details] Bug 641212 Part 9 - Rebuild libmar unit test data files in new XZ format. https://reviewboard.mozilla.org/r/108870/#review160540
Attachment #8832675 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 91•7 years ago
|
||
mozreview-review |
Comment on attachment 8832676 [details] Bug 641212 Part 10 - Rebuild updater test data files in new XZ format. https://reviewboard.mozilla.org/r/108872/#review160542
Attachment #8832676 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 92•7 years ago
|
||
Pushed latest set of patches to oak
Assignee | ||
Comment 93•7 years ago
|
||
I don't think this needs to be built on Android or when MOZ_UPDATER isn't defined. Both will need to be checked since Android overrides MOZ_UPDATER.
Assignee | ||
Comment 94•7 years ago
|
||
Strangely enough, the OS X builds succeeded on oak. The spidermonkey and android builds failed so I pushed a fix to oak to see if OS X will succeed a second time.
Assignee | ||
Comment 95•7 years ago
|
||
Ignore the part about spidermonkey... way too sleepy
Assignee | ||
Comment 96•7 years ago
|
||
mozreview-review |
Comment on attachment 8884666 [details] Bug 641212 Part 1 - Incorporate liblzma into the build system. https://reviewboard.mozilla.org/r/155530/#review160636 ::: config/external/moz.build:24 (Diff revision 2) > > # There's no "native" brotli or woff2 yet, but probably in the future... > external_dirs += ['modules/brotli'] > external_dirs += ['modules/woff2'] > > +external_dirs += ['modules/xz'] Just noticed this should just replace the following which is above the addition if CONFIG['MOZ_UPDATER']: if not CONFIG['MOZ_SYSTEM_BZ2']: external_dirs += ['modules/libbz2'] It should be if CONFIG['MOZ_UPDATER']: if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android' external_dirs += ['modules/libbz2'] Building libmar will need to be disabled on android as well which should be fine since they don't use the updater.
Attachment #8884666 -
Flags: review-
Assignee | ||
Comment 97•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #96) <snip> > if CONFIG['MOZ_UPDATER']: > if not CONFIG['MOZ_SYSTEM_BZ2']: > external_dirs += ['modules/libbz2'] > > It should be > if CONFIG['MOZ_UPDATER']: > if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android' > external_dirs += ['modules/libbz2'] I meant external_dirs += ['modules/xz']
Assignee | ||
Comment 98•7 years ago
|
||
Actually just if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android' external_dirs += ['modules/xz'] So signmar is created even when not building the updater
Reporter | ||
Comment 99•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #96) > Building libmar will need to be disabled on android as well which should be > fine since they don't use the updater. FTR - The Mozilla Online version of Fennec does have MOZ_UPDATER set - https://bugzilla.mozilla.org/show_bug.cgi?id=1357288#c8
Assignee | ||
Comment 100•7 years ago
|
||
(In reply to Chris AtLee [:catlee] from comment #99) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #96) > > Building libmar will need to be disabled on android as well which should be > > fine since they don't use the updater. > > FTR - The Mozilla Online version of Fennec does have MOZ_UPDATER set - > https://bugzilla.mozilla.org/show_bug.cgi?id=1357288#c8 Note: Android used to use the in repo application update code and --enable-updater and MOZ_UPDATER are used to enable that component. When Android decided to no longer use it a new define should have been created (see bug 1152634) but they haven't gotten around to it.
Assignee | ||
Comment 101•7 years ago
|
||
Also for reference - bug 1152997
Comment 102•7 years ago
|
||
mozreview-review |
Comment on attachment 8832672 [details] Bug 641212 Part 6 - Remove usage of bzip2 from the MAR builder scripts. https://reviewboard.mozilla.org/r/108864/#review160864 I think you need to update unwrap_full_update.pl, too. It appears that file is used by funsize (per a recent log: https://public-artifacts.taskcluster.net/mnCgNeiaTnyZFDV_KZjY4g/0/public/logs/live_backing.log). Speaking of which, will we need to update funsize for this as well?
Attachment #8832672 -
Flags: review?(bhearsum) → review-
Comment 103•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8832672 [details] Bug 641212 Part 6 - Remove usage of bzip2 from the MAR builder scripts. https://reviewboard.mozilla.org/r/108864/#review160864 Well, the change to unwrap_full_update.pl will be to delete almost all of it, because the "unwrap" step no longer exists in this format; the files are decompressed as part of the mar extraction. But if it still needs to be there for compatibility, then that's okay.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 115•7 years ago
|
||
And of course because I forgot to add those secondary review requests to the commit messages, they got deleted when I pushed a new revision. So let's add those back manually again.
Updated•7 years ago
|
Attachment #8884666 -
Flags: review?(robert.strong.bugs) → review?(mh+mozilla)
Attachment #8884667 -
Flags: review?(mh+mozilla)
Updated•7 years ago
|
Attachment #8815906 -
Attachment is obsolete: true
Comment 116•7 years ago
|
||
Comment on attachment 8832668 [details] Bug 641212 Part 0 - Copy liblzma into the tree. , f?gerv Hi, gerv; this patch adds a new third-party library to the tree, and I'm told you're the person to ask for feedback about whether I've covered the right bases for doing that. The code is public domain with attribution requested, so I've added a line to about:license, along with a readme.mozilla file to indicate where the code came from and which specific version this is. I'd appreciate if you could take a look and let me know anything else I should do. Thanks!
Attachment #8832668 -
Flags: feedback?(gerv)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 127•7 years ago
|
||
mozreview-review |
Comment on attachment 8884666 [details] Bug 641212 Part 1 - Incorporate liblzma into the build system. https://reviewboard.mozilla.org/r/155530/#review160994
Attachment #8884666 -
Flags: review?(robert.strong.bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8832668 -
Flags: feedback?(gerv)
Assignee | ||
Comment 139•7 years ago
|
||
Note: since the updater wil no longer be using bzip2 the updater's licenses.html reference to bzip2 can likely be removed.
Assignee | ||
Comment 140•7 years ago
|
||
mozreview-review |
Comment on attachment 8832670 [details] Bug 641212 Part 3 - Create MAR files using XZ compression. https://reviewboard.mozilla.org/r/108860/#review161070 ::: modules/libmar/src/mar_create.c:402 (Diff revision 6) > goto failure; > - } > + } > + } > + > + // Finish the LZMA stream and write out any remaining data. > + if (lzma_code(&strm, LZMA_FINISH) != LZMA_STREAM_END) { lzma_code specifying LZMA_FINISH needs to be called repeatedly until it returns LZMA_STREAM_END. This doesn't have a problem with small files but when creating a complet mar for Firefox this fails https://github.com/xbmc/xz/blob/master/doc/examples/01_compress_easy.c#L137 https://lists.samba.org/archive/linux/2013-November/032622.html
Attachment #8832670 -
Flags: review+ → review-
Assignee | ||
Comment 141•7 years ago
|
||
mozreview-review |
Comment on attachment 8832671 [details] Bug 641212 Part 5 - Support opening and extracting XZ-compressed MAR archives. https://reviewboard.mozilla.org/r/108862/#review161086 I'm seeing similar issues with decompression as I saw with compression.
Attachment #8832671 -
Flags: review?(robert.strong.bugs) → review-
Comment 142•7 years ago
|
||
mozreview-review |
Comment on attachment 8832672 [details] Bug 641212 Part 6 - Remove usage of bzip2 from the MAR builder scripts. https://reviewboard.mozilla.org/r/108864/#review161342 ::: tools/update-packaging/make_incremental_updates.py:4 (Diff revision 6) > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > I think file is dead now...this won't hurt though. ::: tools/update-packaging/unwrap_full_update.pl:42 (Diff revision 6) > - > -$? && die("Couldn't run \"$MAR\" -t"); > - > -shift @marentries; > - > system("$MAR -x \"$archive\"") == 0 || die "Couldn't run $MAR -x"; I guess we should kill this at some point and make consumers call mar on their own. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1380048 for this.
Attachment #8832672 -
Flags: review?(bhearsum) → review+
Updated•7 years ago
|
QA Contact: amasresha
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 160•7 years ago
|
||
Landed the latest patches on oak and triggered a nightly Win. The complete mar size for Win32 went from around 49 MB to around 37 MB and there was a similar reduction for Win64.
Comment 161•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #160) > Landed the latest patches on oak and triggered a nightly Win. The complete > mar size for Win32 went from around 49 MB to around 37 MB and there was a > similar reduction for Win64. Wow! This is amazing! Thanks a lot for this! \o/
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 173•7 years ago
|
||
mozreview-review |
Comment on attachment 8832674 [details] Bug 641212 Part 8 - Remove bz2 support from the updater. https://reviewboard.mozilla.org/r/108868/#review161716 ::: toolkit/mozapps/update/updater/archivereader.cpp (Diff revisions 8 - 9) > if (mar_decompress(mArchive) == -1) { > return UNEXPECTED_XZ_ERROR; > } > > - if (item->length == 0) { > - return UNEXPECTED_MAR_ERROR; Please also comment out UNEXPECTED_MAR_ERROR in errors.h with a note regarding this no longer being necessary with lzma.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 177•7 years ago
|
||
(In reply to Matt Howell [:mhowell] (PTO July 13 - July 21) from comment #116) > Comment on attachment 8832668 [details] > Bug 641212 Part 0 - Copy liblzma into the tree. , f?gerv > > Hi, gerv; this patch adds a new third-party library to the tree, and I'm > told you're the person to ask for feedback about whether I've covered the > right bases for doing that. The code is public domain with attribution > requested, so I've added a line to about:license, along with a > readme.mozilla file to indicate where the code came from and which specific > version this is. I'd appreciate if you could take a look and let me know > anything else I should do. Thanks! This seems exactly right, although I might put "(Lasse Collin)" (if that's the right name) after the attribution in about:license, to match the other optional credits. Gerv
Assignee | ||
Comment 178•7 years ago
|
||
Matt was on PTO so I created patches that apply after his patches to fix a few issues and Matt will review them.
Assignee | ||
Comment 179•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8889271 -
Attachment description: libmar mar files → libmar mar files part 11
Assignee | ||
Updated•7 years ago
|
Attachment #8889272 -
Attachment description: libmar mar file extracted_certs → libmar mar file extracted certs part 12
Assignee | ||
Comment 180•7 years ago
|
||
Assignee | ||
Comment 181•7 years ago
|
||
Assignee | ||
Comment 182•7 years ago
|
||
Assignee | ||
Comment 183•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #182) > Created attachment 8889276 [details] [diff] [review] > xz changes part 15 Matt, even after fixing all Linux and Windows compiler errors I ran into those same weird OS X compiler errors that you ran into and I *think* this is what fixed them.
Assignee | ||
Comment 184•7 years ago
|
||
Assignee | ||
Comment 185•7 years ago
|
||
Assignee | ||
Comment 186•7 years ago
|
||
Assignee | ||
Comment 187•7 years ago
|
||
Assignee | ||
Comment 188•7 years ago
|
||
I've had a few successful try push and hopefully this one will be also https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de45f24a4e94d91b704614985c2a338bb1c26d9&selectedJob=116813959
Assignee | ||
Comment 189•7 years ago
|
||
Note: I removed the no pib tests since they were created for the transition between not having a pib and having a pib.
Assignee | ||
Comment 190•7 years ago
|
||
Landed all patches on oak along with bug 1105689
Assignee | ||
Comment 191•7 years ago
|
||
Matt, not sure why the lint opt Doc job fails but I think it is related to the license change.
Comment 192•7 years ago
|
||
mozreview-review |
Comment on attachment 8884666 [details] Bug 641212 Part 1 - Incorporate liblzma into the build system. https://reviewboard.mozilla.org/r/155530/#review166070 I'm rather uneased by the addition of another xz decompressor. For compression, we don't have that in the tree, so it's understandable, but why not use xz-embedded for the updater (it would probably be smaller, too) ::: modules/xz/moz.build:98 (Diff revision 7) > +DEFINES['LZMA_API_STATIC'] = HOST_DEFINES['LZMA_API_STATIC'] = 1 > +DEFINES['HAVE_INTTYPES_H'] = HOST_DEFINES['HAVE_INTTYPES_H'] = 1 > +DEFINES['HAVE_STDBOOL_H'] = HOST_DEFINES['HAVE_STDBOOL_H'] = 1 > +DEFINES['HAVE_STRING_H'] = HOST_DEFINES['HAVE_STRING_H'] = 1 > +DEFINES['HAVE_CHECK_CRC32'] = HOST_DEFINES['HAVE_CHECK_CRC32'] = 1 > +DEFINES['HAVE_CHECK_CRC64'] = HOST_DEFINES['HAVE_CHECK_CRC64'] = 1 > +DEFINES['HAVE_DECODER_LZMA2'] = HOST_DEFINES['HAVE_DECODER_LZMA2'] = 1 > +DEFINES['HAVE_ENCODER_LZMA2'] = HOST_DEFINES['HAVE_ENCODER_LZMA2'] = 1 > +DEFINES['HAVE_MF_BT4'] = HOST_DEFINES['HAVE_MF_BT4'] = 1 Some of these look like they should be set to True rather than 1. ::: modules/xz/moz.build:108 (Diff revision 7) > +DEFINES['HAVE_CHECK_CRC64'] = HOST_DEFINES['HAVE_CHECK_CRC64'] = 1 > +DEFINES['HAVE_DECODER_LZMA2'] = HOST_DEFINES['HAVE_DECODER_LZMA2'] = 1 > +DEFINES['HAVE_ENCODER_LZMA2'] = HOST_DEFINES['HAVE_ENCODER_LZMA2'] = 1 > +DEFINES['HAVE_MF_BT4'] = HOST_DEFINES['HAVE_MF_BT4'] = 1 > + > +FORCE_STATIC_LIB = True That's the default. ::: modules/xz/moz.build:117 (Diff revision 7) > + # Some of the liblzma files assume that DWORD is defined whenever MSC_VER > + # is set, and also that memmove and memcpy can be used without declarations. memmove and memcpy are in string.h. ::: modules/xz/moz.build:127 (Diff revision 7) > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > + CFLAGS += ['-FIcommon.h'] > + HOST_CFLAGS += ['-FIcommon.h'] > +else: > + CFLAGS += ['-include', 'common.h'] > + HOST_CFLAGS += ['-include', 'common.h'] Why not add a config.h and define HAVE_CONFIG_H? It also feels like whatever the liblzma build system is doing, it's putting something in config.h for DWORD, memmove and memcpy... might be worth looking at what it does, and do something similar, instead of force-including windows.h.
Attachment #8884666 -
Flags: review?(mh+mozilla)
Comment 193•7 years ago
|
||
mozreview-review |
Comment on attachment 8884667 [details] Bug 641212 Part 2 - Adjust mar/signmar/updater build files to link liblzma instead of bzip. https://reviewboard.mozilla.org/r/155532/#review166074 ::: modules/libmar/moz.build:10 (Diff revision 7) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > with Files('**'): > BUG_COMPONENT = ('Toolkit', 'Application Update') > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android': This doesn't seem right. At the very least, this should be separated out because this isn't directly related. Plus, I think some things are using mar for Fennec (was it Mozilla China?). ::: toolkit/mozapps/update/updater/updater-common.build (Diff revision 7) > > -if CONFIG['MOZ_SYSTEM_BZ2']: > - OS_LIBS += CONFIG['MOZ_BZ2_LIBS'] > +DEFINES['LZMA_API_STATIC'] = 1 > +HOST_DEFINES['LZMA_API_STATIC'] = 1 > -else: > - USE_LIBS += [ > - 'bz2', I'm confused. Looking around, it doesn't seem that mar depends on libbz2. How are we creating mars that require libbz2 in the updater if mar doesn't depend on libbz2? Corrolary: why do we need liblzma in mar if we did't need libbz2 there?
Attachment #8884667 -
Flags: review?(mh+mozilla)
Comment 194•7 years ago
|
||
mozreview-review |
Comment on attachment 8832672 [details] Bug 641212 Part 6 - Remove usage of bzip2 from the MAR builder scripts. https://reviewboard.mozilla.org/r/108864/#review166078 ::: tools/update-packaging/common.sh (Diff revision 9) > # > > # ----------------------------------------------------------------------------- > # By default just assume that these tools exist on our path > MAR=${MAR:-mar} > -BZIP2=${BZIP2:-bzip2} Ok, so this is how... Why are we not doing the same with xz, and avoid importing liblzma? Also, it seems like you can't remove bz2 production of mar files at the same time you add lzma support: you need to be able to produce mars for older releases that don't have lzma support in the updater (e.g. ESR, and the last version that will have bz2 support)
Assignee | ||
Comment 195•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #192) > Comment on attachment 8884666 [details] > Bug 641212 Part 1 - Incorporate liblzma into the build system. > > https://reviewboard.mozilla.org/r/155530/#review166070 > > I'm rather uneased by the addition of another xz decompressor. For > compression, we don't have that in the tree, so it's understandable, but why > not use xz-embedded for the updater (it would probably be smaller, too) xz isn't available on all the build systems and the last thing I requested to have added took over a year. This might also provides flexibility with tuning how files are compressed. > ::: modules/xz/moz.build:98 > (Diff revision 7) > > +DEFINES['LZMA_API_STATIC'] = HOST_DEFINES['LZMA_API_STATIC'] = 1 > > +DEFINES['HAVE_INTTYPES_H'] = HOST_DEFINES['HAVE_INTTYPES_H'] = 1 > > +DEFINES['HAVE_STDBOOL_H'] = HOST_DEFINES['HAVE_STDBOOL_H'] = 1 > > +DEFINES['HAVE_STRING_H'] = HOST_DEFINES['HAVE_STRING_H'] = 1 > > +DEFINES['HAVE_CHECK_CRC32'] = HOST_DEFINES['HAVE_CHECK_CRC32'] = 1 > > +DEFINES['HAVE_CHECK_CRC64'] = HOST_DEFINES['HAVE_CHECK_CRC64'] = 1 > > +DEFINES['HAVE_DECODER_LZMA2'] = HOST_DEFINES['HAVE_DECODER_LZMA2'] = 1 > > +DEFINES['HAVE_ENCODER_LZMA2'] = HOST_DEFINES['HAVE_ENCODER_LZMA2'] = 1 > > +DEFINES['HAVE_MF_BT4'] = HOST_DEFINES['HAVE_MF_BT4'] = 1 > > Some of these look like they should be set to True rather than 1. Other products that use xz use 1 https://insight.io/github.com/freebsd/freebsd/blob/master/lib/liblzma/config.h <snip> > ::: modules/xz/moz.build:127 > (Diff revision 7) > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > > + CFLAGS += ['-FIcommon.h'] > > + HOST_CFLAGS += ['-FIcommon.h'] > > +else: > > + CFLAGS += ['-include', 'common.h'] > > + HOST_CFLAGS += ['-include', 'common.h'] > > Why not add a config.h and define HAVE_CONFIG_H? I added a config.h in attachment #8889276 [details] [diff] [review] while Matt was on PTO since we are trying to finish this up. (In reply to Mike Hommey [:glandium] from comment #193) > Comment on attachment 8884667 [details] > Bug 641212 Part 2 - Adjust mar/signmar/updater build files to link liblzma > instead of bzip. > > https://reviewboard.mozilla.org/r/155532/#review166074 > > ::: modules/libmar/moz.build:10 > (Diff revision 7) > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > with Files('**'): > > BUG_COMPONENT = ('Toolkit', 'Application Update') > > > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android': > > This doesn't seem right. At the very least, this should be separated out > because this isn't directly related. Plus, I think some things are using mar > for Fennec (was it Mozilla China?). The updater which consumes the mar files hasn't been used for Fennec for several years now. Bug 1152997 Bug 1152634 There will need to be an update watershed for the Windows 32 bit to 64 bit Firefox migration and this will hopefully land in the same cycle along with the SHA384 support ( bug 1324498 and friends). With the watershed there isn't a requirement to support both beyond having both the bzip2 version and the lzma version of the tools available to generate the mar files. Since this cycle is almost over we plan on removing the bz2 code later.
Comment 196•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #195) > (In reply to Mike Hommey [:glandium] from comment #192) > > Comment on attachment 8884666 [details] > > Bug 641212 Part 1 - Incorporate liblzma into the build system. > > > > https://reviewboard.mozilla.org/r/155530/#review166070 > > > > I'm rather uneased by the addition of another xz decompressor. For > > compression, we don't have that in the tree, so it's understandable, but why > > not use xz-embedded for the updater (it would probably be smaller, too) > xz isn't available on all the build systems and the last thing I requested > to have added took over a year. How many years ago was that? ;) For one, xz is already there on linux builders, which covers linux and mac, now. For windows, it's possible to put xz in tooltool and have it available on all the relevant jobs. This merely takes minutes, as long as you find someone to upload xz to tooltool (hint: I can). > This might also provides flexibility with tuning how files are compressed. That's also possible with xz...
Assignee | ||
Comment 197•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #196) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #195) > > (In reply to Mike Hommey [:glandium] from comment #192) > > > Comment on attachment 8884666 [details] > > > Bug 641212 Part 1 - Incorporate liblzma into the build system. > > > > > > https://reviewboard.mozilla.org/r/155530/#review166070 > > > > > > I'm rather uneased by the addition of another xz decompressor. For > > > compression, we don't have that in the tree, so it's understandable, but why > > > not use xz-embedded for the updater (it would probably be smaller, too) > > xz isn't available on all the build systems and the last thing I requested > > to have added took over a year. > > How many years ago was that? ;) For one, xz is already there on linux > builders, which covers linux and mac, now. For windows, it's possible to put > xz in tooltool and have it available on all the relevant jobs. This merely > takes minutes, as long as you find someone to upload xz to tooltool (hint: I > can). Bug 1236624 > > This might also provides flexibility with tuning how files are compressed. > > That's also possible with xz...
Assignee | ||
Comment 198•7 years ago
|
||
Note: I don't know all of the different types of build systems we have nowadays but at a minimum it will be used by the systems that generate the mar files and run xpcshell tests.
Assignee | ||
Comment 199•7 years ago
|
||
Mike, would you be able to get this deployed to the build systems and in the path soon? If so, either Matt or myself will update the patches. After thinking about it I think it is just needed on the systems that generate the mar files.
Flags: needinfo?(mh+mozilla)
Comment 200•7 years ago
|
||
Add the following snippet to the tooltool manifest used for the job that generates the mar files, and you should be good to go: { "size": 216733, "digest": "7debd8ce46955eae345ebdf2ee6df3bfc3f3a547af1306f7fb099e38750f5ed9412af7155017574fc796f2bb8f37b5b7c4f91d838fa3dc422a776c230ba2d64b", "algorithm": "sha512", "filename": "xz.tar.bz2", "unpack": true } (For the record, this was generated from the contents of the bin_x86-64 in the xz-5.2.3-windows.zip archive from https://tukaani.org/xz/ (gpg validated)) I don't know which jobs are the ones the generate the mar files, though, so I can't point you to the right place.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 201•7 years ago
|
||
I don't think I have rights to tooltool. Any suggestions as to who might be able to do this today? nthomas, bhearsum, catlee: can you provide the info above or add this to tooltool?
Flags: needinfo?(nthomas)
Flags: needinfo?(catlee)
Flags: needinfo?(bhearsum)
Comment 202•7 years ago
|
||
glandium is saying he's added an archive containing xz executables to tooltool. It has liblzma.a liblzma.dll lzmadec.exe lzmainfo.exe xz.exe xzdec.exe I don't know if there is some magic which gets the unpack location onto the path and/or include path. The nightly/release builds that create complete mar files use these manifests browser/config/tooltool-manifests/win32/releng.manifest browser/config/tooltool-manifests/win64/releng.manifest
Flags: needinfo?(nthomas)
Comment 203•7 years ago
|
||
FYI, that xz archive can be retrieved from https://api.pub.build.mozilla.org/tooltool/sha512/7debd8ce46955eae345ebdf2ee6df3bfc3f3a547af1306f7fb099e38750f5ed9412af7155017574fc796f2bb8f37b5b7c4f91d838fa3dc422a776c230ba2d64b
Comment 204•7 years ago
|
||
> I don't know if there is some magic which gets the unpack location onto the path and/or include path.
There isn't. But all that is needed is to execute xz.exe from the xz directory under whatever directory the tooltool packages are unpacked in ; i.e. it shouldn't be necessary to have it in $PATH as long as you call it with a full path.
Comment 206•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #204) > > I don't know if there is some magic which gets the unpack location onto the path and/or include path. > > There isn't. But all that is needed is to execute xz.exe from the xz > directory under whatever directory the tooltool packages are unpacked in ; > i.e. it shouldn't be necessary to have it in $PATH as long as you call it > with a full path. How do we find that path? I know very little about how tooltool works and nothing about where it unpacks things. Also, we would need to keep this working for local builds, since we need the libmar tests to be able to run locally. MozillaBuild has a copy of xz that gets put in the PATH, so that takes care of Windows, but I don't know what if anything needs to be done for Linux or native Mac; I'm assuming xz-utils should be added to the bootstrap scripts?
Updated•7 years ago
|
Flags: needinfo?(mh+mozilla)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(catlee)
Comment 207•7 years ago
|
||
(In reply to Matt Howell [:mhowell] from comment #206) > (In reply to Mike Hommey [:glandium] from comment #204) > > > I don't know if there is some magic which gets the unpack location onto the path and/or include path. > > > > There isn't. But all that is needed is to execute xz.exe from the xz > > directory under whatever directory the tooltool packages are unpacked in ; > > i.e. it shouldn't be necessary to have it in $PATH as long as you call it > > with a full path. > > How do we find that path? I know very little about how tooltool works and > nothing about where it unpacks things. That pretty much depends on the job that runs that. I don't know what job that is, so I can't say anything. > Also, we would need to keep this working for local builds, since we need the > libmar tests to be able to run locally. MozillaBuild has a copy of xz that > gets put in the PATH, so that takes care of Windows, but I don't know what > if anything needs to be done for Linux or native Mac; I'm assuming xz-utils > should be added to the bootstrap scripts? What libmar tests are you talking about? I don't think the tests in modules/libmar currently require a bzip2 executable, why would they require a xz one?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 208•7 years ago
|
||
Here is the log for an oak nightly build that creates the mar files and will need xz. It is a tc (N) build on Linux. https://public-artifacts.taskcluster.net/CpnarFjXREu1ZuWNcBFlMg/0/public/logs/live_backing.log The libmar tests won't need xz.
Comment 209•7 years ago
|
||
So, xz will presumably be in $topsrcdir/xz, but the log is for a linux job, not a windows job, so I can only presume.
Assignee | ||
Comment 210•7 years ago
|
||
Here is a win32 build that creates a mar file https://archive.mozilla.org/pub/firefox/nightly/2017/07/2017-07-25-04-02-04-oak/oak-win32-nightly-bm91-build1-build2.txt.gz and a win64 build that creates a mar file https://archive.mozilla.org/pub/firefox/nightly/2017/07/2017-07-25-04-02-04-oak/oak-win64-nightly-bm91-build1-build3.txt.gz OS X https://public-artifacts.taskcluster.net/Ev8rAeAcQd2zJNZTPbb6ug/0/public/logs/live_backing.log
Comment 211•7 years ago
|
||
So, those windows builds will have xz in $topsrcdir/xz. The linux and mac builds will just have it in $PATH from the system.
Assignee | ||
Comment 212•7 years ago
|
||
Thank you!
Updated•7 years ago
|
Attachment #8832668 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8884666 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8884667 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832670 -
Attachment is obsolete: true
Attachment #8832670 -
Flags: review?(robert.strong.bugs)
Updated•7 years ago
|
Attachment #8832669 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832671 -
Attachment is obsolete: true
Attachment #8832671 -
Flags: review?(robert.strong.bugs)
Updated•7 years ago
|
Attachment #8832672 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832673 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832674 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832675 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832676 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889271 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889272 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889273 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889275 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889276 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889277 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889278 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889279 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889280 -
Attachment is obsolete: true
Assignee | ||
Comment 213•7 years ago
|
||
Assignee: mhowell → robert.strong.bugs
Attachment #8890501 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 214•7 years ago
|
||
Attachment #8890502 -
Flags: review?(mhowell)
Assignee | ||
Comment 215•7 years ago
|
||
Attachment #8890503 -
Flags: review?(mhowell)
Assignee | ||
Comment 216•7 years ago
|
||
Attachment #8890504 -
Flags: review?(mhowell)
Assignee | ||
Comment 217•7 years ago
|
||
Attachment #8890506 -
Flags: review?(mhowell)
Assignee | ||
Updated•7 years ago
|
Attachment #8890506 -
Flags: review?(bhearsum)
Assignee | ||
Comment 218•7 years ago
|
||
I landed all of these patches and the SHA384 patches from bug 1105689 on oak. All tests pass. Mars were generated for Linux and OS X but not Windows. I landed the mar.py changes there in the hope that fixes Windows mar generation and Callek triggered a new nightly on oak for me.
Updated•7 years ago
|
Attachment #8890503 -
Flags: review?(mhowell) → review+
Updated•7 years ago
|
Attachment #8890504 -
Flags: review?(mhowell) → review+
Comment 219•7 years ago
|
||
Comment on attachment 8890502 [details] [diff] [review] Part 2 - client code Review of attachment 8890502 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +301,5 @@ > break; > } > > + if (offset < (int) item->length && strm.in_pos == strm.in_size) { > + inlen = mar_read(mArchive, item, offset, inbuf, sizeof(inbuf)); This should still be inbuf_size; sizeof(inbuf) won't return the size of the buffer because inbuf is declared as a pointer and dynamically allocated, it's not an array. Alternatively, is there a reason we can't just make inbuf and outbuf static arrays? @@ +302,5 @@ > } > > + if (offset < (int) item->length && strm.in_pos == strm.in_size) { > + inlen = mar_read(mArchive, item, offset, inbuf, sizeof(inbuf)); > + strm.in_size = inlen; We should still be checking the return value from mar_read before just using it. @@ +309,5 @@ > } > > + xz_rv = xz_dec_run(dec, &strm); > + > + if (strm.out_pos == sizeof(outbuf)) { As above, I don't think sizeof is doing what we want here. @@ +333,2 @@ > } > + break; The flow through this loop is a little tough to follow; I think it should be possible to keep it structured pretty much like the old one was, and it seems like that would be a bit clearer.
Comment 220•7 years ago
|
||
Comment on attachment 8890506 [details] [diff] [review] Part 5 - mar generation scripts Review of attachment 8890506 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/repackaging/mar.py @@ +46,5 @@ > > env = os.environ.copy() > env['MOZ_FULL_PRODUCT_VERSION'] = get_application_ini_value(tmpdir, 'App', 'Version') > env['MAR'] = mozpath.normpath(mar) > + # The Windows build systems have xz installed but it isn't in the path I thought the problem was that XZ wasn't on those systems at all? It's included in MozillaBuild, which just puts it in /bin, so I thought it was somehow getting removed and that was the issue. @@ +49,5 @@ > env['MAR'] = mozpath.normpath(mar) > + # The Windows build systems have xz installed but it isn't in the path > + # like it is on Linux and Mac OS X so just use the XZ env var so the mar > + # generation scripts can find it. > + xz_path = mozpath.join(topsrcdir, 'xz/xz.exe') I think the right entry needs to be added to the tooltool manifests before this directory will exist. ::: tools/update-packaging/make_incremental_updates.py @@ +45,5 @@ > m = re.match("((?:|.*/)distribution/extensions/.*)/", filename) > if m: > # Directory immediately following extensions is used for the test > testdir = m.group(1) > + print(' add-if "'+testdir+'" "'+filename+'"') It looks like this file's been through a Python 3 conversion. I assume that's okay? @@ +410,5 @@ > patch_info.create_manifest_files() > > # And construct the mar > + mar_cmd = 'mar -C '+patch_info.work_dir+' -c output.mar '+' '.join(patch_info.archive_files) > + # mar_cmd = 'mar -C '+patch_info.work_dir+' -c output.mar '+string.join(patch_info.archive_files, ' ') Should remove the commented-out line.
Assignee | ||
Comment 221•7 years ago
|
||
(In reply to Matt Howell [:mhowell] from comment #220) > Comment on attachment 8890506 [details] [diff] [review] > Part 5 - mar generation scripts > > Review of attachment 8890506 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/repackaging/mar.py > @@ +46,5 @@ > > > > env = os.environ.copy() > > env['MOZ_FULL_PRODUCT_VERSION'] = get_application_ini_value(tmpdir, 'App', 'Version') > > env['MAR'] = mozpath.normpath(mar) > > + # The Windows build systems have xz installed but it isn't in the path > > I thought the problem was that XZ wasn't on those systems at all? It's > included in MozillaBuild, which just puts it in /bin, so I thought it was > somehow getting removed and that was the issue. The build systems don't install MozillaBuild. In the past they have cherry picked what they need and per comment #211 it does have xz located in $topsrcdir/xz > @@ +49,5 @@ > > env['MAR'] = mozpath.normpath(mar) > > + # The Windows build systems have xz installed but it isn't in the path > > + # like it is on Linux and Mac OS X so just use the XZ env var so the mar > > + # generation scripts can find it. > > + xz_path = mozpath.join(topsrcdir, 'xz/xz.exe') > > I think the right entry needs to be added to the tooltool manifests before > this directory will exist. Quite possibly. Will know more after tonight's nightly on oak > ::: tools/update-packaging/make_incremental_updates.py > @@ +45,5 @@ > > m = re.match("((?:|.*/)distribution/extensions/.*)/", filename) > > if m: > > # Directory immediately following extensions is used for the test > > testdir = m.group(1) > > + print(' add-if "'+testdir+'" "'+filename+'"') > > It looks like this file's been through a Python 3 conversion. I assume > that's okay? It still works with python 2.7 > @@ +410,5 @@ > > patch_info.create_manifest_files() > > > > # And construct the mar > > + mar_cmd = 'mar -C '+patch_info.work_dir+' -c output.mar '+' '.join(patch_info.archive_files) > > + # mar_cmd = 'mar -C '+patch_info.work_dir+' -c output.mar '+string.join(patch_info.archive_files, ' ') > > Should remove the commented-out line. Will do
Assignee | ||
Comment 222•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #221) > (In reply to Matt Howell [:mhowell] from comment #220) > > Comment on attachment 8890506 [details] [diff] [review] > > Part 5 - mar generation scripts > > > > Review of attachment 8890506 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: python/mozbuild/mozbuild/repackaging/mar.py > > @@ +46,5 @@ > > > > > > env = os.environ.copy() > > > env['MOZ_FULL_PRODUCT_VERSION'] = get_application_ini_value(tmpdir, 'App', 'Version') > > > env['MAR'] = mozpath.normpath(mar) > > > + # The Windows build systems have xz installed but it isn't in the path > > > > I thought the problem was that XZ wasn't on those systems at all? It's > > included in MozillaBuild, which just puts it in /bin, so I thought it was > > somehow getting removed and that was the issue. > The build systems don't install MozillaBuild. In the past they have cherry > picked what they need and per comment #211 it does have xz located in > $topsrcdir/xz See bug 1236624 comment #44 and bug 1236624 comment #45 for an example of how NSIS was cherry picked from MozillaBuild and deployed to the build systems.
Assignee | ||
Comment 223•7 years ago
|
||
Attachment #8890506 -
Attachment is obsolete: true
Attachment #8890506 -
Flags: review?(mhowell)
Attachment #8890506 -
Flags: review?(bhearsum)
Attachment #8890583 -
Flags: review?(mhowell)
Assignee | ||
Updated•7 years ago
|
Attachment #8890583 -
Flags: review?(bhearsum)
Assignee | ||
Updated•7 years ago
|
Attachment #8890502 -
Attachment is obsolete: true
Attachment #8890502 -
Flags: review?(mhowell)
Assignee | ||
Comment 224•7 years ago
|
||
Comment on attachment 8890583 [details] [diff] [review] Part 5 - mar generation scripts >diff --git a/tools/update-packaging/Makefile.in b/tools/update-packaging/Makefile.in >--- a/tools/update-packaging/Makefile.in >+++ b/tools/update-packaging/Makefile.in >@@ -16,16 +16,17 @@ STAGE_DIR = $(ABS_DIST)/$(PKG_UPDAT > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)) > PACKAGE_DIR = $(PACKAGE_BASE_DIR)/$(MOZ_PKG_DIR)/$(MOZ_MACBUNDLE_NAME) > else > PACKAGE_DIR = $(PACKAGE_BASE_DIR)/$(MOZ_PKG_DIR) > endif > > MAR_BIN = $(DIST)/host/bin/mar$(HOST_BIN_SUFFIX) > MBSDIFF_BIN = $(DIST)/host/bin/mbsdiff$(HOST_BIN_SUFFIX) >+XZ_BIN_WIN = $(topsrcdir)/xz/xz.exe > > OVERRIDE_DEFAULT_GOAL := full-update > full-update:: complete-patch > > ifeq ($(OS_TARGET), WINNT) > MOZ_PKG_FORMAT := SFX7Z > UNPACKAGE = '$(subst $(DIST),$(ABS_DIST),$(INSTALLER_PACKAGE))' > ifdef AB_CD >@@ -45,27 +46,31 @@ endif > > dir-stage := $(call mkdir_deps,$(STAGE_DIR)) > > complete-patch:: $(dir-stage) > ifeq ($(OS_TARGET), WINNT) > test -f $(UNPACKAGE) > $(RM) -rf '$(PACKAGE_DIR)' > cd $(PACKAGE_BASE_DIR) && $(INNER_UNMAKE_PACKAGE) >+ XZ=$(XZ_BIN_WIN) \ > endif > MAR=$(MAR_BIN) \ > MOZ_PRODUCT_VERSION=$(MOZ_APP_VERSION) \ > $(srcdir)/make_full_update.sh \ > '$(DIST)/$(COMPLETE_MAR)' \ > '$(PACKAGE_DIR)' > ifdef MOZ_SIGN_CMD > $(MOZ_SIGN_CMD) -f mar '$(DIST)/$(COMPLETE_MAR)' > endif > > partial-patch:: $(dir-stage) >+ifeq ($(OS_TARGET), WINNT) >+ XZ=$(XZ_BIN_WIN) \ >+endif > MAR=$(MAR_BIN) \ > MBSDIFF=$(MBSDIFF_BIN) \ > MOZ_PRODUCT_VERSION=$(MOZ_APP_VERSION) \ > $(srcdir)/make_incremental_update.sh \ > '$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$(SRC_BUILD_ID)-$(DST_BUILD_ID).mar' \ > '$(SRC_BUILD)' \ > '$(DST_BUILD)' > ifdef MOZ_SIGN_CMD I don't think the changes to this Makefile are necessary since mar.py appears to be used by the build systems but I would prefer having some form of it since this code exists in this Makefile. Note: with the mar.py change it was possible to generate a Windows Firefox 64 complete mar file on oak.
Assignee | ||
Comment 225•7 years ago
|
||
Not sure if the following is needed +if CONFIG['OS_ARCH'] == 'WINNT': + USE_STATIC_LIBS = True
Attachment #8890501 -
Attachment is obsolete: true
Attachment #8890501 -
Flags: review?(mh+mozilla)
Attachment #8890720 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 226•7 years ago
|
||
Matt, though I am not entirely satisfied with doing so I decided to revert the patch file crc check back to bzip. This way the same mbsdiff can be used for the old format mar files and the new format mar files which will make things easier for releng. I did reduce it to only including crctable.c. I cleaned up the loop a little and added comments. Regarding "is there a reason we can't just make inbuf and outbuf static arrays?" we might go back to that but I'd prefer not trying to evaluate whether it is worth doing so in this bug.
Attachment #8890725 -
Flags: review?(mhowell)
Assignee | ||
Comment 227•7 years ago
|
||
Hey Ben, it was requested that the ability to use the same scripts for generating old format and new format mar files (signing is handled by the signing servers so that isn't addressed by this patch). This provides the ability to take the old format path when an env var name MAR_OLD_FORMAT is present for the mar shell scripts so RelEnd can more easily generate mar files for the 2 formats. This does not add that same support to the python script since I've been told it currently isn't used. This does update the python script for the new format since I find it valuable to verify things are working properly using the mar script tests. I also updated the python script so it works with both Python 2.7 and 3.x. My main concern with these changes is funsize since I have next to no knowledge of how it works. If there are changes required for funsize it would be helpful if someone else that has a better understanding of it could make those changes. Thanks
Attachment #8890583 -
Attachment is obsolete: true
Attachment #8890583 -
Flags: review?(mhowell)
Attachment #8890583 -
Flags: review?(bhearsum)
Attachment #8890731 -
Flags: review?(bhearsum)
Assignee | ||
Comment 228•7 years ago
|
||
Ben, forgot to mention that though it is possible to make the scripts cleaner I would prefer not doing so since supporting both is only temporary and it will be easier to remove support with how it is written.
Assignee | ||
Comment 229•7 years ago
|
||
Had to update the test partial mars. Carrying forward r+
Attachment #8890504 -
Attachment is obsolete: true
Attachment #8890761 -
Flags: review+
Assignee | ||
Comment 230•7 years ago
|
||
bah... build failures
Attachment #8890797 -
Flags: review?(mhowell)
Assignee | ||
Updated•7 years ago
|
Attachment #8890725 -
Attachment is obsolete: true
Attachment #8890725 -
Flags: review?(mhowell)
Comment 231•7 years ago
|
||
Comment on attachment 8890731 [details] [diff] [review] Part 5 - mar generation scripts Review of attachment 8890731 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good to my eyes. It's tough to be 100% certain because of all of the different ways we call it. My biggest concern is that something may come up in the first or second Beta with this patch (that's 56.0b1 & b2, I think?). (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #228) > Ben, forgot to mention that though it is possible to make the scripts > cleaner I would prefer not doing so since supporting both is only temporary > and it will be easier to remove support with how it is written. I totally agree that this is the right way to go. Is the idea that we leave these until we uplift 56 -> beta, so that we can make use of them for Beta/Release 56.0 as well? And what about ESR59, when it comes along? I think we may need some extra work for its first release, unless we backport all of the LZMA work to ESR52. I was looking over all of our callers and found a couple of red flags. Specifically, funsize explicitly uses mozilla-central to download a couple of tools: https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/taskcluster/docker/funsize-update-generator/scripts/funsize.py#125 https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/taskcluster/docker/funsize-update-generator/scripts/funsize.py#141 We already use branch-specific tools for a couple of scripts, so it seems like we should do that for all of them... Rail, can you answer the question about the branch specific tools? Also, if you have time before you're gone for PTO, can you have a look at the patch as well? You know the funsize integrations better than I do.
Attachment #8890731 -
Flags: review?(rail)
Attachment #8890731 -
Flags: review?(bhearsum)
Attachment #8890731 -
Flags: review+
Comment 232•7 years ago
|
||
Comment on attachment 8890797 [details] [diff] [review] Part 2 - client code Review of attachment 8890797 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #226) > Matt, though I am not entirely satisfied with doing so I decided to revert > the patch file crc check back to bzip. This way the same mbsdiff can be used > for the old format mar files and the new format mar files which will make > things easier for releng. I did reduce it to only including crctable.c. > > I cleaned up the loop a little and added comments. Regarding "is there a > reason we can't just make inbuf and outbuf static arrays?" we might go back > to that but I'd prefer not trying to evaluate whether it is worth doing so > in this bug. Fair enough, on both counts. I was a little worried about changing out the CRC code there, though my patches also had that change. And the comments have helped with the decompression loop, so I'll stop worrying about it. ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +343,1 @@ > break; This break is redundant with the next one.
Attachment #8890797 -
Flags: review?(mhowell) → review+
Comment 234•7 years ago
|
||
(In reply to Ben Hearsum (:bhearsum) from comment #231) > Comment on attachment 8890731 [details] [diff] [review] > Part 5 - mar generation scripts > > Review of attachment 8890731 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch looks good to my eyes. It's tough to be 100% certain because of > all of the different ways we call it. My biggest concern is that something > may come up in the first or second Beta with this patch (that's 56.0b1 & b2, > I think?). > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #228) > > Ben, forgot to mention that though it is possible to make the scripts > > cleaner I would prefer not doing so since supporting both is only temporary > > and it will be easier to remove support with how it is written. > > I totally agree that this is the right way to go. Is the idea that we leave > these until we uplift 56 -> beta, so that we can make use of them for > Beta/Release 56.0 as well? And what about ESR59, when it comes along? I > think we may need some extra work for its first release, unless we backport > all of the LZMA work to ESR52. > > I was looking over all of our callers and found a couple of red flags. > Specifically, funsize explicitly uses mozilla-central to download a couple > of tools: > https://dxr.mozilla.org/mozilla-central/rev/ > 7d2e89fb92331d7e4296391213c1e63db628e046/taskcluster/docker/funsize-update- > generator/scripts/funsize.py#125 > https://dxr.mozilla.org/mozilla-central/rev/ > 7d2e89fb92331d7e4296391213c1e63db628e046/taskcluster/docker/funsize-update- > generator/scripts/funsize.py#141 > > We already use branch-specific tools for a couple of scripts, so it seems > like we should do that for all of them... Using branch-specific is not enough here, funsize should also care about the revision (old vs new mar.exe actually), so we can unpack both complete.mar files. I filed bug 1379773 and Simon is going to look at it. > Rail, can you answer the question about the branch specific tools? Also, if > you have time before you're gone for PTO, can you have a look at the patch > as well? You know the funsize integrations better than I do. Sure!
Assignee | ||
Comment 235•7 years ago
|
||
(In reply to Ben Hearsum (:bhearsum) from comment #231) > Comment on attachment 8890731 [details] [diff] [review] > Part 5 - mar generation scripts > > Review of attachment 8890731 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch looks good to my eyes. It's tough to be 100% certain because of > all of the different ways we call it. My biggest concern is that something > may come up in the first or second Beta with this patch (that's 56.0b1 & b2, > I think?). > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #228) > > Ben, forgot to mention that though it is possible to make the scripts > > cleaner I would prefer not doing so since supporting both is only temporary > > and it will be easier to remove support with how it is written. > > I totally agree that this is the right way to go. Is the idea that we leave > these until we uplift 56 -> beta, so that we can make use of them for > Beta/Release 56.0 as well? And what about ESR59, when it comes along? I > think we may need some extra work for its first release, unless we backport > all of the LZMA work to ESR52. The scripts don't need to have support for MAR_OLD_FORMAT removed so i have no plan to remove it at this time. I suggest using it for both beta and release 56. For ESR I'd prefer just letting this ride the trains so ESR 59 would use MAR_OLD_FORMAT as well. Sometime after that the scripts can be cleaned up.
Comment 236•7 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #235) > (In reply to Ben Hearsum (:bhearsum) from comment #231) > > Comment on attachment 8890731 [details] [diff] [review] > > Part 5 - mar generation scripts > > > > Review of attachment 8890731 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This patch looks good to my eyes. It's tough to be 100% certain because of > > all of the different ways we call it. My biggest concern is that something > > may come up in the first or second Beta with this patch (that's 56.0b1 & b2, > > I think?). > > > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #228) > > > Ben, forgot to mention that though it is possible to make the scripts > > > cleaner I would prefer not doing so since supporting both is only temporary > > > and it will be easier to remove support with how it is written. > > > > I totally agree that this is the right way to go. Is the idea that we leave > > these until we uplift 56 -> beta, so that we can make use of them for > > Beta/Release 56.0 as well? And what about ESR59, when it comes along? I > > think we may need some extra work for its first release, unless we backport > > all of the LZMA work to ESR52. > The scripts don't need to have support for MAR_OLD_FORMAT removed so i have > no plan to remove it at this time. I suggest using it for both beta and > release 56. > > For ESR I'd prefer just letting this ride the trains so ESR 59 would use > MAR_OLD_FORMAT as well. Sometime after that the scripts can be cleaned up. Makes sense to me. We can kill MAR_OLD_FORMAT after we stop producing bzip mars everywhere, I guess.
Assignee | ||
Comment 237•7 years ago
|
||
I've used MAR_OLD_FORMAT when converting test mar files to the new format. I should be able to write a fairly simple script that will take a new format mar file and convert it to the old format mar file. From there it would just need to be signed.
Assignee | ||
Comment 238•7 years ago
|
||
though I think it will just be simpler to set MAR_OLD_FORMAT and let automation do the rest without conversion.
Assignee | ||
Comment 239•7 years ago
|
||
Conversion might be handy since a 56 RC build will use the new format and it could convert it to the old format
Assignee | ||
Comment 240•7 years ago
|
||
Removed redundant break... carrying forward r+
Attachment #8890797 -
Attachment is obsolete: true
Attachment #8891117 -
Flags: review+
Assignee | ||
Comment 241•7 years ago
|
||
I didn't update the partial_mac.mar correctly and it was causing tests that use it to fail. I tested this one locally and it passes. Carrying forward r+ on the test data files.
Attachment #8890761 -
Attachment is obsolete: true
Attachment #8891227 -
Flags: review+
Assignee | ||
Comment 242•7 years ago
|
||
All of these patches and the patches from bug 1105689 have been pushed to oak. Callek helped get mar sha384 signing and I verified the mars for Linux-i686, Linux-x86_64, Mac, and Win32 were all signed with the new sha384 certificate. The Win64 Nightly build failed early on in the process and hopefully the next nightly build will succeed. If all works correctly it will be possible to manually verify both this bug and bug 1105689 with the next nightly build.
Updated•7 years ago
|
Attachment #8890720 -
Attachment is patch: true
Comment 243•7 years ago
|
||
Comment on attachment 8890720 [details] [diff] [review] Part 1 - changes to xz-embed build config Review of attachment 8890720 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/xz-embedded/moz.build @@ +44,4 @@ > > Library('xz-embedded') > + > +HOST_SOURCES += common_sources I think you could do HOST_SOURCES += UNIFIED_SOURCES and HOST_DEFINES.update(DEFINES) OTOH, I see nothing that actually uses the host library, so I don't think you actually need all this (except the USE_STATIC_LIBS)
Attachment #8890720 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 244•7 years ago
|
||
Thanks and sorry about that... I forgot to remove that when I converted it to use xz from the build scripts. At least this simplifies the build changes.
Attachment #8890720 -
Attachment is obsolete: true
Attachment #8891243 -
Flags: review?(mh+mozilla)
Updated•7 years ago
|
Attachment #8891243 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 245•7 years ago
|
||
Rail and Ben, we should be able to land everything except Part 5 which contains the mar generation script changes. This will make it so the mars generated after this lands are created with bzip2 compression. After those mars are generated the update to those mars should be a watershed and Part 5 can land so future mars are generated with lzma.
Updated•7 years ago
|
Attachment #8890731 -
Flags: review?(rail) → review+
Assignee | ||
Comment 246•7 years ago
|
||
I cleaned up the perl script I used for converting the test mar files. The script will detect if the files contained in the mar are compressed using bzip2 and if not it assumes they are compressed with lzma. It could also detect if lzma was used but bzip2 should be enough. It will convert the mar file specified to the other compression method. As long as the script is run on Linux or Mac the permissions for the files contained in the mar file are retained. The MAR channel id is set to the same value as the original mar file. The MAR product version is set to the same value as the original mar file. bzip2, xz, and the mar binary must either be in the path or the associated name in uppercase needs to be in the environment with the value set to the path for binary. The script accepts an optional arg of -r to replace the original mar file with the new mar file. If that is not specified then the mar file name will have .xz appended to it when the mar file is converted to lzma and it will have .bz appended to it when the mar file is converted to bzip2.
Comment 247•7 years ago
|
||
Pushed by rstrong@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/89a4dd1ba384 use lzma compression for application update. Part 1 - xz-embedded build config changes. r=glandium, a=app_update_lzma https://hg.mozilla.org/mozilla-central/rev/692137868c6b use lzma compression for application update. Part 2 - add xe-embedded decompression support to the updater. r=mhowell, , a=app_update_lzma https://hg.mozilla.org/mozilla-central/rev/8fbee1d5b748 use lzma compression for application update. Part 3 - test changes to support the new test mar file size and additional logging. r=mhowell, a=app_update_lzma https://hg.mozilla.org/mozilla-central/rev/620ff920d666 use lzma compression for application update. Part 4 - app update test mar files created using lzma compression. r=mhowell, a=app_update_lzma
Assignee | ||
Updated•7 years ago
|
status-firefox54:
--- → wontfix
status-firefox55:
--- → wontfix
status-firefox56:
--- → fixed
status-firefox-esr52:
--- → wontfix
status-thunderbird_esr52:
--- → wontfix
Flags: in-testsuite+
Assignee | ||
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 248•7 years ago
|
||
Comment on attachment 8891601 [details] [diff] [review] perl script to change a mar file's contents compression Moved to bug 1385780
Attachment #8891601 -
Attachment is obsolete: true
Assignee | ||
Comment 249•7 years ago
|
||
Comment on attachment 8890731 [details] [diff] [review] Part 5 - mar generation scripts Moved to bug 1385780
Attachment #8890731 -
Attachment is obsolete: true
Updated•7 years ago
|
Target Milestone: --- → mozilla56
Assignee | ||
Comment 250•7 years ago
|
||
Since I created a script that converts a mar file's format from bzip2 to lzma and from lzma to bzip2 I went ahead and created mar files using both bzip2 and lzma using the same published mar file for both since it was trivial to do so. I noticed that the compression of the Release mar files is no where near as good as with the Nightly mar files so I did a little investigation. One possible reason for the difference might be the following bug which landed for 55: https://bugzilla.mozilla.org/show_bug.cgi?id=1351071 Release 54 Platform | BZip2 Size | LZMA Size | Diff Size | Reduction % | ---------------------+------------+------------+------------+-------------+ Linux 32 Complete | 61,436,243 | 53,031,650 | 8,404,593 | 13.68% | Linux 32 53.0.3-54.0 | 26,630,072 | 23,941,433 | 2,688,639 | 10.10% | Linux 64 Complete | 59,581,447 | 51,500,446 | 8,081,001 | 13.56% | Linux 64 53.0.3-54.0 | 26,089,825 | 23,543,381 | 2,546,444 | 9.76% | Mac OS X Complete | 58,078,137 | 50,220,147 | 7,857,990 | 13.53% | Mac OS X 53.0.3-54.0 | 21,617,242 | 20,329,144 | 1,288,098 | 5.96% | Win 32 Complete | 53,555,687 | 46,435,472 | 7,120,215 | 13.29% | Win 32 53.0.3-54.0 | 20,290,049 | 19,159,785 | 1,130,264 | 5.57% | Win 64 Complete | 57,613,587 | 49,459,165 | 8,154,422 | 14.15% | Win 64 53.0.3-54.0 | 22,784,086 | 20,912,940 | 1,871,146 | 8.21% | ---------------------+------------+------------+------------+-------------+ Beta 55 Platform | BZip2 Size | LZMA Size | Diff Size | Reduction % | ---------------------+------------+------------+------------+-------------+ Linux 32 Complete | 52,933,716 | 42,700,773 | 10,232,943 | 19.33% | Linux 32 beta8-beta9 | 6,466,007 | 5,399,865 | 1,066,142 | 16.49% | Linux 64 Complete | 51,047,155 | 41,167,509 | 9,879,646 | 19.35% | Linux 64 beta8-beta9 | 6,292,020 | 5,055,137 | 1,236,883 | 19.66% | Mac OS X Complete | 48,914,651 | 39,277,988 | 9,636,663 | 19.70% | Mac OS X beta8-beta9 | 2,504,568 | 2,742,315 | -237,747 | -9.50% | <- Note Win 32 Complete | 44,963,189 | 36,021,033 | 8,942,156 | 19.89% | Win 32 beta8-beta9 | 6,376,937 | 5,716,587 | 660,350 | 10.35% | Win 64 Complete | 49,351,510 | 39,207,722 | 10,143,788 | 20.55% | Win 64 beta8-beta9 | 7,928,124 | 6,594,670 | 1,333,454 | 16.82% | ---------------------+------------+------------+------------+-------------+ Note: based on the Release numbers it appears that when the mar size is small such as with a partial it is possible for the mar to be larger when using lzma. Nightly 56 Platform | BZip2 Size | LZMA Size | Diff Size | Reduction % | ---------------------+------------+------------+------------+-------------+ Linux 32 Complete | 61,464,591 | 48,378,986 | 13,085,605 | 21.29% | Linux 64 Complete | 63,035,418 | 50,050,490 | 12,984,928 | 20.60% | Mac OS X Complete | 63,458,063 | 49,883,327 | 13,574,736 | 21.39% | Win 32 Complete | 50,709,999 | 40,386,354 | 10,323,645 | 20.36% | Win 64 Complete | 55,088,628 | 43,498,291 | 11,590,337 | 21.04% | ---------------------+------------+------------+------------+-------------+ I'm not sure where the nightly partials are located nowadays and I think the beta numbers should suffice.
Comment 251•7 years ago
|
||
Verified as fixed. Test runs are available here: https://testrail.stage.mozaws.net/index.php?/reports/view/493
Status: RESOLVED → VERIFIED
Comment 252•7 years ago
|
||
bugherder uplift |
This was backed out from Beta prior to the creation of 56b1 by way of bug 1387231. https://hg.mozilla.org/releases/mozilla-beta/rev/c495c07bdff0 It has now been re-landed for 56b3 by way of bug 1387238. https://hg.mozilla.org/releases/mozilla-beta/rev/38f2acfc18e2 https://hg.mozilla.org/releases/mozilla-beta/rev/841c1a6eaa9c https://hg.mozilla.org/releases/mozilla-beta/rev/b51545b93e3b https://hg.mozilla.org/releases/mozilla-beta/rev/5f9d8dc39631
Comment 253•7 years ago
|
||
Added to the 56 release notes, "Reduced update download file size by approximately 20 percent" Also I want to say that is amazing, congratulations!
relnote-firefox:
--- → 56+
Updated•6 years ago
|
Whiteboard: [fxgrowth][fce-active] → [fxgrowth][fce-active-legacy]
You need to log in
before you can comment on or make changes to this bug.
Description
•