Status

()

enhancement
P1
normal
VERIFIED FIXED
8 years ago
a year ago

People

(Reporter: catlee, Assigned: rstrong)

Tracking

(Blocks 2 bugs)

Trunk
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 56+, thunderbird_esr52 wontfix, firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 verified)

Details

(Whiteboard: [fxgrowth][fce-active-legacy])

Attachments

(4 attachments, 43 obsolete attachments)

2.41 KB, patch
mhowell
: review+
Details | Diff | Splinter Review
8.87 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
205.67 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
1.59 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
Out of curiosity, was lzma2 also looked at? Also, was this with default compression settings?
(Reporter)

Comment 2

8 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.
Might be interesting to try out the LZMA SDK 9.20 with LZMA2 compression.
(Reporter)

Comment 4

8 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.
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

8 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

8 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.
Sure looks like LZMA2 isn't worth the extra effort and memory footprint. Thanks for checking!
(Reporter)

Comment 9

8 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.
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

7 years ago
Posted patch add xz support to updater (obsolete) — Splinter Review
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)
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

5 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

4 years ago
Whiteboard: [fxgrowth]
(Reporter)

Comment 15

4 years ago
Posted patch add xz support to updater (obsolete) — Splinter Review
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)

Updated

3 years ago
Blocks: 1303172
(Reporter)

Updated

3 years ago
See Also: → 1303180
(Reporter)

Comment 16

3 years ago
This adds support for decompressing xz archives in the updater. Tested on linux.
Attachment #8583049 - Attachment is obsolete: true
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

3 years ago
Just FYI - the xz-embedded work was cherry picked from bug 1307570.
Whiteboard: [fxgrowth] → [fxgrowth][fce-active]
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
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

3 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)
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

2 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

2 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.
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)
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)
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)
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)
Attachment #8815461 - Attachment is obsolete: true
Attachment #8815462 - Attachment is obsolete: true
Attachment #8815463 - Attachment is obsolete: true
Attachment #8815464 - Attachment is obsolete: true
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)
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?
(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.
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?
Status: NEW → ASSIGNED
Attachment #8820922 - Attachment is obsolete: true
Attachment #8820922 - Flags: review?(robert.strong.bugs)
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)
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).
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.
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+
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+
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+
(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
test_create.js is failing with
'xz' is not recognized as an internal or external command,
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 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-
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+
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+
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

2 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.
About to add bhearsum and glandium as reviewers for specific patches per rstrong's request.
Attachment #8884666 - Flags: review?(robert.strong.bugs) → review?(mh+mozilla)
Attachment #8884667 - Flags: review?(mh+mozilla)
Attachment #8832672 - Flags: review?(bhearsum)
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+
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+
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+
Pushed latest set of patches to oak
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.
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.
Ignore the part about spidermonkey... way too sleepy
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-
(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']
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

2 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
(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.

Comment 102

2 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

2 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)
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.
Attachment #8884666 - Flags: review?(robert.strong.bugs) → review?(mh+mozilla)
Attachment #8884667 - Flags: review?(mh+mozilla)
Attachment #8815906 - Attachment is obsolete: true
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)
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)
Attachment #8832668 - Flags: feedback?(gerv)
Note: since the updater wil no longer be using bzip2 the updater's licenses.html reference to bzip2 can likely be removed.
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-
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

2 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

2 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)
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.
(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/
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 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)
(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
Posted patch libmar mar files part 11 (obsolete) — Splinter Review
Matt was on PTO so I created patches that apply after his patches to fix a few issues and Matt will review them.
Attachment #8889271 - Attachment description: libmar mar files → libmar mar files part 11
Attachment #8889272 - Attachment description: libmar mar file extracted_certs → libmar mar file extracted certs part 12
(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.
Note: I removed the no pib tests since they were created for the transition between not having a pib and having a pib.
Landed all patches on oak along with bug 1105689
Matt, not sure why the lint opt Doc job fails but I think it is related to the license change.

Comment 192

2 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

2 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

2 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)
(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.
(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...
(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...
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.
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)
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)
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)
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)
> 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.
Looks like I'm no longer needed here.
Flags: needinfo?(bhearsum)
(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?
Flags: needinfo?(mh+mozilla)
(Reporter)

Updated

2 years ago
Flags: needinfo?(catlee)
(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)
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.
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.
So, those windows builds will have xz in $topsrcdir/xz. The linux and mac builds will just have it in $PATH from the system.
Attachment #8832668 - Attachment is obsolete: true
Attachment #8884666 - Attachment is obsolete: true
Attachment #8884667 - Attachment is obsolete: true
Attachment #8832670 - Attachment is obsolete: true
Attachment #8832670 - Flags: review?(robert.strong.bugs)
Attachment #8832669 - Attachment is obsolete: true
Attachment #8832671 - Attachment is obsolete: true
Attachment #8832671 - Flags: review?(robert.strong.bugs)
Attachment #8832672 - Attachment is obsolete: true
Attachment #8832673 - Attachment is obsolete: true
Attachment #8832674 - Attachment is obsolete: true
Attachment #8832675 - Attachment is obsolete: true
Attachment #8832676 - Attachment is obsolete: true
Assignee: mhowell → robert.strong.bugs
Attachment #8890501 - Flags: review?(mh+mozilla)
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.
Attachment #8890503 - Flags: review?(mhowell) → review+
Attachment #8890504 - Flags: review?(mhowell) → review+
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 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.
(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
(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.
Attachment #8890506 - Attachment is obsolete: true
Attachment #8890506 - Flags: review?(mhowell)
Attachment #8890506 - Flags: review?(bhearsum)
Attachment #8890583 - Flags: review?(mhowell)
Attachment #8890502 - Attachment is obsolete: true
Attachment #8890502 - Flags: review?(mhowell)
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.
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)
Posted patch Part 2 - client code (obsolete) — Splinter Review
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)
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)
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.
Posted patch Part 4 - Test data files (obsolete) — Splinter Review
Had to update the test partial mars. Carrying forward r+
Attachment #8890504 - Attachment is obsolete: true
Attachment #8890761 - Flags: review+
Posted patch Part 2 - client code (obsolete) — Splinter Review
bah... build failures
Attachment #8890797 - Flags: review?(mhowell)
Attachment #8890725 - Attachment is obsolete: true
Attachment #8890725 - Flags: review?(mhowell)
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 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+

Updated

2 years ago
Duplicate of this bug: 1384894
(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!
(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.
(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.
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.
though I think it will just be simpler to set MAR_OLD_FORMAT and let automation do the rest without conversion.
Conversion might be handy since a 56 RC build will use the new format and it could convert it to the old format
Removed redundant break... carrying forward r+
Attachment #8890797 - Attachment is obsolete: true
Attachment #8891117 - Flags: review+
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+
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.
Attachment #8890720 - Attachment is patch: true
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)
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)
Attachment #8891243 - Flags: review?(mh+mozilla) → review+
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

2 years ago
Attachment #8890731 - Flags: review?(rail) → review+
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

2 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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
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
Comment on attachment 8890731 [details] [diff] [review]
Part 5 - mar generation scripts

Moved to bug 1385780
Attachment #8890731 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Depends on: 1385856
(Reporter)

Updated

2 years ago
Blocks: 1385865
Target Milestone: --- → mozilla56