Closed Bug 1440789 Opened 2 years ago Closed 2 years ago

built_in_addons.json not created during mobile/android packaging

Categories

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

60 Branch
defect

Tracking

()

RESOLVED DUPLICATE of bug 1457321

People

(Reporter: sysrqb, Assigned: sysrqb)

References

Details

Attachments

(1 file, 3 obsolete files)

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 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.
Attachment #8953630 - Flags: review+
Matthew, is this ready to land?  You can set the checkin-needed keyword if so.
Assignee: nobody → matthew.finkel
Flags: needinfo?(matthew.finkel)
Priority: -- → P2
Keywords: checkin-needed
(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.
Flags: needinfo?(matthew.finkel)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf64879466c4
Create built_in_addons.json at package-time on mobile r=nalexander
Keywords: checkin-needed
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.
Flags: needinfo?(matthew.finkel)
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)?
Flags: needinfo?(matthew.finkel)
Attached patch bug1440789.patch (obsolete) — Splinter Review
Attachment #8953630 - Attachment is obsolete: true
Attachment #8955408 - Flags: review?(nalexander)
Attached patch bug1440789.patch (obsolete) — Splinter Review
I looked at this again this morning and realized the previous patch doesn't do the right thing either..it only redirected |true|.
Attachment #8955408 - Attachment is obsolete: true
Attachment #8955408 - Flags: review?(nalexander)
Attachment #8955520 - Flags: review?(nalexander)
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.
Attachment #8955520 - Flags: review?(nalexander) → review+
(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?
Attachment #8955520 - Attachment is obsolete: true
Attachment #8961334 - Flags: review+
Attachment #8961334 - Flags: checkin+
> Done, I updated the patch only modifying the comment. Does this need another
> r+ for a minor change?

Nope!
Thanks! Try this again :)
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d345b546edce
Create built_in_addons.json at package-time on mobile r=nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d345b546edce
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(matthew.finkel)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
(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?
Flags: needinfo?(matthew.finkel)
Maybe Nathan can help with this one. I have no knowledge here.
Flags: needinfo?(nfroyd)
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.
Flags: needinfo?(nfroyd) → needinfo?(nalexander)
(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!
(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...
Flags: needinfo?(nalexander)
(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
(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.
Blocks: 1453691
(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.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1457321
You need to log in before you can comment on or make changes to this bug.