901059 - Clean up old unused package-manifest.in references

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

package-manifest.in has references to desktop files that we don't use ... packing step generates warnings ... the references should be removed ...

Perform a local build once to get the entire warning list but basically you'll see:

Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:20: Missing file(s): bin/searchplugins/*
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:21: Missing file(s): bin/defaults/profile/bookmarks.html
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:22: Missing file(s): bin/defaults/profile/localstore.rdf
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:23: Missing file(s): bin/defaults/profile/mimeTypes.rdf
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:24: Missing file(s): bin/defaults/profile/chrome/*
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:43: Missing file(s): bin/libnssdbm3.so
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:51: Missing file(s): bin/AndroidManifest.xml
Warning: /home/mozilla/mozilla-fig-droid/mobile/android/installer/package-manifest:52: Missing file(s): bin/resources.arsc

Comment 1

[Shane Tully (:stully:]

Attachment: Bug 901059 - Clean up old unused package-manifest.in references

Try run: https://tbpl.mozilla.org/?tree=Try&rev=aa54f0b6fcb8

Sorry for taking a 'good-first-bug'; I'm blocked on a bunch of things and was looking for something small to do in the meantime.

Comment 2

[Mark Capella [:capella]]

Comment on attachment 786052 [details] [diff] [review]
Bug 901059 - Clean up old unused package-manifest.in references

I'm happy getting beginners started in the right direction, and you seem to have already been / gotten there :-)

For reviews I defer to someone like margaret or nalexander who are more familiar with the particular patch. 

Some of the things you've removed look to me like they should be left there, but I'd have to ask them why / why not also.

Comment 3

[Shane Tully (:stully:]

I just took the output from mach of "missing file(s)" and removed each from the manifest file. If you want, it's fine to leave this patch as is for a beginner to build off of or verify that what I removed should, in fact, be removed.

Comment 4

[Mark Capella [:capella]]

Oh ... I just meant you needed a proper review now, and we should ask nalexander or margaret to do that ... not that you shouldn't be working on this :(

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 786052 [details] [diff] [review]
Bug 901059 - Clean up old unused package-manifest.in references

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

I'm comfortable killing the chunk between line 96 and line 456; but I'd like to see a try build.  This patch is a very low reward for the risk of completing butchering the build; I'm not taking any chances.  Let's reduce scope, see a try, and then request r? from another build peer who is more familiar with desktop.

::: mobile/android/installer/package-manifest.in
@@ -17,5 @@
>  [@AB_CD@]
>  @BINPATH@/chrome/@AB_CD@@JAREXT@
>  @BINPATH@/chrome/@AB_CD@.manifest
>  @BINPATH@/@PREF_DIR@/mobile-l10n.js
> -@BINPATH@/searchplugins/*

Leave these.  Who knows what distributions do.

@@ +56,5 @@
>  #ifndef CROSS_COMPILE
>  @BINPATH@/@[e-mail]
>  @BINPATH@/@[e-mail]
>  #endif
> +#if ! defined(NSS_DISABLE_DBM) && ! defined(CROSS_COMPILE)

I don't get it.  The .chk only makes sense with the library.  (I don't think either are used on Android, but I can't say for certain.)  Leve all this.

@@ -83,2 @@
>  @BINPATH@/package-name.txt
>  @BINPATH@/classes.dex

I find this section very odd; why would we every include classes.dex in omni.ja?

@@ -88,2 @@
>  @BINPATH@/recommended-addons.json
> -@BINPATH@/distribution/*

We shouldn't kill this one: I'm not clear distributions don't use it.

@@ -457,2 @@
>  
>  ; [Default Preferences]

I'm comfortable killing the chunk between line 96 and line 456; but I'd like to see a try build.  This patch is a very low reward for the risk of completing butchering the build; I'm not taking any chances.

@@ -457,5 @@
>  
>  ; [Default Preferences]
>  ; All the pref files must be part of base to prevent migration bugs
>  @BINPATH@/@PREF_DIR@/mobile.js
> -@BINPATH@/@PREF_DIR@/mobile-branding.js

Leave these three.

@@ -529,5 @@
>  
>  ; [Crash Reporter]
>  ;
>  #ifdef MOZ_CRASHREPORTER
> -@BINPATH@/crashreporter@BIN_SUFFIX@

Leave these.

@@ -545,5 @@
>  bin/components/@DLL_PREFIX@nkgnomevfs@DLL_SUFFIX@
>  #endif
>  
>  [mobile]
> -@BINPATH@/chrome/icons/

Leave.

@@ -574,5 @@
>  @BINPATH@/components/Payment.manifest
>  @BINPATH@/components/PaymentsUI.js
>  
> -#ifdef MOZ_SAFE_BROWSING
> -@BINPATH@/components/SafeBrowsing.jsm

Leave.

Comment 7

[Mark Capella [:capella]]

This bug has been obviated by subsequent build system changes.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Capella [:capella] from comment #7)
> This bug has been obviated by subsequent build system changes.

I don't think it has -- we still have tons of warnings when we |mach package|, and only some are reasonable due to l10n repacks, build flags, etc.

I think we should stop calling this a good first bug, though.

Comment 9

[Mark Capella [:capella]]

thanks, that wfm :-)

This may not be what I originally noticed when reporting, but anything we can clean-up helps.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Current logs.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Fix up the manifest

This is a diff I prepared, using the help of some infrastructure I built for bug 1158018. Try is here: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed78cdde37d0>.

To confirm that I didn't botch anything, I ran this build on try: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5cbd1153952> which used some package-manifest diff stuff (<https://hg.mozilla.org/try/rev/661f59265ce7>). Yeah, that build's red, but the logs all indicate that there's nothing missing (a log where something was missing is <https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d0cd8ec8701>).

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8637605 [details] [diff] [review]
Fix up the manifest

Sadly, the distribution/* line is necessary: Bug 1163082.  There may be others.

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Nick Alexander :nalexander from comment #12)
> Comment on attachment 8637605 [details] [diff] [review]
> Fix up the manifest
> 
> Sadly, the distribution/* line is necessary: Bug 1163082.  There may be
> others.

The build cares to disagree if I try that:
 20:28:00 INFO - Error: /builds/slave/try-and-x86-000000000000000000/build/src/mobile/android/installer/package-manifest.in:82: Missing file(s): bin/distribution/*

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Joshua Cranmer [:jcranmer] from comment #13)
> (In reply to Nick Alexander :nalexander from comment #12)
> > Comment on attachment 8637605 [details] [diff] [review]
> > Fix up the manifest
> > 
> > Sadly, the distribution/* line is necessary: Bug 1163082.  There may be
> > others.
> 
> The build cares to disagree if I try that:
>  20:28:00 INFO - Error:
> /builds/slave/try-and-x86-000000000000000000/build/src/mobile/android/
> installer/package-manifest.in:82: Missing file(s): bin/distribution/*

Correct.  It's an optional include of an Android distribution, which customizes things for partners.  Right now we have poor support for building with a packaged distribution.  The ticket I linked improves that support.  Removing that line removes what support we have.

This is why improving package-manifest.in is hard -- I don't care to audit every possible line.

Comment 15

[Joshua Cranmer [:jcranmer]]

(In reply to Nick Alexander :nalexander from comment #14)
> (In reply to Joshua Cranmer [:jcranmer] from comment #13)
> > (In reply to Nick Alexander :nalexander from comment #12)
> > > Comment on attachment 8637605 [details] [diff] [review]
> > > Fix up the manifest
> > > 
> > > Sadly, the distribution/* line is necessary: Bug 1163082.  There may be
> > > others.
> > 
> > The build cares to disagree if I try that:
> >  20:28:00 INFO - Error:
> > /builds/slave/try-and-x86-000000000000000000/build/src/mobile/android/
> > installer/package-manifest.in:82: Missing file(s): bin/distribution/*
> 
> Correct.  It's an optional include of an Android distribution, which
> customizes things for partners.  Right now we have poor support for building
> with a packaged distribution.  The ticket I linked improves that support. 
> Removing that line removes what support we have.
> 
> This is why improving package-manifest.in is hard -- I don't care to audit
> every possible line.

I used a script to verify that no files that are currently packaged are omitted from the list; however I can only test these on builds available on try. If you want to include distribution files, then you need to provide an ifdef that it can be provided off of.

Comment 16

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 901059 - Clean up old unused mobile/android package-manifest.in references.

I'm taking an old ticket number just to close it.  The files removed
no longer exist in the tree; the NSS option exists and probably
shouldn't -- but that's for another day, so let's just make it not
warn for now.

Depends on D15016

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2717febc588c
Clean up old unused mobile/android package-manifest.in references. r=agi,froydnj

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2717febc588c