Closed Bug 1011562 Opened 6 years ago Closed 5 years ago

Ship Firefox OS fonts with the simulator

Categories

(Firefox OS Graveyard :: Simulator, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(6 files, 13 obsolete files)

2.17 KB, patch
janx
: review+
Details | Diff | Splinter Review
7.71 KB, patch
janx
: review+
Details | Diff | Splinter Review
3.12 KB, patch
mshal
: review+
Details | Diff | Splinter Review
5.10 KB, patch
janx
: review+
Details | Diff | Splinter Review
1.71 KB, patch
mshal
: review+
Details | Diff | Splinter Review
1.78 KB, patch
janx
: review+
Details | Diff | Splinter Review
When bug 998844 lands, we'll be able to ship a Firefox OS font bundle along with simulators.

With bug 993137's font prefs, this will be the final step in making the Firefox OS simulator use the proper fonts on every platform (meta bug 992210)
Attachment #8444470 - Attachment is obsolete: true
Mike, I've been told you might be able to help me with this.

The attached patch roughly shows what I'm trying to achieve: When building B2G Desktop, if FXOS_FONTS is defined, download the moztt repo[1] and copy useful fonts to $BINPATH/fonts. Also bundle them with the simulator.

I have a few questions:
- How do I download a tarball from git.mozilla.org?
- Where should I extract it to?
- How do I include its fonts.mk in order to get the PRODUCT_COPY_FILES variable it defines?

Many thanks!

[1] https://github.com/mozilla-b2g/moztt or http://git.mozilla.org/?p=b2g/moztt.git;a=summary
Flags: needinfo?(mh+mozilla)
I'm not a big fan of downloading stuff as part of the build. It feels to me this should be done as a pre-build step. Chris, what do you think?
Flags: needinfo?(mh+mozilla) → needinfo?(catlee)
(See what we do for gaia, for instance)
Why not merge that in gaia, btw?
Thanks for your feedback!

(In reply to Mike Hommey [:glandium] from comment #4)
> I'm not a big fan of downloading stuff as part of the build. It feels to me
> this should be done as a pre-build step. Chris, what do you think?

I also thought about downloading this as a pre-build step, but :ochameau seemed to say this is complex and not useful. It's precious to be able to build a simulator against a local checkout of gaia, so that you can work on it, but not that much for moztt.

(In reply to Mike Hommey [:glandium] from comment #6)
> Why not merge that in gaia, btw?

That's because of the way :jfkthame implemented the dynamic loading of fonts. We're expecting a "fonts/" folder to be in the B2G Desktop runtime folder, and not in the Gaia profile. For more information see https://bugzilla.mozilla.org/show_bug.cgi?id=998844#c20
It's usually better from an automation point of view to download it as a pre-build step. For developer ease-of-use I can see it being easier to have the build download it as needed.

Something we've done in the past to address this is to be able to tell the build system that we already have the bits downloaded.

So in this case, maybe we put the fonts in tooltool, and they get downloaded prior to the build starting. The build system could notice the fonts already available locally and not attempt to download them.
Flags: needinfo?(catlee)
Blocks: 993137
What's the status of this bug?
Flags: needinfo?(janx)
I still need to figure out how to do the TODOs in the attached wip patch, with maybe an additional way to provide an already-downloaded moztt repo.

I'll give this bug another try soon.
Flags: needinfo?(janx)
Attachment #8444476 - Attachment is obsolete: true
Clone https://github.com/mozilla-b2g/moztt somewhere, and indicate the location in
your mozconfig with "MOZTTDIR=/path/to/moztt", then build the Simulator.

"./mach build" will copy the Firefox OS fonts into "obj-xxx/dist/bin/fonts/", and
"./mach package" should bundle them into the Simulator XPI.

Note: You still won't see the right fonts used, because you'll be missing prefs
from bug 993137.
Attachment #8482332 - Attachment is obsolete: true
Comment on attachment 8482350 [details] [diff] [review]
Ship Firefox OS fonts with the simulator.

Alex, please have a look.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d5bb216c1d4
Attachment #8482350 - Flags: review?(poirot.alex)
Comment on attachment 8482350 [details] [diff] [review]
Ship Firefox OS fonts with the simulator.

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

Looks good to me (works great).
But I'm not reviewer of b2g/ and we also need a build peer review.
Attachment #8482350 - Flags: review?(poirot.alex)
Attachment #8482350 - Flags: review?(mshal)
Attachment #8482350 - Flags: review?(fabrice)
Attachment #8482350 - Flags: review?(fabrice) → review+
Thanks Alex and Fabrice! This patch looks in good shape.

Chris, the next steps will be to put the moztt repo in tooltool, so it gets downloaded prior to the build, and to define its location as MOZTTDIR in the right mozconfig files.

How does it work? I'm especially keen on making sure that B2G Desktop Linux builds with the right fonts on Try, in order to unblock bug 993137.
Flags: needinfo?(catlee)
First off, we'll need to upload the fonts to tooltool.

I'm not clear which builds you're referring to here, the desktop linux b2g builds, or the mulet builds, or both?

For B2G desktop builds, the manifests like b2g/config/tooltool-manifests/linux32/releng.manifest would need updating with the font package in tooltool.

The name of the tooltool manifest used by the mulet builds needs updating:
http://hg.mozilla.org/build/buildbot-configs/file/efdd374d3260/mozilla/config.py#l714

Right now we're using browser/config/tooltool-manifests/linux64/releng.manifest, which is shared by other builds. If we want to add fonts for the simulator, then we should create a separate manifest for the mulet builds and add the fonts to that.
Flags: needinfo?(catlee)
Thanks! But maybe there is another approach: having the build scripts download the moztt repo (as it's done for Gaia at http://mxr.mozilla.org/build/source/mozharness/scripts/merge_day/b2g_tag.py#332).

The required fonts are in https://github.com/mozilla-b2g/moztt. If we upload a tarball to tooltool, I'm a bit worried that we'll have to upload new versions every time someone contributes to the moztt repo. This doesn't happen very often today, but if Gaia switches to icon fonts and chooses to host them in moztt, we'll be in trouble.

If we decide to go with a tarball upload anyway, I'll change the releng manifests for both desktop linux b2g and mulet, on all platforms (so maybe there is no need for a separate manifest?).

But do you think it might be worth it to instead edit the build scripts so they can download moztt on every build?
Flags: needinfo?(catlee)
Comment on attachment 8482350 [details] [diff] [review]
Ship Firefox OS fonts with the simulator.

>+# Copy the Firefox OS fonts if available
>+ifdef MOZTTDIR
>+include $(MOZTTDIR)/fonts.mk
>+
>+libs::
>+	@mkdir $(DIST)/bin/fonts

You would want 'mkdir -p' here, otherwise it will fail if the directory already exists.

Also, is dist/bin/fonts the correct place? We already have a dist/bin/res/fonts - is there a reason why that can't be used?

>+	@for line in $(PRODUCT_COPY_FILES) ; do `echo "$$line" | sed "s_external/moztt_cp $(MOZTTDIR)_" | sed "s :system \ $(abspath $(DIST))/bin "` ; done
>+endif

While including moztt/fonts.mk might seem like a nice way to avoid having to manage updates, it seems like things conflicts with mozbuild's export manifests. Ie: if you build this, and then run 'make export' again, the dist/bin/fonts directory gets wiped out. Essentially you'll have to copy these files on every build, and doing something every time you run a build when it's unnecessary to do so is bad.

I agree with #c4 that we shouldn't download things automatically as part of a build. If we're going to put specific versions into tooltool anyway, what do you think about listing the fonts to install in a moz.build file? You could have a script to generate that automatically from fonts.mk when a new moztt version is needed. Then the update procedure for pulling in a new moztt rev is:

1) upload moztt tarball to tooltool
2) run the fonts.mk -> moz.build script
3) update the tooltool manifests
4) commit the tooltool manifests & new moz.build file

Does that sound reasonable?
Attachment #8482350 - Flags: review?(mshal) → feedback+
How about putting the fonts into the XML manifest used to check out the sources? That way it's handled by the 'repo' tool.
Flags: needinfo?(catlee)
Thanks for the review! Here is the next iteration of the patch.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e0139e5f9990

(In reply to Michael Shal [:mshal] from comment #18)
> >+  @mkdir $(DIST)/bin/fonts
> 
> You would want 'mkdir -p' here, otherwise it will fail if the directory
> already exists.

Done.

> Also, is dist/bin/fonts the correct place? We already have a
> dist/bin/res/fonts - is there a reason why that can't be used?

- `dist/bin/fonts` is the right place as per bug 998844, because it's where fonts are loaded dynamically from.
- `dist/bin/res/fonts` is used by Fennec to do something with omni.ja and Android resources.

Jonathan, you probably know more about the difference between `dist/bin/fonts` and `dist/bin/res/fonts`. Should we open a bug to merge them, or are they entirely incompatible?

> While including moztt/fonts.mk might seem like a nice way to avoid having to
> manage updates, it seems like things conflicts with mozbuild's export
> manifests. Ie: if you build this, and then run 'make export' again, the
> dist/bin/fonts directory gets wiped out. Essentially you'll have to copy
> these files on every build, and doing something every time you run a build
> when it's unnecessary to do so is bad.
> 
> I agree with #c4 that we shouldn't download things automatically as part of
> a build. If we're going to put specific versions into tooltool anyway, what
> do you think about listing the fonts to install in a moz.build file? You
> could have a script to generate that automatically from fonts.mk when a new
> moztt version is needed.

I'm not exactly sure how this works, but I created a separate moz.build file and folder for the fonts. The moz.build file is empty because everything went into the Makefile.in, am I doing this correctly? And I don't know what `make export` is for, but does my change fix the wipe-out/rebuild problem?

The workflow I test with is:
1) Add `MOZTTDIR = /path/to/moztt` to the mozconfig file.
2) Run `./mach build` then verify the fonts are in `obj-xxx/dist/bin/fonts`.
3) Run `./mach package` then unzip `obj-xxx/dist/fxos-simulator-xxx.xpi` and verify the fonts are in `b2g/fonts`.

> Then the update procedure for pulling in a new
> moztt rev is:
> 
> 1) upload moztt tarball to tooltool
> 2) run the fonts.mk -> moz.build script
> 3) update the tooltool manifests
> 4) commit the tooltool manifests & new moz.build file
> 
> Does that sound reasonable?

That sounds reasonable, but I'm not sure there is value in generating the build file instead of including `fonts.mk` at build time.

(In reply to Chris AtLee [:catlee] from comment #19)
> How about putting the fonts into the XML manifest used to check out the
> sources? That way it's handled by the 'repo' tool.

I'm not sure I understand your suggestion. The `repo` tool already handles fonts because it's used as an external resource in the B2G repository to copy them onto the device. This feature however is to add them to the simulator, which is built directly from mozilla-central with mozconfig references to gaia and moztt repos.
Attachment #8482350 - Attachment is obsolete: true
Attachment #8504391 - Flags: review?(mshal)
(needinfo for the question about dist/bin/fonts and dist/bin/res/fonts in comment #20)
Flags: needinfo?(jfkthame)
(In reply to Jan Keromnes [:janx] from comment #20)

> - `dist/bin/fonts` is the right place as per bug 998844, because it's where
> fonts are loaded dynamically from.
> - `dist/bin/res/fonts` is used by Fennec to do something with omni.ja and
> Android resources.
> 
> Jonathan, you probably know more about the difference between
> `dist/bin/fonts` and `dist/bin/res/fonts`. Should we open a bug to merge
> them, or are they entirely incompatible?

There are two quite different things going on here. On fennec, we bundle a set of fonts in omni.ja, which we can do because font enumeration and loading is entirely within our control, and we can read/load them from the archive as needed. For the B2G simulator on desktop platforms, OTOH, we need the fonts installed as standalone files at a known location in the filesystem, which we can then hand to the system libraries/APIs that manage font loading and rendering.

So the two mechanisms are quite distinct and have different requirements: on fennec, we want the fonts in omni.ja to keep our footprint on the device smaller, while on desktop simulators we need the fonts in the filesystem so that the OS components can see them. Given the different requirements for how the fonts are ultimately packaged and installed, I don't think there's any benefit in trying to merge these in dist/bin.
Flags: needinfo?(jfkthame)
(In reply to Jan Keromnes [:janx] from comment #20)
> I'm not exactly sure how this works, but I created a separate moz.build file
> and folder for the fonts. The moz.build file is empty because everything
> went into the Makefile.in, am I doing this correctly? And I don't know what
> `make export` is for, but does my change fix the wipe-out/rebuild problem?

Ahh, I was thinking we had a way to just install files from moz.build, but it looks like bug 853594 isn't finished yet, so I guess adding the moz.build file won't help. In any case, you can at least use the existing INSTALL_TARGETS variables to copy files rather than creating your custom libs rule to copy files. Something like this:

include $(MOZTTDIR)/fonts.mk

INSTALL_TARGETS += MOZTT
MOZTT_TARGET := libs
MOZTT_DEST := $(DIST)/bin/fonts
MOZTT_FILES := $(patsubst external/moztt/%,$(MOZTTDIR)/%,$(filter external/moztt/%,$(subst :, ,$(PRODUCT_COPY_FILES))))

include $(topsrcdir)/config/rules.mk

That doesn't fix the problem I was talking about, since we still remove the dist/bin/fonts directory during the export phase (ie: if you run 'mach build' and keep doing 'ls dist/bin/fonts' in another terminal, you'll see all the files are removed, then re-created later. 'make export' just does the first part of the build). But I don't see how you can address that here - that's just our build system being broken. At least using the INSTALL_TARGETS variable will mean this can be converted into moz.build when the time comes, rather than being left as a straggler one-off libs target.

gps, is there a better solution you can suggest?
Flags: needinfo?(gps)
We need to live with INSTALL_TARGETS in Makefile.in until we support a better way in moz.build. Sorry.
Flags: needinfo?(gps)
Comment on attachment 8504391 [details] [diff] [review]
Ship Firefox OS fonts with the simulator.

Thanks for the feedback! I'll come back with an updated patch.
Attachment #8504391 - Flags: review?(mshal)
Switched to using INSTALL_TARGETS.
Attachment #8504391 - Attachment is obsolete: true
Attachment #8505821 - Flags: review+
Comment on attachment 8505821 [details] [diff] [review]
Ship Firefox OS fonts with the simulator.

Michael, thanks for the `patsubst` one-liner. Please have a look at the updated patch.
Attachment #8505821 - Flags: review?(mshal)
Attachment #8505821 - Flags: review?(mshal) → review+
There is a certain zen side to these all-green try pushes.
This is supposed to make releng machines download the moztt archive from tooltool and build the Firefox OS fonts into B2G Desktop simulators.

Try for both patches: https://tbpl.mozilla.org/?tree=Try&rev=275b5125a692
TBPL cross-fire, trying again: https://tbpl.mozilla.org/?tree=Try&rev=9975051f960d
Re-uploaded archive with a "moztt" subfolder.

https://tbpl.mozilla.org/?tree=Try&rev=8ae3641a31f9
Attachment #8508375 - Attachment is obsolete: true
Comment on attachment 8508396 [details] [diff] [review]
Use Firefox OS fonts in B2G Desktop tests.

Michael, please have a look.

I'm basically telling releng to build all B2G Desktop targets with the Firefox OS fonts I uploaded to tooltool.

However, windows is misbehaving: does "unpack":"True" work on windows?
Attachment #8508396 - Flags: review?(mshal)
Comment on attachment 8508396 [details] [diff] [review]
Use Firefox OS fonts in B2G Desktop tests.

Looks good!

For the Windows build, it seems there was a problem with bug 1027669 (deploying a newer tooltool.py on Windows machines), which is being fixed. Can you try again before landing? I'm not sure how long it will take to update, so if it doesn't work right away just give it some time before trying again.
Attachment #8508396 - Flags: review?(mshal) → review+
Thanks! Sure, I'll re-trigger for a while until it's green.
Rebased, carried over.
Attachment #8505821 - Attachment is obsolete: true
Attachment #8508943 - Flags: review+
Rebased, carried over.

Trying both: https://tbpl.mozilla.org/?tree=Try&rev=2022a8990953 .
Attachment #8508396 - Attachment is obsolete: true
Attachment #8508944 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9cf689a48b8f
https://hg.mozilla.org/mozilla-central/rev/78245457fc60
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Forgot about the Mulet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Ship Firefox OS fonts in Mulet. (obsolete) — Splinter Review
I forked the tooltool manifests used by Mulet so that I could add the Firefox OS fonts archive to them.

Not sure that calling them "mulet.manifest" and putting them into b2g/config/tooltool-manifests/ is the right way to do it.

Also, I didn't find a tooltool manifest for the macosx-universal/mulet mozconfig, so I ignored it.
Attachment #8512321 - Flags: review?(catlee)
This is supposed to make buildbots use the new mulet tooltool manifests introduced in the other patch.

I'm not sure how to test this so I wrote the patches blindly. Chris, can you please verify that it behaves as expected?
Attachment #8512327 - Flags: review?(catlee)
Comment on attachment 8512327 [details] [diff] [review]
Make mulet use its own tooltool manifests.

Maybe Chris is too busy. Michael, could you please have a look?
Attachment #8512327 - Flags: review?(catlee) → review?(mshal)
Attachment #8512321 - Flags: review?(catlee) → review?(mshal)
Comment on attachment 8512321 [details] [diff] [review]
Ship Firefox OS fonts in Mulet.

>diff --git a/b2g/config/tooltool-manifests/linux64/mulet.manifest b/b2g/config/tooltool-manifests/linux64/mulet.manifest

Since mulet is b2g/dev, we should probably have these as b2g/dev/config/tooltool-manifests/linux64/releng.manifest (similarly for other platforms). Sound reasonable to you?

Also, you may not need the setup.sh script in the manifest anymore. The archives should be unpacked automatically.
Attachment #8512321 - Flags: review?(mshal) → feedback+
Attached patch Ship Firefox OS fonts in Mulet. (obsolete) — Splinter Review
Thanks for the feedback Michael! I moved the tooltool manifests to b2g/dev/.

I'll leave the setup.sh removal as a follow-up once the fonts work in Mulet.
Attachment #8512321 - Attachment is obsolete: true
Attachment #8512327 - Attachment is obsolete: true
Attachment #8512327 - Flags: review?(mshal)
Attachment #8517368 - Flags: review?(mshal)
Attachment #8517399 - Attachment is patch: true
Comment on attachment 8517368 [details] [diff] [review]
Ship Firefox OS fonts in Mulet.

Sounds reasonable to me!
Attachment #8517368 - Flags: review?(mshal) → review+
Attachment #8517399 - Flags: review?(mshal) → review+
Thanks Michael! Carried your r+ over.

Commented mozconfig changes out while releng switches to the new tooltool manifests.
Attachment #8517368 - Attachment is obsolete: true
Attachment #8518112 - Flags: review+
Sheriffs, please land the patch "Ship Firefox OS fonts in Mulet".

No try, because I'm adding comments, and three unused files.
After it landed, releng will try switching to these new tooltool manifests.
Keywords: checkin-needed
Michael, can I please ask you to land the buildbot patch "Make mulet use its own tooltool manifests" the right way once the new releng manifests are landed in m-c?
Flags: needinfo?(mshal)
https://hg.mozilla.org/mozilla-central/rev/8a53b8b76456
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S7 (24Oct) → 2.1 S8 (7Nov)
Forgot leave-open. Michael still needs to update the buildbots, and I'll then uncomment the font code.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
buildbot-configs: https://hg.mozilla.org/build/buildbot-configs/rev/bd6998682ead

Just needs a reconfig...
Flags: needinfo?(mshal)
Last part of the 3-step land, uncomment the fonts line in Mulet's mozconfigs.
Comment on attachment 8518934 [details] [diff] [review]
Build Mulet with the Firefox OS fonts.

Seems about green, and Mulet builds nicely with the fonts. Michael, shall we move forward with this?
Attachment #8518934 - Flags: review?(mshal)
Attachment #8518934 - Flags: review?(mshal) → review+
Depends on: 1095679
(In reply to Wes Kocher (:KWierso) from comment #52)
> https://hg.mozilla.org/mozilla-central/rev/8a53b8b76456

Uplifted to Aurora/Beta to un-bust Mulet builds. Please needinfo me if/when attachment 8518934 [details] [diff] [review] is landed and ready for uplift as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0686d3ac4a9
https://hg.mozilla.org/releases/mozilla-beta/rev/00f9c65b2f83
Previous try for last patch "Build Mulet with the Firefox OS fonts" was greenish (apart from e10s orange):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e0d913b931

Trying again, just in case:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0210b372e3
I re-triggered the few oranges to green, so checkin-needed for last patch "Build Mulet with the Firefox OS fonts" please.
(In reply to Jan Keromnes [:janx] from comment #62)
> I re-triggered the few oranges to green, so checkin-needed for last patch
> "Build Mulet with the Firefox OS fonts" please.

done in remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/e0b32a35fe6f
Keywords: checkin-needed
Changes to b2g/app/Makefile.in done in patch "Ship Firefox OS fonts with the simulator" also need to be applied to b2g/dev/app/Makefile.in to affect Mulet.
Keywords: leave-open
Fabrice, mind having a look?

I'm moving the code that responds to the MOZTTDIR environment variable to install the Firefox OS fonts into a dist/bin/fonts/ dir. Before, it was only available to B2G builds, and I'm making it available to other builds, like Mulet and desktop browser.

This has two motivations:
- Mulet needs to install the Firefox OS fonts to look more like a device.
- Bug 998844 dynamic font loading works on all platforms, so it makes sense to allow all platforms to build with MOZTT fonts if they so wish.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf095ed9ca7f
Attachment #8521522 - Attachment is obsolete: true
Attachment #8522118 - Flags: review?(fabrice)
Greenest try I've seen in a while.
Comment on attachment 8522118 [details] [diff] [review]
Enable the packaging of MOZTT fonts on all build targets.

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

You want a build peer to review that.
Attachment #8522118 - Flags: review?(fabrice)
Please bounce reviews to more appropriate reviewers instead of dropping them.
Comment on attachment 8522118 [details] [diff] [review]
Enable the packaging of MOZTT fonts on all build targets.

Mike, could you please have a look?

It's a simple change with an all-green try. Details in comment 67.
Attachment #8522118 - Flags: review?(mh+mozilla)
Comment on attachment 8522118 [details] [diff] [review]
Enable the packaging of MOZTT fonts on all build targets.

Or Michael please?
Attachment #8522118 - Flags: review?(mh+mozilla) → review?(mshal)
Attachment #8522118 - Flags: review?(mshal) → review+
Thanks Michael!

"checkin-needed" for last patch "Enable the packaging of MOZTT fonts on all build targets".

Please land this patch together with the two patches from bug 993137.

Try for all three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c66d38ae32b
Other bug needs rebasing.
Keywords: checkin-needed
Rebased, carried over.
Attachment #8522118 - Attachment is obsolete: true
Attachment #8523399 - Flags: review+
Please see comment 73.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a39986967fbd
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.