Closed Bug 1307886 Opened 8 years ago Closed 8 years ago

LZMA-compress shared libraries before packaging

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

51 Branch
defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(3 files, 7 obsolete files)

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
With bug 1294736 we enable linker support for LZMA-decompression.
In this bug, we enable LZMA-compression of shared libraries in our build system.
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)
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)
Addressed comments. I'll remove szip in patch 4.
Attachment #8798168 - Attachment is obsolete: true
Attachment #8800800 - Flags: review?(mh+mozilla)
Remove szip support. I'm not sure this should go into this bug.
Attachment #8800801 - Flags: review?(mh+mozilla)
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)
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)
Remove szip.cpp.
Attachment #8800801 - Attachment is obsolete: true
Attachment #8800801 - Flags: review?(mh+mozilla)
Attachment #8801105 - Flags: review?(mh+mozilla)
(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)
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)
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)
Addressed comments.
Attachment #8798169 - Attachment is obsolete: true
Attachment #8802275 - Flags: review?(mh+mozilla)
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)
Attachment #8802275 - Attachment is obsolete: true
Attachment #8804334 - Flags: review?(mh+mozilla)
Removed absolute paths.
Attachment #8802274 - Attachment is obsolete: true
Attachment #8804448 - Flags: review+
Attachment #8804334 - Flags: review?(mh+mozilla) → review+
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
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 52 → mozilla52
You need to log in before you can comment on or make changes to this bug.