LZMA-compress shared libraries before packaging

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

51 Branch
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

12.39 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.28 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.18 KB, patch
esawin
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
With bug 1294736 we enable linker support for LZMA-decompression.
In this bug, we enable LZMA-compression of shared libraries in our build system.
Assignee

Comment 1

3 years ago
Add xz-compression of shared libraries analogous to how we enabled szip-compression.

Patch 1 (missing) would be to add xz (utils) as a dependency during configuration, what's the proper way to do this now?
Attachment #8798168 - Flags: review?(mh+mozilla)
Assignee

Comment 2

3 years ago
Extract the libraries for static tests.
Attachment #8798169 - Flags: review?(mh+mozilla)
Comment on attachment 8798168 [details] [diff] [review]
0001-Bug-1307886-2.0-Compress-libraries-with-XZ-during-pa.patch

Review of attachment 8798168 [details] [diff] [review]:
-----------------------------------------------------------------

Seems this would all be simpler if you removed szip support, since we've disabled it and are unlikely to reenable it, and don't make xz optional.

::: mobile/android/confvars.sh
@@ +37,4 @@
>  
>  MOZ_APP_STATIC_INI=1
>  
> +if test "$COMPILE_ENVIRONMENT"; then

This shouldn't be compile-environment dependent. In fact, artifact builds should be xz'ing too (except the file would likely already be xz'ed).

::: python/mozbuild/mozbuild/action/package_fennec_apk.py
@@ +73,4 @@
>                      compress = False
> +                elif xz_compress_assets_libs:
> +                    print("xz-compressing %s" % p);
> +                    cmd = ["xz", "-zkf", "--check=crc32", mozpath.join(finder.base, p)]

don't add --check=crc32, and add the right BCJ filter depending on the target platform and MOZ_THUMB.

xz should be checked/searched for in configure, and a variable used here, instead of "xz".

@@ +74,5 @@
> +                elif xz_compress_assets_libs:
> +                    print("xz-compressing %s" % p);
> +                    cmd = ["xz", "-zkf", "--check=crc32", mozpath.join(finder.base, p)]
> +                    subprocess.check_output(cmd)
> +                    absXzPath = ''.join((os.path.abspath(f.path), '.xz'))

why do you need abspath here and below?
Attachment #8798168 - Flags: review?(mh+mozilla)
Comment on attachment 8798169 [details] [diff] [review]
0002-Bug-1307886-3.0-Extract-xz-compressed-libs-for-xpcsh.patch

Review of attachment 8798169 [details] [diff] [review]:
-----------------------------------------------------------------

Same comment about szip support.
Attachment #8798169 - Flags: review?(mh+mozilla)
Assignee

Comment 5

3 years ago
Addressed comments. I'll remove szip in patch 4.
Attachment #8798168 - Attachment is obsolete: true
Attachment #8800800 - Flags: review?(mh+mozilla)
Assignee

Comment 6

3 years ago
Remove szip support. I'm not sure this should go into this bug.
Attachment #8800801 - Flags: review?(mh+mozilla)
Assignee

Comment 7

3 years ago
Comment on attachment 8798169 [details] [diff] [review]
0002-Bug-1307886-3.0-Extract-xz-compressed-libs-for-xpcsh.patch

Can you give this another look, ignoring szip parts which are stripped in patch 4.
Attachment #8798169 - Flags: review?(mh+mozilla)
Assignee

Comment 8

3 years ago
Nick, is there anything else I need to do to make it work for artifact builds?
Flags: needinfo?(nalexander)
(In reply to Eugen Sawin [:esawin] from comment #8)
> Nick, is there anything else I need to do to make it work for artifact
> builds?

It's a little difficult for me to understand the state of the patches, but artifact builds should be fine provided:

1) the artifacts in automation are XZ-compressed;
2) locally, we either have xz without a compile environment (which would only be the case if it's now a pre-req for building Firefox for Android) or we have trying to invoke xz flagged off without a compile environment (which we have for szip);
3) xz is idempotent, meaning that re-xz-ing an already xz-compressed file is a no-op.

If you followed the szip example, which I think you did, these are probably all in place.
Flags: needinfo?(nalexander)
Assignee

Comment 10

3 years ago
Remove szip.cpp.
Attachment #8800801 - Attachment is obsolete: true
Attachment #8800801 - Flags: review?(mh+mozilla)
Attachment #8801105 - Flags: review?(mh+mozilla)
Assignee

Comment 11

3 years ago
(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Eugen Sawin [:esawin] from comment #8)
> > Nick, is there anything else I need to do to make it work for artifact
> > builds?
> 
> It's a little difficult for me to understand the state of the patches, but
> artifact builds should be fine provided:
> 
> 1) the artifacts in automation are XZ-compressed;

I've modified package_fennec_apk.py to use xz for compression.

> 2) locally, we either have xz without a compile environment (which would
> only be the case if it's now a pre-req for building Firefox for Android) or
> we have trying to invoke xz flagged off without a compile environment (which
> we have for szip);

With patch 2.1, we make xz a pre-req for building Fennec.

> 3) xz is idempotent, meaning that re-xz-ing an already xz-compressed file is
> a no-op.

To our disadvantage it isn't. Re-xz-ing will at least add redundant xz headers, which will not be accepted by some decoders, including XZ Embedded.
To avoid the issue, I added a check in package_fennec_apk.py to ignore already compressed files.

I don't know much about the way we handle/create artifact builds, so I'm trying to make sure I don't miss anything.
> I don't know much about the way we handle/create artifact builds, so I'm
> trying to make sure I don't miss anything.

With your previous bits, I think you're good.  People will yell if it's busted, and we'll fix it :)  It's not trivial to test a packaging change and artifacts at the same time in automation, so that's not a requirement.
Comment on attachment 8800800 [details] [diff] [review]
0001-Bug-1307886-2.1-Compress-libraries-with-XZ-during-pa.patch

Review of attachment 8800800 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +1310,4 @@
>  if test -n "$MOZ_LINKER"; then
>    AC_DEFINE(MOZ_LINKER)
>    MOZ_LINKER_EXTRACT=1
> +  AC_CHECK_PROGS(XZ_UTILS, xz)

XZ

@@ +1323,5 @@
> +  fi
> +  if test "$CPU_ARCH" = "x86"; then
> +    XZ_BCJ_X86=1
> +  fi
> +  AC_SUBST(XZ_BCJ_ARMTHUMB)

Please don't use substs for the BCJ filters. You have access to the value of CPU_ARCH and MOZ_THUMB2 in package_fennec_apk.py, through buildconfig.substs.

::: python/mozbuild/mozbuild/action/package_fennec_apk.py
@@ +29,4 @@
>                         assets_dirs=[],
>                         features_dirs=[],
>                         szip_assets_libs_with=None,
> +                       xz_compress_assets_libs_with=None,

Incidentally, this means you don't need to add more arguments to this function, nor additional command line arguments.

@@ +69,4 @@
>              compress = None  # Take default from Jarrer.
>              if p.endswith('.so'):
>                  # Asset libraries are special.
> +                if f.open().read(4) == 'SeZz' or f.open().read(5)[1:] == '7zXZ':

The point of removing szip was to make *this* patch simpler.
Attachment #8800800 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #13)
> Please don't use substs for the BCJ filters. You have access to the value of
> CPU_ARCH and MOZ_THUMB2 in package_fennec_apk.py, through buildconfig.substs.

Same for XZ.
Comment on attachment 8798169 [details] [diff] [review]
0002-Bug-1307886-3.0-Extract-xz-compressed-libs-for-xpcsh.patch

Review of attachment 8798169 [details] [diff] [review]:
-----------------------------------------------------------------

Same comment about szip support.

::: testing/remotecppunittests.py
@@ +76,5 @@
> +                        xz = None
> +                        with open(local_file) as f:
> +                            # Decompress xz-compressed file.
> +                            if f.read(5)[1:] == '7zXZ':
> +                                xz = ['xz', '-df', '--suffix', '.so', local_file]

You need to use XZ from the configure check.
Attachment #8798169 - Flags: review?(mh+mozilla)
Comment on attachment 8801105 [details] [diff] [review]
0003-Bug-1307886-4.1-Remove-szip-support.-r-glandium.patch

Review of attachment 8801105 [details] [diff] [review]:
-----------------------------------------------------------------

If you're going to remove code, please also remove SeekableZStream and related things. Or you can keep the szip code around, but remove szip support in the fennec packaging and configure, that would be enough for this bug. But please do so as part 1, not part 3.
Attachment #8801105 - Flags: review?(mh+mozilla)
Assignee

Comment 17

3 years ago
Remove szip packaging. We can remove szip source files in a new bug, if we want.
Attachment #8801105 - Attachment is obsolete: true
Attachment #8802273 - Flags: review?(mh+mozilla)
Assignee

Comment 18

3 years ago
Addressed comments.
We still need to check whether a library has already been xz-ed before compressing to avoid adding redundant xz headers which break decoding.
Attachment #8800800 - Attachment is obsolete: true
Attachment #8802274 - Flags: review?(mh+mozilla)
Assignee

Comment 19

3 years ago
Addressed comments.
Attachment #8798169 - Attachment is obsolete: true
Attachment #8802275 - Flags: review?(mh+mozilla)
Assignee

Comment 20

3 years ago
Patch 3.1 fails because buildconfig cannot be imported in testing/. How do I make it available there?
Flags: needinfo?(mh+mozilla)
All things considered, I guess the status quo using the literal 'xz' for the remote*.py scripts is the least bad option.
Flags: needinfo?(mh+mozilla)
Attachment #8802273 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8802274 [details] [diff] [review]
0002-Bug-1307886-2.2-Compress-libraries-with-XZ-during-pa.patch

Review of attachment 8802274 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/action/package_fennec_apk.py
@@ +85,5 @@
> +                        cmd.extend([bcj, '--lzma2'])
> +                    print('xz-compressing %s with %s' % (p, ' '.join(cmd)))
> +                    subprocess.check_output(cmd)
> +                    absXzPath = ''.join((os.path.abspath(f.path), '.xz'))
> +                    os.rename(absXzPath, os.path.abspath(f.path))

You shouldn't need the absolute paths here.
Attachment #8802274 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8802275 [details] [diff] [review]
0003-Bug-1307886-3.1-Extract-xz-compressed-libs-for-xpcsh.patch

Review of attachment 8802275 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 22.
Attachment #8802275 - Flags: review?(mh+mozilla)
Assignee

Comment 25

3 years ago
Attachment #8802275 - Attachment is obsolete: true
Attachment #8804334 - Flags: review?(mh+mozilla)
Assignee

Comment 26

3 years ago
Removed absolute paths.
Attachment #8802274 - Attachment is obsolete: true
Attachment #8804448 - Flags: review+
Attachment #8804334 - Flags: review?(mh+mozilla) → review+

Comment 28

3 years ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6f8253d511
[1.2] Remove szip support. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e85e8b3969
[2.3] Compress libraries with XZ during packaging. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/d523f32cf87f
[3.2] Extract xz-compressed libs for xpcshell and unit tests. r=glandium
Blocks: 1376704
You need to log in before you can comment on or make changes to this bug.