1207888 - Extend mach artifact to unpack builds completely

Status: NEW

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

mach artifact currently only pulls .so files out of Android builds. This bug is to extend it to optionally pull every file in there, unpacking omni.ja at the same time. The code used by toolkit/mozapps/installer/unpack.py can be leveraged to achieve this.

Comment 1

[Nick Alexander :nalexander [he/him]]

I think the right approach here is to unpack using |make -C $OBJDIR/$MOZ_BUILD_APP/locales unpack|, which invokes the packager to possibly move files around; and then to run |mach python toolkit/mozapps/installer/unpack.py $OBJDIR/dist/l10n-stage| to unpack the omni.ja.  Since we don't want to conflate this with the l10n code, we may need to extract or duplicate some of that functionality.

On Linux, this should just work, since the package dist/bin are so close.

On Mac OS X, this won't be enough.  Unpacking will put binaries and libraries into the correct hierarchy in dist/bin/FirefoxNightly.app/..., but the build system expects to copy said artifacts from dist/bin into the hierarchy during the build.  I think the right thing to do here is to have the build system actually place binaries into the appropriate paths, rather than copying during the build, but I don't know how much work that is.

For Android builds, this also needs work.  Bug 873569 and Bug 887121 moved the artifact .so libraries into lib/ and assets/.  The packager copies from dist/bin into $STAGE/{lib,assets}/@ANDROID_CPU_ARCH@ (using the package-manifest.in destdir= mechanism), and the .so files are szipped in place.  |make unpack| does not move the .so files back into $STAGE, so they won't be in the right place for artifact builds.  That is:

build produces dist/bin/libxul.so

package copies dist/bin/libxul.so to $STAGE/assets/armeabi-v7a/libxul.so
package szips $STAGE/assets/armeabi-v7a/libxul.so
package stuffs assets/ into APK

unpack unzips APK
artifact copies APK contents; libxul.so is in assets/armeabi-v7a/libxul.so, so does not end up in dist/bin/libxul.so

I think the right thing to do here is to make the build system produce binaries that are szipped (in dist/bin) when appropriate, rather than having the packager be involved; and to claw back the destdir= mechanisms and manually move files around during package and unpack in upload-files.mk.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1207888 - Don't use 'destdir' in Android package-manifest.in files. r?glandium

Bug 1207888 - Don't use 'destdir' in Android package-manifest.in files. r?glandium

This manually moves the .so libraries from dist/bin into lib/ and
assets/ during 'mach package', and then replaces the .so libraries in
dist/bin during 'make -C objdir-droid/mobile/android/locales unpack'.

It's easier to bake this into the packaging Makefile than to track and
unwind the 'destdir' parameters during unpack.

Comment 3

[Nick Alexander :nalexander [he/him]]

glandium: the patch attached does not solve this entirely, it's just what's needed to make the "unpack entirely" approach have a hope of working for Android.

Overall, I think this is not the right approach for a few reasons, including the fact that Desktop needs some libraries (GMP related stuff) in sub-directories of dist/bin.

Comment 4

[Nick Alexander :nalexander [he/him]]

There's a big issue with how to update these "completely unpacked" dist/bin directories, since unpacked artifacts compete with files changed in the local build.  I don't know how to address this.

A concrete example of this:  since binary targets aren't handled at all in !COMPILE_ENVIRONMENT
builds, the master chrome.manifest does not include components/components.manifest and components/components.manifest does not include 'binary-component libbrowsercomps.dylib'.

If the build system writes chrome.manifest, we'll lose the reference to the binary components.  If the artifact version wins, artifact-based builds might not be able to  add things to the chrome.manifest.

See https://dxr.mozilla.org/mozilla-central/rev/cc48981c026c50fdf80d47b040ae1fb8fe99ad07/config/makefiles/target_binaries.mk#14.  (It's worth noting that Fennec artifact-based builds don't see this since they never use "binary-component" (via XPCOMBinaryComponent in moz.build).)

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8682056 [details]
MozReview Request: Bug 1207888 - Don't use 'destdir' in Android package-manifest.in files. r?glandium

https://reviewboard.mozilla.org/r/23963/#review23557

::: mobile/android/b2gdroid/installer/package-manifest.in:33
(Diff revision 1)
> -[assets destdir="assets/@ANDROID_CPU_ARCH@"]
> +[assets]

You don't need to keep the [assets] here, not the [libs] further down. The code to handle destdir=... (from bug 887121) could be removed too (in a followup).

::: python/mozbuild/mozbuild/action/zip.py:38
(Diff revision 1)
> -    jarrer = Jarrer(optimize=False)
> +    jarrer = Jarrer(optimize=False, compress=args.compress)
> +
> +    output = mozpath.join(args.C, args.zip)
> +    if args.update and os.path.exists(output):
> +        from mozpack.mozjar import JarWriter, JarReader
> +        for f in JarReader(output):
> +            jarrer.add(f.filename, DeflatedFile(f))
>  
>      with errors.accumulate():
>          finder = FileFinder(args.C)
>          for path in args.input:
>              for p, f in finder.find(path):
> -                jarrer.add(p, f)
> -        jarrer.copy(mozpath.join(args.C, args.zip))
> +                q = mozpath.join(args.D, p) if args.D else p
> +                jarrer.add(q, f)
> +        jarrer.copy(output)

I'd rather see this in a separate function with its own unit test ensuring we never regress the preservation of compression of files already in the archive when using -u.

Note this the zip.py improvements are independent and should probably go in their own patch, and even their own bug.

::: python/mozbuild/mozbuild/action/zip.py:42
(Diff revision 1)
> +        from mozpack.mozjar import JarWriter, JarReader

You're not using JarWriter

::: toolkit/mozapps/installer/upload-files.mk:464
(Diff revision 1)
> -    $(ZIP) -r9D $(_ABS_DIST)/gecko.ap_ assets && \
> +    $(call py_action,zip,--update -C . -D lib/$(ANDROID_CPU_ARCH) -0 $(_ABS_DIST)/gecko.ap_ $(LIB_SO_LIBRARIES)) && \

-C . is the default, no need to pass it.

::: toolkit/mozapps/installer/upload-files.mk:511
(Diff revision 1)
> +    mv lib/$(ANDROID_CPU_ARCH)/* . && \
> +    mv assets/$(ANDROID_CPU_ARCH)/* . \

I'd prefer it if dist/bin just had the files in {lib,assets}/$ANDROID_CPU_ARCH directly. On the other hand, I don't want to block you further on this.