1464689 - Port bug 1457321: fix creation of built_in_addons.json

Status: RESOLVED FIXED

(Thunderbird :: Build Config, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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?

Comment 1

[Jorg K (CEST = GMT+2)]

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

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

Right, it appears that TB no longer has the en-US dictionary.

Comment 4

[Jorg K (CEST = GMT+2)]

Maybe Ted can point us in the right direction.

Comment 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Kris Maglione [:kmag]]

Yes, this was bug 1457321. You need to update your makefiles to generate built_in_addons.json with a list of built-in dictionaries.

Comment 7

[Kris Maglione [:kmag]]

(No idea about the test_temporary failures. I'd need to see the whole log.)

Comment 8

[Jorg K (CEST = GMT+2)]

(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.

Comment 9

[Kris Maglione [:kmag]]

(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

Comment 10

[Jorg K (CEST = GMT+2)]

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.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1464689-add-en-US.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=617326020e08392c0593615c52e232826a5a48e1

Comment 12

[Jorg K (CEST = GMT+2)]

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".

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8e4d696c483e
temporarily add en-US dictionary to built_in_addons.json. rs=bustage-fix

Comment 14

[Kris Maglione [:kmag]]

(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.

Comment 15

[Jorg K (CEST = GMT+2)]

With the band-aid from comment #13 in place, things have been working until when the fail again:

EST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1527978974/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_1527978974/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_context_menu

Log
https://taskcluster-artifacts.net/LnQ5wxyjRWuC3JheghgS7A/0/public/logs/live_backing.log
says the same as already quoted in comment #1.

M-C last good: 8c926373039374cd1a47d92215e9efb4d5
M-C first bad: 1f62ecdf59b6ecaa3c0fdda39bb296ec09
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c926373039374cd1a47d92215e9efb4d5&tochange=1f62ecdf59b6ecaa3c0fdda39bb296ec09

I can't see anything in that range. Kris, are you aware of any changes?

Comment 16

[Jorg K (CEST = GMT+2)]

Still OK at M-C rev 3b9d55ec3055 (3b9d55ec305565977c6d3adfe7f4eef0dd)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0d715e3cb1ea61e18ee6944eda51b76e1fb4e6e0

Hence:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3b9d55ec305565977c6d3adfe7f4eef0dd&tochange=1f62ecdf59b6ecaa3c0fdda39bb296ec09

Sadly that link doesn't work, like I've noticed a few times before.

Comment 17

[Jorg K (CEST = GMT+2)]

Still OK at M-C rev b7688ef14e3a (b7688ef14e3ae06221387df0820affe16f)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56906045a0bae1908a7d986387bd6c4d63d00f69

So the regression range is now very small, including bug Bug 1465875 :-(

I'll file a new bug and fix that. I'll obsolete the comments here.

Comment 18

[davidj]

Attachment: Generate_built_in_addons_json.patch

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

Comment 19

[davidj]

Attachment: Generate_built_in_addons_json2.patch

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

Comment 20

[davidj]

Attachment: Generate_built_in_addons_json3.patch

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Generate_built_in_addons_json.patch (folded)

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.

Comment 22

[Tom Prince [:tomprince]]

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?

Comment 23

[Jorg K (CEST = GMT+2)]

Bug 1459004 just landed, so the solution we need here will possibly have to change:
https://hg.mozilla.org/mozilla-central/rev/d6120c2bb51e

Comment 24

[Rob Lemley [:rjl]]

Attachment: Generate_built_in_addons_json.patch

Updated patch.

An attempt to rework the patch per Tom's comments.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8991752 [details] [diff] [review]
Generate_built_in_addons_json.patch

Tom, could you please take a look.

Comment 26

[Tom Prince [:tomprince]]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

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

Comment 28

[Rob Lemley [:rjl]]

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.

Comment 29

[Rob Lemley [:rjl]]

Attachment: Bug 1464689 - Use gen_built_in_addons.py for Thunderbird builds. r?kmag

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

Comment 30

[Rob Lemley [:rjl]]

Attachment: gen_built_in_addons.patch

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

Comment 31

[Geoff Lankow (:darktrojan)]

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`.

Comment 32

[Rob Lemley [:rjl]]

Attachment: gen_built_in_addons.patch

Updated per comments. Try job is in progress.

Comment 33

[Geoff Lankow (:darktrojan)]

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.

Comment 34

[Rob Lemley [:rjl]]

Attachment: gen_built_in_addons.patch

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.

Comment 35

[Rob Lemley [:rjl]]

Attachment: gen_built_in_addons.patch

Verified Wetransfer does not break with this. Try build at:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e9d96cfe044f4c6dea7850da7bf445c891711e70

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b70174177b4
Use gen_built_in_addons.py for Thunderbird builds. r=kmag

Comment 37

[Rob Lemley [:rjl]]

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

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2b70174177b4

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c272f21f60ce
Use gen_built_in_addons.py for Thunderbird builds. r=darktrojan