Closed Bug 1362496 Opened 3 years ago Closed 3 years ago

Add chrome-% for Firefox

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We currently don't have `make chrome-%` command for Firefox, just for toolkit and Fennec.

It is needed for multi-locale Firefox desktop builds.
Need some guidance here.

I started by trying to add:

+chrome-%:
+       @$(MAKE) chrome AB_CD=$*
+


to `browser/locales/Makefile.in` but that produces an error:

```
processing /Users/zbraniecki/src/larch/browser/locales/jar.mn
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/action/jar_maker.py", line 17, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/action/jar_maker.py", line 13, in main
    return mozbuild.jar.main(args)
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/jar.py", line 597, in main
    jm.makeJar(infile, options.d)
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/jar.py", line 333, in makeJar
    self.processJarSection(info, jardir)
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/jar.py", line 388, in processJarSection
    self._processEntryLine(e, outHelper, jf)
  File "/Users/zbraniecki/src/larch/python/mozbuild/mozbuild/jar.py", line 449, in _processEntryLine
    ', '.join(src_base)))
RuntimeError: File ".deps/generated_pl/list.json" not found in /Users/zbraniecki/src/larch/browser/locales, /Users/zbraniecki/src/larch/obj-x86_64-apple-darwin16.5.0/browser/locales
```
Flags: needinfo?(bugspam.Callek)
I'm not really all that familiar with this searchplugin stuff... but:

https://dxr.mozilla.org/mozilla-central/source/mobile/locales/Makefile.in#79
is pretty different than:
https://dxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#87

Though the jar.mn directives are pretty similar. Maybe someone of the build peers (coop's team) could assist?
Flags: needinfo?(bugspam.Callek)
:coop, can you mentor me through this or point out the right person to help me?
Flags: needinfo?(coop)
Yeah, that's not going to work.

I suggest to do what libs-% does, without the jazz to put things up for langpacks.

mobile only does mobile because the jar.mn packages some toolkit files selectively. On desktop, we want the full monty, and recurse into all the directories that libs-% does.
CCing Mike, since he's the one who's been working on centralizing search for both mobile and desktop.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> :coop, can you mentor me through this or point out the right person to help
> me?

Axel's approach seems sound, and mkaply seems to have touched most of the code around there.

If you still need a build peer to assist, I can try to loop in Ted.
Flags: needinfo?(coop)
Moving over to Firefox Build, as that's what this is about.

I'm not fond of the patch, as there's a ton of repitition, which rubs me backwards. I didn't find anything non-aweful now.

I wonder if it's bad to pass XPI_ROOT_ID for all subtargets in libs-% ? If not, we could define the list of subdirs once, and then loop over that?

I'm surprised to not see searchplugins in chrome-%, too?
Component: General → Build Config
Product: Localization Infrastructure and Tools → Firefox
> I'm surprised to not see searchplugins in chrome-%, too?

I'll add that.

Also, this and bug 1362617 give me a whole ab-CD directory and ab-CD.manifest file in ./obj/dist/bin, but I still don't get it packaged I think.
Attachment #8865651 - Flags: review?(l10n)
Zibi, I think you need to find a build peer to review this, the actual implementation will depend a ton on opinions on how to make make make stuff.
What is chrome-% exactly?
Flags: needinfo?(gandalf)
Comment on attachment 8865651 [details]
Bug 1362496 - Add chrome-% for Firefox Desktop.

Ted, can you review this? Axel said that he'd love to see us not duplicating the entry list, between libs-% and chrome-%, but I don't know if that's possible.
Flags: needinfo?(gandalf)
Attachment #8865651 - Flags: review?(ted)
(In reply to Mike Kaply [:mkaply] from comment #11)
> What is chrome-% exactly?

It's the make target we use on Fx Android to stage additional localized chrome into a multi-lingual package, so that the package step can bundle it all up. It's working on slightly different paths and options than the libs-% target.
(In reply to Axel Hecht [:Pike] from comment #13)
> (In reply to Mike Kaply [:mkaply] from comment #11)
> > What is chrome-% exactly?
> 
> It's the make target we use on Fx Android to stage additional localized
> chrome into a multi-lingual package, so that the package step can bundle it
> all up. It's working on slightly different paths and options than the libs-%
> target.

Could we drive towards _removing_ this from Android rather than adding it to all platforms?  This stuff is incredibly difficult to reason about; I'd much rather Desktop lead us to the brave new world than cult-the-cult-of-the-cult from Android.
I think getting the basics shared is a good thing, and then we can talk about getting the packaging step sanitized once Callek has all the things over in taskcluster, and we only have one spot that calls l10n, and rides the train, and can be tested on try.

Note, I do think that the way that we duplicate stuff in libs-%  and chrome-% is not sane, which is why I didn't give an r+ on that patch. I didn't have a constructive counter-proposal, though, which is what I'm hoping Ted to have.
I'll be honest - my work here was pretty much guess work in trying to get things to work the way they needed. 

I actually think we're doing too much work on desktop (preprocessing the search files). That work is unnecessary now.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8865651 [details]
Bug 1362496 - Add chrome-% for Firefox Desktop.

https://reviewboard.mozilla.org/r/137266/#review158502

I think this is OK. Ideally we wouldn't repeat ourselves, but I think trying to generalize this in a make function would just wind up making it uglier.
Attachment #8865651 - Flags: review?(ted) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4da81020780
Add chrome-% for Firefox Desktop. r=ted
We definitely want to make things better, and our current plan is to start moving on that once Callek gets his l10n automation in the right state so we're not making his life harder.
https://hg.mozilla.org/mozilla-central/rev/e4da81020780
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.