Closed Bug 1464689 Opened 2 years ago Closed 4 months ago

Port bug 1457321: fix creation of built_in_addons.json

Categories

(Thunderbird :: Build Config, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 73.0

People

(Reporter: jorgk-bmo, Assigned: rjl)

Details

(Whiteboard: [Thunderbird-testfailure: XZ all][Thunderbird-temporary-fix])

Attachments

(2 files, 9 obsolete files)

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_temporary.js | Test timed out

First seen:
Sun, May 27, 2018, 16:03:52

M-C last good: 4e9446f9e8f0a75c7ffe063f1dfb311cc9
M-C first bad: 6b9076ac236cb0f9f301bc601eac03f9ec
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e9446f9e8f0a75c7ffe063f1dfb311cc9&tochange=6b9076ac236cb0f9f301bc601eac03f9ec

Looks like Kris caused this in one of the various bugs he landed ;-)

test_dictionary_webextension.js was last touched here:
Kris Maglione - Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories. 

And test_temporary.js was last touched in
Kris Maglione - Bug 1462500: Improve our detection of XPI vs. directory installs.

Kris, what do we need to do to get these tests to pass in Thunderbird? Or should they simply be switched off?
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [Thunderbird-testfailure: Z all] → [Thunderbird-testfailure: X all]
I guess this is also responsible for:
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1527430329/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_spellcheck_in_content_tabs
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1527430329/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_context_menu

since those failures are related to spellcheck. Log
https://taskcluster-artifacts.net/CEnUhCINSu-NPildMgGeyw/0/public/logs/live_backing.log
says:

INFO -  SUMMARY-UNEXPECTED-FAIL | test-content-tab.js | test-content-tab.js::test_spellcheck_in_content_tabs
INFO -    EXCEPTION:
INFO -      at: test-folder-display-helpers.js line 2913
INFO -         assert_true test-folder-display-helpers.js:2913 11
INFO -         assert_element_visible test-dom-helpers.js:71 3
INFO -         test_spellcheck_in_content_tabs test-content-tab.js:68 3
INFO -         Runner.prototype.wrapper frame.js:584 9
INFO -         Runner.prototype._runTestModule frame.js:654 9
INFO -         Runner.prototype.runTestModule frame.js:700 3
INFO -         Runner.prototype.runTestDirectory frame.js:524 7
INFO -         runTestDirectory frame.js:706 3
INFO -         Bridge.prototype._execFunction server.js:177 10
INFO -         Bridge.prototype.execFunction server.js:181 16
INFO -         Session.prototype.receive server.js:282 3
INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
INFO -  SUMMARY-UNEXPECTED-FAIL | test-content-tab.js | test-content-tab.js::test_content_tab_context_menu
INFO -    EXCEPTION: a != b: 'undefined' != 'Click me!'.
INFO -      at: test-folder-display-helpers.js line 2913
INFO -         assert_true test-folder-display-helpers.js:2913 11
INFO -         assert_equals test-folder-display-helpers.js:2900 3
INFO -         test_content_tab_context_menu test-content-tab.js:108 3
INFO -         Runner.prototype.wrapper frame.js:584 9
INFO -         Runner.prototype._runTestModule frame.js:654 9
INFO -         Runner.prototype.runTestModule frame.js:700 3
INFO -         Runner.prototype.runTestDirectory frame.js:524 7
INFO -         runTestDirectory frame.js:706 3
INFO -         Bridge.prototype._execFunction server.js:177 10
INFO -         Bridge.prototype.execFunction server.js:181 16
INFO -         Session.prototype.receive server.js:282 3
INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

Aceman, could you please take a look at the Mozmill failures. Also in the log there are heaps of JS errors, perhaps most prominently this one:
test-window-helpers.js, line 1548: TypeError: target.ownerDocument is null
Flags: needinfo?(acelists)
Summary: TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js and TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_temporary.js → TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js and TEST-UNEXPECTED-TIMEOUT | toolkit/mozapps/extensions/test/xpcshell/test_temporary.js and mozmill/content-tabs/test-content-tab.js | test-content-tab.js
Whiteboard: [Thunderbird-testfailure: X all] → [Thunderbird-testfailure: XZ all]
Looking at bug 1457321 for the dictionary changes, there appear to be a lot of "build bits" with probably have to be ported to TB.
Right, it appears that TB no longer has the en-US dictionary.
Maybe Ted can point us in the right direction.
Flags: needinfo?(ted)
I've looked at the changesets from bug 1457321:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5d46cd794bfb3e8974d8213cb3fa752902ef3b
Bug 1457321: Part 1 - Add bundled dictionaries to built_in_addons.json. r=ted,rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/d31a5556d42a6c9314f51fd115a0cb5531433bac
Bug 1457321: Part 2 - Add dictionaries to omnijar. r=ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/8810007550b1fdeeae0f5d797c4a511818a57be7
Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories. r=aswan,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a358272d5e8ac3080e2afb192cfde755dc4e56
Bug 1457321: Part 4 - Update built-in add-ons manifst during l10n repack. r=ted f=pike

I don't know where to look. All the files touched here are M-C specific, there is now port by "pattern matching". This needs a build person to look at. Philipp, do you have time? Perhaps Geoff wants to have a go.
Flags: needinfo?(philipp)
Flags: needinfo?(geoff)
Flags: needinfo?(acelists)
Yes, this was bug 1457321. You need to update your makefiles to generate built_in_addons.json with a list of built-in dictionaries.
Flags: needinfo?(kmaglione+bmo)
(No idea about the test_temporary failures. I'd need to see the whole log.)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> Yes, this was bug 1457321. You need to update your makefiles to generate
> built_in_addons.json with a list of built-in dictionaries.
Sadly I know nothing about makefiles and how that could be done. As a temporary measure, could we just hard-code en-US here:
https://dxr.mozilla.org/comm-central/source/mail/extensions/built_in_addons.json

What's the syntax?

I'll move test_temporary.js to another bug and provide a log from a local run.
Summary: TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js and TEST-UNEXPECTED-TIMEOUT | toolkit/mozapps/extensions/test/xpcshell/test_temporary.js and mozmill/content-tabs/test-content-tab.js | test-content-tab.js → Port bug 1457321: TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js and mozmill/content-tabs/test-content-tab.js | test-content-tab.js
(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #8)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #6)
> > Yes, this was bug 1457321. You need to update your makefiles to generate
> > built_in_addons.json with a list of built-in dictionaries.
> Sadly I know nothing about makefiles and how that could be done. As a
> temporary measure, could we just hard-code en-US here:
> https://dxr.mozilla.org/comm-central/source/mail/extensions/built_in_addons.
> json
> 
> What's the syntax?

I suppose you could. It should basically be something like:

  "dictionaries": {"en-US": "dictionaries/en-US.dic"}

But it should be easy enough to just update your Makefiles to call the generate action:

https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/mobile/android/base/Makefile.in#49-51
Thanks, I've just dug out that file from a FF build and saw the same:
{"dictionaries": {"en-US": "dictionaries/en-US.dic"}

As for the makefile: Yes, I saw the snippet, but I could pattern-match its context to any of our makefiles.
That worked, at least for Xpcshell so far. Of course I'm causing an l10n problem with that hack. Tom, could you backout my hack and get it fixed properly. Or any of the other "build people".
Flags: needinfo?(ted) → needinfo?(mozilla)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: XZ all] → [Thunderbird-testfailure: XZ all][Thunderbird-temporary-fix]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8e4d696c483e
temporarily add en-US dictionary to built_in_addons.json. rs=bustage-fix
(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #12)
> That worked, at least for Xpcshell so far. Of course I'm causing an l10n
> problem with that hack.

You're probably not. While it's technically possible to build with locales other than en-US, we generally don't. Instead, we generally build locale packs, and then repack the en-US build with resources from those.
Flags: needinfo?(geoff)
I broke the commits up in to 3 parts for review in cases changes needed to be made.
Part 1 is Bug 1464689 changed mail/extensions/moz.build|
Part 2 is Bug 1464689 added mail/extensions/gen_built_in_addons.py removed mail/extensions/built_in_addons.json
Part 3 is Bug 1464689 Changed mail/installer/Makefile.in to include repackage
Attachment #8986986 - Flags: review?(jorgk)
I broke the commits up in to 3 parts for review in cases changes needed to be made.
Part 1 is Bug 1464689 changed mail/extensions/moz.build|
Part 2 is Bug 1464689 added mail/extensions/gen_built_in_addons.py removed mail/extensions/built_in_addons.json
Part 3 is Bug 1464689 Changed mail/installer/Makefile.in to include repackage
Attachment #8986987 - Flags: review?(jorgk)
Attachment #8986988 - Flags: review?(jorgk)
Thanks David. I've folded the patches and added a proper commit message.

You removed built_in_addons.json completely. I thought we'd just have to remove the hard-coded dictionary from it again, so revert:
https://hg.mozilla.org/comm-central/rev/8e4d696c483e
But perhaps the script also adds the system entry.

Sadly, I'm not a build person, so I'll have to find a reviewer for this.
Attachment #8986986 - Attachment is obsolete: true
Attachment #8986987 - Attachment is obsolete: true
Attachment #8986988 - Attachment is obsolete: true
Attachment #8986986 - Flags: review?(jorgk)
Attachment #8986987 - Flags: review?(jorgk)
Attachment #8986988 - Flags: review?(jorgk)
Flags: needinfo?(philipp)
Flags: needinfo?(mozilla)
Attachment #8986989 - Flags: review?(mozilla)
Comment on attachment 8986989 [details] [diff] [review]
Generate_built_in_addons_json.patch (folded)

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

It looks like this is copied from the patch in Bug 1459004. While we should definitely track that work and implement the corresponding changes in comm-central when it lands, until it does, we should use the existing implementation.

::: mail/extensions/gen_built_in_addons.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

There is no reason to duplicate this file. We can use the copy in mozilla-central.

::: mail/extensions/moz.build
@@ +15,5 @@
> +
> +manifest = GENERATED_FILES[built_in_addons]
> +manifest.script = 'gen_built_in_addons.py'
> +
> +if CONFIG['MOZ_BUILD_APP'] == 'browser':

This logic is firefox specific and needs to be adjusted.

::: mail/installer/Makefile.in
@@ +68,5 @@
>  
>  DEFINES += -DJAREXT=
>  
> +.PHONY: repackage
> +tools repackage:: $(DIST)/bin/$(MOZ_APP_NAME)

Could you comment on why this line is needed?
Attachment #8986989 - Flags: review?(mozilla) → review-
Bug 1459004 just landed, so the solution we need here will possibly have to change:
https://hg.mozilla.org/mozilla-central/rev/d6120c2bb51e
Updated patch.

An attempt to rework the patch per Tom's comments.
Attachment #8991752 - Flags: review+
Comment on attachment 8991752 [details] [diff] [review]
Generate_built_in_addons_json.patch

Tom, could you please take a look.
Attachment #8991752 - Flags: review+ → review?(mozilla)
Comment on attachment 8991752 [details] [diff] [review]
Generate_built_in_addons_json.patch

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

I assume you've tested this code, and it generates the appropriate file in the correct place, and that it gets read appropriately. Given that, this looks like a reasonable fix.

That being said, I've generally tried to keep thunderbird's build system structured as similar to mozilla-central's code as possible. In this particular case, looking at https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/toolkit/mozapps/extensions/moz.build#28-31 mozilla-central code is already generating the file (https://treeherder.mozilla.org/logviewer.html#?job_id=190798137&repo=comm-central&lineNumber=42947). It just isn't getting installed because there isn't an appropriate stanza in https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/toolkit/mozapps/extensions/moz.build#33-44 . I think the better fix is to reach out to the authors/reviewers of that code, and either move the branches of those conditionals into the appropriate files in `browser`/`mobile/android`/`comm/mail` or to add a `comm/mail` branch to that conditional.

::: mail/extensions/built_in_addons.json
@@ +1,1 @@
> +{"dictionaries": {"en-US": "dictionaries/en-US.dic"}, "system": []}
\ No newline at end of file

Since this file is now generated, it shouldn't be kept in the source.
Attachment #8991752 - Flags: review?(mozilla) → review-
Component: General → Add-Ons: Extensions API
Component: Add-Ons: Extensions API → Build Config
Summary: Port bug 1457321: TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js and mozmill/content-tabs/test-content-tab.js | test-content-tab.js → Port bug 1457321: fix creation of built_in_addons.json
Assignee: nobody → rob

Time to fix this one day? New action in this area, see bug 1496995.

Looking at it. In addition to getting the generated file to copy to the right place, I suspect that gen_built_in_addons.py will require changes now as well since the built_in_addons.json file it generates is missing the wetransfer extension information.

Attachment #8986989 - Attachment is obsolete: true
Attachment #8991752 - Attachment is obsolete: true
Attachment #8981020 - Attachment is obsolete: true

Thunderbird has a couple of system extensions that are able to make use of
this now, specifically the wetransfer extension.

Attached patch gen_built_in_addons.patch (obsolete) — Splinter Review
This patch requires the M-C change in D39990. Bustage will occur in C-C if
landings are not coordinated.

I ran this locally several times to make sure that the en-US dictionary
and the wetransfer extension that currently make use of this are still found.

Try job running at:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=642daa445422dbc53cc417ed3158016cf9a30c42
Attachment #9081846 - Flags: review?(geoff)
Comment on attachment 9081846 [details] [diff] [review]
gen_built_in_addons.patch

Let's move the remains of that manifest to `mail/base/jar.mn`.
Attachment #9081846 - Flags: review?(geoff) → feedback+
Attached patch gen_built_in_addons.patch (obsolete) — Splinter Review
Updated per comments. Try job is in progress.
Attachment #9108001 - Flags: review?(geoff)
Attachment #9081846 - Attachment is obsolete: true
Comment on attachment 9108001 [details] [diff] [review]
gen_built_in_addons.patch

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

Yeah, that's better, as long as it works. Which I think it should.
Attachment #9108001 - Flags: review?(geoff) → review+
Attached patch gen_built_in_addons.patch (obsolete) — Splinter Review
This iteration actually works. We do need the browser.jar bit to be present
because that's the only place that built_in_addons.json can go it seems.

Local build completed and packaged.
Attachment #9111004 - Flags: review?(geoff)
Attachment #9108001 - Attachment is obsolete: true
Verified Wetransfer does not break with this. Try build at:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e9d96cfe044f4c6dea7850da7bf445c891711e70
Attachment #9111012 - Flags: review?(geoff)
Attachment #9111004 - Attachment is obsolete: true
Attachment #9111004 - Flags: review?(geoff)
Attachment #9111012 - Flags: review?(geoff) → review+
Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/integration/autoland/rev/2b70174177b4
Use gen_built_in_addons.py for Thunderbird builds. r=kmag

Attachment 9111012 [details] [diff] needs landing on comm-central once the piece in comment 36 makes its way onto mozilla-central.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/c272f21f60ce
Use gen_built_in_addons.py for Thunderbird builds. r=darktrojan

Status: NEW → RESOLVED
Closed: 4 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0
You need to log in before you can comment on or make changes to this bug.