1440789 - built_in_addons.json not created during mobile/android packaging

Status: RESOLVED DUPLICATE of bug 1457321

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Matthew Finkel [:sysrqb]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

Bundle a system add-on in a Fennec build. I created it under toolkit, but the location should not factor into this.


Actual results:

As a system add-on, it is loaded at application startup. During the startup procedure, an exception is thrown because the built_in_addons.json manifest does not exist:

I/Gecko   (29113): 1519227169354        addons.xpi      WARN    List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 6376"  data: no] Stack trace: _readAddons()@resource://gre/modules/addons/XPIProvider.jsm:6376 < getAddonLocations()@resource://gre/modules/addons/XPIProvider.jsm:6037 < getInstallState()@resource://gre/modules/addons/XPIProvider.jsm:1582 < checkForChanges()@resource://gre/modules/addons/XPIProvider.jsm:3196 < startup()@resource://gre/modules/addons/XPIProvider.jsm:2176 < callProvider()@resource://gre/modules/AddonManager.jsm:253 < _startProvider()@resource://gre/modules/AddonManager.jsm:728 < startup()@resource://gre/modules/AddonManager.jsm:892 < startup()@resource://gre/modules/AddonManager.jsm:2967 < observe()@jar:jar:file:///data/app/org.mozilla


Expected results:

During the packaging process, all system add-ons should be listed in the built_in_addons.json file. It should be available at chrome://browser/content/built_in_addons.json at runtime. On mobile/android, this would be $(DIST)/bin/chrome/chrome/content/built_in_addons.json

It seems like mobile/android was missed when bug 1348981 landed.

Comment 1

[Matthew Finkel [:sysrqb]]

Attachment: Bug 1440789 Create built_in_addons.json during mobile/android packaging

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8953630 [details] [diff] [review]
Bug 1440789 Create built_in_addons.json during mobile/android packaging

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

OK, we'll take these Makefile.in hacks until we do better.  But I'd much rather see investment into declaring extensions in moz.build than cult this garbage into mobile/android, making it twice as hard to remove from the tree when we finally get to it.

Comment 3

[Andrew Swan [:aswan]]

Matthew, is this ready to land?  You can set the checkin-needed keyword if so.

Comment 4

[Matthew Finkel [:sysrqb]]

(In reply to Andrew Swan [:aswan] from comment #3)
> Matthew, is this ready to land?  You can set the checkin-needed keyword if
> so.

Yes, it should be ready, I added the keyword.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf64879466c4
Create built_in_addons.json at package-time on mobile r=nalexander

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for Android packaging failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/be154f4de4e9f53e8cdd5363d5a923794cb72174

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cf64879466c495a47732d9736ef5daa9b35cefc4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165387960&repo=mozilla-inbound

[task 2018-03-02T00:20:43.294Z] 00:20:43     INFO -  echo packing symbols
[task 2018-03-02T00:20:43.295Z] 00:20:43     INFO -  packing symbols
[task 2018-03-02T00:20:43.295Z] 00:20:43     INFO -  ./config/nsinstall -D dist/
[task 2018-03-02T00:20:43.295Z] 00:20:43     INFO -  make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-03-02T00:20:43.296Z] 00:20:43     INFO -  make[2]: Entering directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-03-02T00:20:43.297Z] 00:20:43     INFO -  ./config/nsinstall -D dist/
[task 2018-03-02T00:20:43.297Z] 00:20:43     INFO -  make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-03-02T00:20:43.300Z] 00:20:43     INFO -  make[3]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-02T00:20:43.300Z] 00:20:43     INFO -  Makefile:94: *** missing separator.  Stop.
[task 2018-03-02T00:20:43.300Z] 00:20:43     INFO -  make[3]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-02T00:20:43.301Z] 00:20:43     INFO -  /builds/worker/workspace/build/src/mobile/android/build.mk:11: recipe for target 'package' failed
[task 2018-03-02T00:20:43.301Z] 00:20:43     INFO -  make[2]: *** [package] Error 2
[task 2018-03-02T00:20:43.301Z] 00:20:43     INFO -  /builds/worker/workspace/build/src/build/moz-automation.mk:98: recipe for target 'automation/package' failed
[task 2018-03-02T00:20:43.301Z] 00:20:43     INFO -  make[1]: *** [automation/package] Error 2

Please fix the issue and submit an updated patch.

Comment 7

[Matthew Finkel [:sysrqb]]

Ugh. Sigh, sorry, I confirmed this is because I copied and pasted the patch. The last line must be tab-indented. But now I found another build failure when dist/bin/features does not exist (which is the normal case on mobile right now).

Now the patch is slightly more hacky than it was previously. I think it makes sense that the "create built_in_addons.json" logic is as similar as possible between browser/ and mobile/android/, but single line python scripts do not make that easy. This works well-enough for now, I think.

The last few lines output from |mach package| are:
 0:20.53 Verification succesful
 0:20.53 make[3]: Leaving directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi/mobile/android/installer'
 0:20.54 make[2]: Leaving directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi/mobile/android/installer'
 0:20.54 make[2]: Entering directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi/mobile/android/installer'
 0:20.54 Traceback (most recent call last):
 0:20.54   File "<string>", line 1, in <module>
 0:20.54 OSError: [Errno 2] No such file or directory: '../../../dist/bin/features'
 0:20.54 make[2]: Leaving directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi/mobile/android/installer'
 0:20.54 make[1]: Leaving directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi/mobile/android/installer'
 0:20.54 make: Leaving directory '/home/sysrqb/mozilla-central/obj-arm-linux-androideabi'
 0:20.56 Notification center failed: Install notify-send (usually part of the libnotify package) to get a notification when the build finishes.

Would it be better if python's stderr is redirected to /dev/null instead of printing the OSError message and traceback (which developers are more likely to see than not while there aren't any builtin features)?

Comment 8

[Matthew Finkel [:sysrqb]]

Attachment: bug1440789.patch

Comment 9

[Matthew Finkel [:sysrqb]]

Attachment: bug1440789.patch

I looked at this again this morning and realized the previous patch doesn't do the right thing either..it only redirected |true|.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8955520 [details] [diff] [review]
bug1440789.patch

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

Sure.  Can you push a try build to show that this is green before landing?  Thanks!

::: mobile/android/installer/Makefile.in
@@ +84,5 @@
> +.PHONY: features
> +tools features::
> +	# Create a sorted list of built-in features. If the features
> +	# directory does not exist, then the recipe line returns true
> +	# because this is not a fatal error.

Please add the line "This should determine  the target directory using FINAL_TARGET and DIST_SUBDIR, but it doesn't yet for simplicity."  I want grep to hit those special values when we come to simplify all this cruft.

Bonus points for using FINAL_TARGET and DIST_SUBDIR correctly in the actual invocation, which might be non-trivial since I don't know if they're set (or used!) in the installer Makefiles.  Sigh.

Comment 11

[Matthew Finkel [:sysrqb]]

(In reply to Nick Alexander :nalexander from comment #10)
> Comment on attachment 8955520 [details] [diff] [review]
> bug1440789.patch
> 
> Review of attachment 8955520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure.  Can you push a try build to show that this is green before landing?

Thanks! I had green try builds last week, but I re-ran them overnight and they're still good.

try -b do -p all -u none -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=baa2d6bd429afce58ccf7580196276d5f2b78181

try -b do -p android-api-16 -u all -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f44a4383cd05098f72b3b4a627a4daa5b859182

> ::: mobile/android/installer/Makefile.in
> @@ +84,5 @@
> > +.PHONY: features
> > +tools features::
> > +	# Create a sorted list of built-in features. If the features
> > +	# directory does not exist, then the recipe line returns true
> > +	# because this is not a fatal error.
> 
> Please add the line "This should determine  the target directory using
> FINAL_TARGET and DIST_SUBDIR, but it doesn't yet for simplicity."  I want
> grep to hit those special values when we come to simplify all this cruft.

Done, I updated the patch only modifying the comment. Does this need another r+ for a minor change?

Comment 12

[Matthew Finkel [:sysrqb]]

Attachment: bug1440789_2.patch

Comment 13

[Nick Alexander :nalexander [he/him]]

> Done, I updated the patch only modifying the comment. Does this need another
> r+ for a minor change?

Nope!

Comment 14

[Matthew Finkel [:sysrqb]]

Thanks! Try this again :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d345b546edce
Create built_in_addons.json at package-time on mobile r=nalexander

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d345b546edce

Comment 17

[Cosmin Sabou [:CosminS]]

Backed out changeset for Android nightly build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=5bf126434fac78a31256c994b9dbf4b1031b0350&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=nightly%20android&selectedJob=170759817

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170759824&repo=mozilla-central&lineNumber=27754

Backout link: https://hg.mozilla.org/mozilla-central/rev/a456475502b80a1264642d9eaee9394a8fad8315

Please ignore the geckoview noise. This should be the relevant failure:

[task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  OSError: [Errno 2] No such file or directory: '../../../dist/bin/features


[task 2018-03-28T10:29:54.188Z] 10:29:54     INFO -  Verification succesful
[task 2018-03-28T10:29:54.188Z] 10:29:54     INFO -  make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:54.188Z] 10:29:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:54.188Z] 10:29:54     INFO -  touch make-package
[task 2018-03-28T10:29:54.188Z] 10:29:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  # Create a sorted list of built-in features. If the features
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  # directory does not exist, then the recipe line returns true
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  # because this is not a fatal error.
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  #
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  # This should determine the target directory using FINAL_TARGET and DIST_SUBDIR, but it doesn't yet for simplicity.
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenv/bin/python -c 'import os, json; listing = {"system": sorted(os.listdir("../../../dist/bin/features"))}; print json.dumps(listing)' > \
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  	../../../dist/bin/chrome/chrome/content/built_in_addons.json || true
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  Traceback (most recent call last):
[task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -    File "<string>", line 1, in <module>
[task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  OSError: [Errno 2] No such file or directory: '../../../dist/bin/features'
[task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  make[3]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
[task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  mkdir -p `dirname '../../../dist//target.checksums'`
[task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  CHECKSUM FILE START

Comment 18

[Matthew Finkel [:sysrqb]]

(In reply to Cosmin Sabou [:cosmin_s] from comment #17)
> Backed out changeset for Android nightly build bustages.
[snip]
> [task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  OSError: [Errno 2] No
> such file or directory: '../../../dist/bin/features
> 
> 
[snip]
> [task 2018-03-28T10:29:54.189Z] 10:29:54     INFO - 
> /builds/worker/workspace/build/src/obj-firefox/_virtualenv/bin/python -c
> 'import os, json; listing = {"system":
> sorted(os.listdir("../../../dist/bin/features"))}; print
> json.dumps(listing)' > \
> [task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  
> ../../../dist/bin/chrome/chrome/content/built_in_addons.json || true
> [task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -  Traceback (most recent
> call last):
> [task 2018-03-28T10:29:54.189Z] 10:29:54     INFO -    File "<string>", line
> 1, in <module>
> [task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  OSError: [Errno 2] No
> such file or directory: '../../../dist/bin/features'
> [task 2018-03-28T10:29:54.190Z] 10:29:54     INFO -  make[4]: Leaving
> directory
> '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
> [task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  make[3]: Entering
> directory
> '/builds/worker/workspace/build/src/obj-firefox/mobile/android/installer'
> [task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  mkdir -p `dirname
> '../../../dist//target.checksums'`
> [task 2018-03-28T10:29:59.488Z] 10:29:59     INFO -  CHECKSUM FILE START

Right. I was worried about this. Despite what it says, this is a non-fatal error. Unless a feature add-on is shipped in-tree, the OSError will be triggered. So, this should not be the cause of the bustage.

As I mentioned in comment 7:
> Would it be better if python's stderr is redirected to /dev/null instead of printing the OSError message and traceback (which developers are more likely to see than not while there aren't any builtin features)?

Should we handle this differently?

Comment 19

[Cosmin Sabou [:CosminS]]

Maybe Nathan can help with this one. I have no knowledge here.

Comment 20

[Nathan Froyd [:froydnj]]

Redirecting to nalexander as the reviewer here.  I suspect that something is picking up on the "OSError" string in the output and using that information, rather than the command exit status, to determine whether things are broken.  (Which seems a little bonkers, really, but...)  If that's the case, comment 7 sounds like the right approach, but nalexander could probably say for sure.

Comment 21

[Caitlin Neiman [:caitmuenster]]

(In reply to Matthew Finkel (sysrqb) from comment #14)
> Thanks! Try this again :)

Hey Matthew! Thanks so much for submitting this patch. While this is still under refinement, we've gone ahead and added it to our contributions wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#March_2018

If you would like to create an account on mozillians.org, I'd be happy to vouch for you!

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #20)
> Redirecting to nalexander as the reviewer here.  I suspect that something is
> picking up on the "OSError" string in the output and using that information,
> rather than the command exit status, to determine whether things are broken.
> (Which seems a little bonkers, really, but...)  If that's the case, comment
> 7 sounds like the right approach, but nalexander could probably say for sure.

Hmm.  I think mozharness or the log parsing is just stupid in this situation, which is annoying.

However, this points to a need to handle this in a more principled manner.  Let's extract a py_action (see https://searchfox.org/mozilla-central/search?q=py_action&path=) for producing the file, have the Python script walk a directory provided as an argument, and have the script take an --allow-missing-features or similar flag to control whether this should fail or not.  There are lots of examples of actions in python/mozbuild/mozbuild/action.

Let's use this approach for Desktop and for Android.

Sorry that this is taking more time than expected, Matthew.  Do we understand why we only see the error for Nightly builds?  It seems like we should see this bad path for regular developer builds too...

Comment 23

[Matthew Finkel [:sysrqb]]

(In reply to Nick Alexander :nalexander from comment #22)
> (In reply to Nathan Froyd [:froydnj] from comment #20)
> > Redirecting to nalexander as the reviewer here.  I suspect that something is
> > picking up on the "OSError" string in the output and using that information,
> > rather than the command exit status, to determine whether things are broken.
> > (Which seems a little bonkers, really, but...)  If that's the case, comment
> > 7 sounds like the right approach, but nalexander could probably say for sure.
> 
> Hmm.  I think mozharness or the log parsing is just stupid in this
> situation, which is annoying.

Yes...

> 
> However, this points to a need to handle this in a more principled manner. 
> Let's extract a py_action (see
> https://searchfox.org/mozilla-central/search?q=py_action&path=) for
> producing the file, have the Python script walk a directory provided as an
> argument, and have the script take an --allow-missing-features or similar
> flag to control whether this should fail or not.  There are lots of examples
> of actions in python/mozbuild/mozbuild/action.
> 
> Let's use this approach for Desktop and for Android.

Okay, I'll write another patch for this. Thanks for the pointer.

> 
> Sorry that this is taking more time than expected, Matthew.  Do we
> understand why we only see the error for Nightly builds?  It seems like we
> should see this bad path for regular developer builds too...

Okay, I see now.

Try build:
https://treeherder.mozilla.org/logviewer.html#?job_id=169602137&repo=try&lineNumber=28654

Nightly bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=170759824&repo=mozilla-central&lineNumber=28586


The multi_l10n build sets |halt_on_failure|
https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildbase.py#1351

Apparently this multi-l10n code path is only run in nightly. The normal build process does not set |halt_on_failure|:
https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildbase.py#1287

And |halt_on_failure| is false by default:
https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#1313

Comment 24

[Matthew Finkel [:sysrqb]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #21)
> (In reply to Matthew Finkel (sysrqb) from comment #14)
> > Thanks! Try this again :)
> 
> Hey Matthew! Thanks so much for submitting this patch. While this is still
> under refinement, we've gone ahead and added it to our contributions wiki:
> https://wiki.mozilla.org/Add-ons/Contribute/Recognition#March_2018

Hi Caitlin, thank you! One minor nitpick, it looks like my last name has the 'e' and 'l' reversed.

> 
> If you would like to create an account on mozillians.org, I'd be happy to
> vouch for you!

Thanks! I'll check it out.

Comment 25

[Matthew Finkel [:sysrqb]]

(In reply to Matthew Finkel (:sysrqb) from comment #23)
> (In reply to Nick Alexander :nalexander from comment #22)
> > 
> > However, this points to a need to handle this in a more principled manner. 
> > Let's extract a py_action (see
> > https://searchfox.org/mozilla-central/search?q=py_action&path=) for
> > producing the file, have the Python script walk a directory provided as an
> > argument, and have the script take an --allow-missing-features or similar
> > flag to control whether this should fail or not.  There are lots of examples
> > of actions in python/mozbuild/mozbuild/action.
> > 
> > Let's use this approach for Desktop and for Android.
> 
> Okay, I'll write another patch for this. Thanks for the pointer.

I was about to pick this up again, but then I looked at bug1453691. It looks like the necessary change actually landed as part of bug1457321:

https://hg.mozilla.org/mozilla-central/rev/fe5d46cd794b

I'll close this as a duplicate.