Closed Bug 1048905 Opened 10 years ago Closed 10 years ago

Copy icon-font into system/fonts/hidden/ on build

Categories

(Core Graveyard :: Widget: Gonk, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: wilsonpage, Assigned: wilsonpage)

References

Details

Attachments

(3 files)

      No description provided.
Attachment #8467861 - Flags: review?(mwu)
Attachment #8467862 - Flags: review?(mwu)
Attachment #8467861 - Flags: review?(mwu) → review+
Attachment #8467862 - Flags: review?(mwu) → review+
Attachment #8467861 - Flags: review?(yurenju.mozilla)
Wilson, why do we need gaia.mk?
Flags: needinfo?(wilsonpage)
That was my recommendation. PRODUCT_COPY_FILES is the standard (Android) way to specify installation of simple files and it makes sense to have it specified in the repo that the file comes from.
Flags: needinfo?(wilsonpage)
Comment on attachment 8467861 [details] [review]
pull-request (gaia:master)

also set feedback? to Tim since I'm not sure that's right if we push files outside /system/b2g/ on gaia build system. from my view it should be a part of B2G/gecko build system.
Attachment #8467861 - Flags: feedback?(timdream)
(In reply to Yuren [:yurenju] from comment #5)
> Comment on attachment 8467861 [details] [review]
> pull-request (gaia:master)
> 
> also set feedback? to Tim since I'm not sure that's right if we push files
> outside /system/b2g/ on gaia build system. from my view it should be a part
> of B2G/gecko build system.

We have a patch (attachment 8467862 [details] [review]) that covers the B2G/Gecko build side of things. We need the Gaia build patch so that development devices stay up to date with the latest font file.
Comment on attachment 8467861 [details] [review]
pull-request (gaia:master)

I don't have a strong opinion on what dir "Gaia" build script should cover, we define it ourselves based on our needs. 

In this case I think it make sense to use PRODUCT_COPY_FILES to interface with B2G build script (maybe all Gaia file should be copied this way).

However the push-to-device part is incomplete, maybe it should skip if:

-- if (APP !== '*') ?
-- if app(s) are not push to system partition, meaning we are not remounting the partition?
Attachment #8467861 - Flags: feedback?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (OOO ~Arg 9) (MoCo-TPE) (please ni?) from comment #7)
> -- if app(s) are not push to system partition, meaning we are not remounting
> the partition?

yes but we only need to remount to rw for system partition.
Comment on attachment 8467861 [details] [review]
pull-request (gaia:master)

Wilson, two issues:
1. just like Tim mentioned, please add if condition for the command (see comment on github)
2. this change break builg system unit test, you can run test by |make build-test-unit|, it can be fixed by a small patch for test case, will be attched.
Attachment #8467861 - Flags: review?(yurenju.mozilla)
Comment on attachment 8467861 [details] [review]
pull-request (gaia:master)

- Update to explicitly push to '//system/fonts/hidden/gaia-icons.ttf' so that adb push still succeeds when hidden/ dir doesn't exist
- Update so command doesn't get run on single app installs
- Fix unit-test with Yuren's patch
Attachment #8467861 - Flags: review?(yurenju.mozilla)
Comment on attachment 8467861 [details] [review]
pull-request (gaia:master)

since "gaia-icons.ttf" is added into push-to-devices.js, please also add it into push-to-devices.test.js to pass test, otherwise looks good, r=yurenju
Attachment #8467861 - Flags: review?(yurenju.mozilla) → review+
noted, build unit test is not run on try server, so for now we need to run it manually, I will file a bug to enable it on try server
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1036394
Target Milestone: --- → 2.1 S2 (15aug)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.