Closed Bug 1390461 Opened 7 years ago Closed 7 years ago

Update langpack-webext-% for bug 1370506

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 affected)

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox57 --- affected

People

(Reporter: Pike, Assigned: zbraniecki)

References

Details

Attachments

(2 files)

Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACKS mk flag.
59 bytes, text/x-review-board-request
Pike
: review+
Details
makefile snippet
261 bytes, text/plain
Details 
The landing of langpack-webext-% lacks a few things I added in bug 1370506.

Notably, https://hg.mozilla.org/mozilla-central/diff/8e37fd6588f2/toolkit/locales/l10n.mk#l1.44.

It's also unclear to me how we should integrate the webext work with bug 1385227, which realigns all those installers stuff and gets rid of bad dependencies. Which bug 1365440 copied, of course.

I'm tempted to make a new variable that's not defined that you can set in the enviroment if you want webext langpacks, and remove the new target -webext- alltogether. Given they're both writing to the same file, there's no point having both as separate entry points, right?
I verified that `make -C objdir/browser/locale langpack-webext-fr' builds a proper webext type language pack on windows, mac and linux using today's m-c.

Happy to make any adjustments you see necessary :)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
This patch updates the new webextension based langpack build code in two ways:

1) Insted of keeping two make commands, we now have one with an mk WEBEXT_LANGPACK flag
to switch between which type of language pack is being built.

2) Updates the manifest scheme to listing resources per language (bug 1365709 comment 33)

that basically means that instead of:

{
  "languages": ["es-MX", "es"],
  "chrome_resources": [
    "locale global pl path/",
  ]
}

we will now have:


{
  "languages": {
    "es-MX": {
      "version": "201709121000",
      "resources": null,
      "chrome_resources": [
        "locale global pl path/",
      }
    },
    "es": {
      "version": "201709111521",
      "resources": null,
      "chrome_resources": [
        "locale global pl path/",
      }
    }
  },
}

This is not super meaningful at the moment since the langpacks are single-locale, but will come handy when we'll want to add fallback locales into the langpack.

The "resources" entry will allow us to test the FileSource vs IndexedFileSource in L10nRegistry. For now it'll stay null.
Comment on attachment 8900026 [details]
Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACKS mk flag.

https://reviewboard.mozilla.org/r/171354/#review176814

The manifest changes should really go into the other bug, and not this one. r- for that.

The actual langpack changes look OK, but I'd like to get at least the most essential test coverage added for them. Can you do a

  $(MAKE) installers-x-test L10NBASEDIR='$(PWD)' WEBEXT_LANGPACKS=1

in l10n-check in browser/Makefile.in ?

You might need to wrap the installer test line into () to make the cd temporary.

Not sure if there's something trivial to test in the language pack. Maybe that there's no chrome.manifest but a manifest.json?
Attachment #8900026 - Flags: review?(l10n) → review-
Attachment #8900026 - Attachment description: Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACK mk flag and update it to the latest manifest consensus. → Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACK mk flag.
(In reply to Axel Hecht [:Pike] from comment #6)
> Comment on attachment 8900026 [details]
> Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACK mk flag.
> 
> https://reviewboard.mozilla.org/r/171354/#review176814
> 
> The manifest changes should really go into the other bug, and not this one.
> r- for that.

Spun off as bug 1393147.


> The actual langpack changes look OK, but I'd like to get at least the most
> essential test coverage added for them. Can you do a
> 
>   $(MAKE) installers-x-test L10NBASEDIR='$(PWD)' WEBEXT_LANGPACKS=1
> 
> in l10n-check in browser/Makefile.in ?

Why installers-x-test?

> You might need to wrap the installer test line into () to make the cd
> temporary.
> 
> Not sure if there's something trivial to test in the language pack. Maybe
> that there's no chrome.manifest but a manifest.json?

I'm not sure where should I check for that. Should I look into .xpi, unpack it, and see if it has manifest.json and doesn't have chrome.manifest?

How do I get the path to the .xpi in browser/locales/Makefile.in? Where should I unpack?
Flags: needinfo?(l10n)
I meant langpack-x-test.

I don't think you actually need to unpack, you can just check the return value of unzip -l ....xpi chrome.manifest to be an error, and the one of manifest.json to be success (i.e. 0).
Flags: needinfo?(l10n)
Thanks!

Managed to get the test working. :)
Pike, I ended up having to pass the LANGPACK_FILE path from browser/locales/Makefile.in to the langpack-% target. Otherwise on try it was building with a weird file name ('target.langpack.xpi' or sth like that).

It does work locally without it, but try seems to do something differently :)

Now, there seems to be only Windows build broken because of windows repack installer. I don't understand why is there a windows installer repack when I'm building langpack. Need help.
And it's green now! :)

No windows repack anymore  :)
Comment on attachment 8900026 [details]
Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACKS mk flag.

https://reviewboard.mozilla.org/r/171354/#review178584

This doesn't work at all. I did find what triggers it, but then also figured that you're lost.

I've undone most of the test, and did it somehow else, and pushed the follow-up to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=318341d4eb03d1d3b6dbb938ea1b7832a8537b91

I hope that actually builds, and runs the l10n-check tests. Most builds on automation don't. I do think that the Nightly builds run the check, you'll need to dig into the logs to ensure that's true.

Note, I added a clobber post-test, 'cause your stuff only passed locally because you had a pre-existing x-test binary from before you broke the test :-/

::: browser/locales/Makefile.in:197
(Diff revision 9)
>  	    $(STAGEDIST)/application.ini App BuildID
>  
>  # test target, depends on make package
>  # try to repack x-test, with just toolkit/defines.inc being there
>  l10n-check:: INNER_UNMAKE_PACKAGE=true
> +l10n-check:: AB_CD=x-test

This breaks the test, as at this point, the installer it tries to repack is x-test, not en-US.

Probably passes for you because you had that around from previous versions of your tests.
Attachment #8900026 - Flags: review?(l10n) → review-
Pike, seems like there's some error on windows:
 - https://treeherder.mozilla.org/logviewer.html#?job_id=126545809&repo=try&lineNumber=41619
 - https://treeherder.mozilla.org/logviewer.html#?job_id=126545782&repo=try&lineNumber=37748

I tried to replicate it on Win10 and couldn't :(
Any idea what's going on?
Flags: needinfo?(l10n)
My suspicion would be different quoting on windows.

That might hurt you here when you try to pass **/*.manifest from the makefile to zip to not package the chrome.manifest files.

No idea how to fix that, and what triggers your windows to be fine and automation not. Maybe different versions of mozmake?
Flags: needinfo?(l10n)
Ok, reproduced it. Basically, windows doesn't like "! command" syntax. I switched it back to `test` command and it seems to work now.
Looks green! :)
Comment on attachment 8900026 [details]
Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACKS mk flag.

https://reviewboard.mozilla.org/r/171354/#review179454

I'd prefer to see the check be simpler.

::: browser/locales/Makefile.in:218
(Diff revision 11)
> +# We need to split this out from l10n-check, as that needs AB_CD to be en-US
> +# at the top level to unpack the right file
> +analyze-langpack-%: AB_CD=$*
> +analyze-langpack-%:
> +	test "`$(UNZIP) -qql $(ABS_DIST)/$(LANGPACK) 'manifest.json' | wc -l`" == "1"
> +	test "`$(UNZIP) -qql $(ABS_DIST)/$(LANGPACK) 'chrome.manifest' | wc -l`" == "0"

This should work without wc.

Also, I'd only test the negative assertion, as that's only where we need it.
Attachment #8900026 - Flags: review?(l10n) → review-
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> Looks green! :)

Can you help out and list which builds on try actually ran the l10n-check test? Then we don't all have to scrape the logs for which builds did and which builds just didn't.
Pike, I switched to "test -n" and "test -z" as per manual. Let me know if that works for you.

> Can you help out and list which builds on try actually ran the l10n-check test? Then we don't all have to scrape the logs for which builds did and which builds just didn't.

Here's a try build with a broken l10n-check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=815503b83ef1ecdfb7eb8590e2b177e14c568f13

Linux opt (B)
Linux pgo (B)
Linux x64 opt (B) (Bb) (V)
Linux x64 pgo (B)
Windows 2012 pgo (B)
Windows 2012 x64 opt (B)
Windows 2012 x64 pgo (B)

I do not know how to do this without manual scrapping so I did manually scrap for "analyze-langpack-" in the latest try build to verify that the list above ran those tests.
Attached file makefile snippet — 
I got around the (IMHO) weird errors, by using $(shell) and $(if)

HTH.
Yup, thanks! Applied.
Comment on attachment 8900026 [details]
Bug 1390461 - Update langpack-webext to use WEBEXT_LANGPACKS mk flag.

https://reviewboard.mozilla.org/r/171354/#review179690
Attachment #8900026 - Flags: review?(l10n) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27f6228d4118
Update langpack-webext to use WEBEXT_LANGPACKS mk flag. r=Pike
Backed out for frequently failing own check-clobber-l10n-x-test during build:

https://hg.mozilla.org/integration/autoland/rev/f8f16fe399e4b7ec1ee21115f209a142414aa950

A push with the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1e8c41b9bcc89680b24c6119a89f9f58ea783e43&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=runnable&filter-resultStatus=busted&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=127248456&repo=autoland

05:28:11     INFO -  z:/build/build/src/mozmake.EXE check-clobber-l10n-x-test
05:28:11     INFO -  z:/build/build/src/mozmake.EXE clobber-x-test
05:28:11     INFO -  mozmake.EXE[7]: Entering directory 'z:/build/build/src/obj-firefox/browser/locales'
05:28:11     INFO -  rm -f -rf ../../dist/xpi-stage/locale-x-test
05:28:11     INFO -  mozmake.EXE[7]: Leaving directory 'z:/build/build/src/obj-firefox/browser/locales'
05:28:18     INFO -  mozmake.EXE[6]: Entering directory 'z:/build/build/src/obj-firefox/browser/locales'
05:28:18     INFO -  find z:/build/build/src/obj-firefox/dist -name \*x-test\* -type f -delete
05:28:18     INFO -  find: File system loop detected; `z:/build/build/src/obj-firefox/dist/test-stage/jetpack/test/addons/packaging' is part of the same file system loop as `z:/build/build/src/obj-firefox/dist'.
05:28:18     INFO -  Makefile:230: recipe for target 'check-clobber-l10n-x-test' failed
05:28:18     INFO -  mozmake.EXE[6]: *** [check-clobber-l10n-x-test] Error 1
05:28:18     INFO -  mozmake.EXE[6]: Leaving directory 'z:/build/build/src/obj-firefox/browser/locales'
05:28:18     INFO -  Makefile:204: recipe for target 'l10n-check' failed
05:28:18     INFO -  mozmake.EXE[5]: *** [l10n-check] Error 2
05:28:18     INFO -  z:/build/build/src/browser/build.mk:39: recipe for target 'l10n-check' failed
05:28:18     INFO -  mozmake.EXE[4]: *** [l10n-check] Error 2
05:28:18     INFO -  z:/build/build/src/build/moz-automation.mk:98: recipe for target 'automation/l10n-check' failed
05:28:18     INFO -  mozmake.EXE[3]: *** [automation/l10n-check] Error 2
05:28:18     INFO -  z:/build/build/src/client.mk:431: recipe for target 'automation/build' failed
05:28:18     INFO -  mozmake.EXE[2]: *** [automation/build] Error 2
05:28:18     INFO -  mozmake.EXE[2]: Leaving directory 'z:/build/build/src'
05:28:18     INFO -  z:/build/build/src/client.mk:454: recipe for target 'realbuild' failed
05:28:18     INFO -  mozmake.EXE[1]: *** [realbuild] Error 2
05:28:18     INFO -  mozmake.EXE[1]: Leaving directory 'z:/build/build/src'
05:28:18     INFO -  client.mk:175: recipe for target 'build' failed
05:28:18     INFO -  mozmake.EXE: *** [build] Error 2
Flags: needinfo?(gandalf)
Sorry for the trouble Aryx.

To my defense, here's the same machine, same patch, right before I landed: https://treeherder.mozilla.org/logviewer.html#?job_id=127125957&repo=try from https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e2fd12c263

No idea why it failed now.
Pike suggested to ensure that find doesn't follow symbolic links. Per man of `find`:

```
       -P     Never  follow  symbolic  links.   This is the default behaviour.  When find examines or prints information a file, and the file is a symbolic link, the information used shall be taken from the
              properties of the symbolic link itself.
```

But maybe somehow it isn't the default behaviour on Windows. I tested that if I create a symlink loop and run find with `-L` (enable symlinks) I get this error. If I then switch to `-P` I don't.

Updated the patch to include `-P`.
Flags: needinfo?(gandalf)
Unfortunately, the `-P` switch did not fix it. I got an intermittent failure here: https://treeherder.mozilla.org/logviewer.html#?job_id=127486628&repo=try&lineNumber=41691
I was hoping that we could just find stuff generously, but apparently that's not the case.

I suggest that we limit the stuff we try to find. Maybe using $(wildcard pattern) to be cross-platform easily? Like */xpi, sea, l10n-stage?
> I suggest that we limit the stuff we try to find. Maybe using $(wildcard pattern) to be cross-platform easily? Like */xpi, sea, l10n-stage?

After discussing it on IRC Pike accepted my suggestion to try to use more direct $(RM)s for targets we want to clobber.

I pushed the updated patch to try and will try to get 5 clean green builds on windows platforms. If that succeeds, I'll re-land.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec6c7dd9e25f
Update langpack-webext to use WEBEXT_LANGPACKS mk flag. r=Pike
https://hg.mozilla.org/mozilla-central/rev/ec6c7dd9e25f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I suspect this may have broken today's linux nightly beetmover-repackage.

https://tools.taskcluster.net/groups/IQklRjIJSie4zG4QEj8F5w/tasks/EvP3DpdESSSkbb2_MQJ4kA/details

It expects to find the target.langpack.xpi in the build artifacts; the build deletes target.langpack.xpi afaict.
Sorry! Will follow up in an hour.
Hmm, looking through the task, it doesn't seem that serious. Most repackages seems to work, except of the "beetmover-repackage-linux-nightly/opt" and "beetmover-repackage-linux64-nightly/opt".

I'm not sure if I need to plaster any quick fix (which would likely be to remove the command to rm the .xpi in browser/locales/Makefile.in#clobber-l10n-check), or rather wait for Pike to get his feedback (he's the owner of the code in question).

The error log looks like this:
```
2017-09-01T14:26:00    DEBUG -   /builds/scriptworker/work/cot/FzXh684mR-mpvPs4-hLKog/public/build/target.mochitest.tests.zip matches the expected sha256 c881bf8720abe06581ef594eaea9a020002094ce2a30fd97a7995ac205c0e68e
2017-09-01T14:26:02    DEBUG -   /builds/scriptworker/work/cot/FzXh684mR-mpvPs4-hLKog/public/build/target.crashreporter-symbols.zip matches the expected sha256 aa775dd3a6b7244218fd6f988e562d05f88977311940a8f13ef744e6a71ae1eb
2017-09-01T14:26:02 CRITICAL - Chain of Trust verification error!
Traceback (most recent call last):
  File "/builds/scriptworker/lib/python3.5/site-packages/scriptworker/cot/verify.py", line 1370, in verify_chain_of_trust
    await download_firefox_cot_artifacts(chain)
  File "/builds/scriptworker/lib/python3.5/site-packages/scriptworker/cot/verify.py", line 720, in download_firefox_cot_artifacts
    return await download_cot_artifacts(chain, artifact_dict)
  File "/builds/scriptworker/lib/python3.5/site-packages/scriptworker/cot/verify.py", line 688, in download_cot_artifacts
    full_paths = await raise_future_exceptions(tasks)
  File "/builds/scriptworker/lib/python3.5/site-packages/scriptworker/utils.py", line 325, in raise_future_exceptions
    raise exc
  File "/tools/python35/lib/python3.5/asyncio/tasks.py", line 239, in _step
    result = coro.send(None)
  File "/builds/scriptworker/lib/python3.5/site-packages/scriptworker/cot/verify.py", line 645, in download_cot_artifact
    raise CoTError("path {} not in {} {} chain of trust artifacts!".format(path, link.name, link.task_id))
scriptworker.exceptions.CoTError: 'path public/build/target.langpack.xpi not in beetmover:build FzXh684mR-mpvPs4-hLKog chain of trust artifacts!'
```

:aki - I don't fully understand the consequences of those two failing. Do you need me to fix it ASAP or is it ok to fail over the weekend?
:pike - what should we do about it?
Flags: needinfo?(l10n)
Flags: needinfo?(aki)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> :aki - I don't fully understand the consequences of those two failing. Do
> you need me to fix it ASAP or is it ok to fail over the weekend?

This affects en-US linux and linux64. It looks like signed complete mars (updates) + checksum files won't make it to nightly.mozilla.org, archive.mozilla.org, or balrog (update server) until this is fixed. That's not ideal to keep broken.

Is a backout feasible? If not, the problem is restricted to a section of the nightly population, so it's not as critical as if it affected all platforms or all locales, but we should fix soon.
Flags: needinfo?(aki)
(In reply to Aki Sasaki [:aki] from comment #49)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> > :aki - I don't fully understand the consequences of those two failing. Do
> > you need me to fix it ASAP or is it ok to fail over the weekend?
> 
> This affects en-US linux and linux64. It looks like signed complete mars
> (updates) + checksum files won't make it to nightly.mozilla.org,
> archive.mozilla.org, or balrog (update server) until this is fixed. That's
> not ideal to keep broken.

We might not be uploading the tarball or gpg signature either.
Thanks for the info :aki! I filed bug 1396177 to deal with it. I'd like to avoid backing out if we can fix it instead.
Flags: needinfo?(l10n)
Backed out for breaking Beta release automation per bug 1385227.
https://hg.mozilla.org/mozilla-central/rev/c079f3f1d24a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox57: fixed → affected
Target Milestone: mozilla57 → ---
See Also: → 1416000
Moved the test part to bug 1416000. Closing
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: