Closed Bug 1022728 Opened 11 years ago Closed 10 years ago

Prevent building gaia when running ./build.sh gecko

Categories

(Firefox OS Graveyard :: Gaia::Build, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Build
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ochameau, Assigned: mwu)

References

Details

Attachments

(6 files, 1 obsolete file)

gaia patch
46 bytes, text/x-github-pull-request
mwu
: feedback-
Details | Review
Prevent building gaia when running ./build.sh gecko. r=yurenju,mwu
1.00 KB, patch
Details | Diff | Splinter Review
[PR] Add preferences only target to gaia's Android.mk
46 bytes, text/x-github-pull-request
yurenju
: review+
Details | Review
Gaia PR for |make settings|
46 bytes, text/x-github-pull-request
gduan
: review+
mwu
: feedback+
Details | Review
Gaia PR for |make settings| v2
46 bytes, text/x-github-pull-request
Details | Review
[PR] Depend on gaia-prefs rather than profile.tar.gz (reland)
49 bytes, text/x-github-pull-request
Details | Review
Today, when you run `$ ./build.sh gecko` from your B2G folder, it takes a while, just to rebuild unecessarely gaia. I don't think we want to do anything in gaia folder when running `build.sh gecko`. This issue is highlighted by the fact that we rebuild gaia completely each time we run make in gaia. This should be fixed soon by bug 1015868 or similar. But even then, I think we will still save some seconds by ignoring gaia folder at all when running this command.
Attached file gaia patch —
I'm all but a Makefile expert, so suggestions are welcomed! I tried to prevent even looking at gaia folder. I've seen some reference to it from gonk-misc/b2g.mk, the PRODUCT_PACKAGES list, but it doesn't seems to be it or it isn't computed on each call to ./build.sh... Anyway, with this patch we don't build gaia anymore when running ./build.sh gecko and on my laptop I save ~40s of build time (4m25s on master and 3m38s with this patch, given that ./build.sh gaia takes 54s)
Attachment #8437026 - Flags: review?(yurenju.mozilla)
Attachment #8437026 - Flags: feedback?(mwu)
Comment on attachment 8437026 [details] [review] gaia patch good to me but I also want to hear micheal's opinion.
Attachment #8437026 - Flags: review?(yurenju.mozilla) → review+
Yuren, do you think we can ask feedback to anyone else given that we don't hear back from mwu?
Comment on attachment 8437026 [details] [review] gaia patch Hey Marco, can you help us review this patch?
Attachment #8437026 - Flags: feedback?(mwu) → feedback?(mchen)
Comment on attachment 8437026 [details] [review] gaia patch This approach has issues. I just haven't been able to respond.
Attachment #8437026 - Flags: feedback?(mchen) → feedback?(mwu)
Comment on attachment 8437026 [details] [review] gaia patch In the particular case, gecko depends on gaia for its settings files. If we can create a target specifically for the necessary gecko pref files, we can make this much quicker. I prefer doing this over carving out a special case for when we're only building gecko.
Attachment #8437026 - Flags: feedback?(mwu) → feedback-
Comment on attachment 8437026 [details] [review] gaia patch (In reply to Michael Wu [:mwu] from comment #6) > Comment on attachment 8437026 [details] [review] > gaia patch > > In the particular case, gecko depends on gaia for its settings files. Oh! I didn't realized that. Is it only the profile/user.js file? Is it pulled over here: https://github.com/mozilla-b2g/gaia/blob/master/Android.mk#L59 or from somewhere else outside of this makefile? > If we > can create a target specifically for the necessary gecko pref files, we can > make this much quicker. We already have that, the `preferences` target. > I prefer doing this over carving out a special case > for when we're only building gecko. I'm afraid I have to keep doing a special case as, if I understand this correctly, this makefile (and esp. this `$(LOCAL_PATH)/profile.tar.gz` target) is also going to be used when building the full gaia profile?
Attachment #8437026 - Flags: review+ → review?(mwu)
Yuren & Alex, it would be great to land this finally! What can we do to move it forward? Can someone other than :mwu review this and find out what's needed to make progress?
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(poirot.alex)
Address the review comments and it can be moved forward. (In reply to Alexandre Poirot [:ochameau] from comment #7) > > I prefer doing this over carving out a special case > > for when we're only building gecko. > > I'm afraid I have to keep doing a special case as, if I understand this > correctly, this makefile (and esp. this `$(LOCAL_PATH)/profile.tar.gz` > target) is also going to be used when building the full gaia profile? Not sure what you're looking at, but no this special case is not necessary.
Flags: needinfo?(yurenju.mozilla)
(In reply to Michael Wu [:mwu] from comment #9) > (In reply to Alexandre Poirot [:ochameau] from comment #7) > > > I prefer doing this over carving out a special case > > > for when we're only building gecko. > > > > I'm afraid I have to keep doing a special case as, if I understand this > > correctly, this makefile (and esp. this `$(LOCAL_PATH)/profile.tar.gz` > > target) is also going to be used when building the full gaia profile? > > Not sure what you're looking at, but no this special case is not necessary. Did you meant something like this? It doesn't work for the reason I tried to explain. When we do a complete build, `./build.sh`, we expect to build the full gaia profile here... mv: cannot stat `out/target/product/unagi/data/local/webapps': No such file or directory make: *** [out/target/product/unagi/data/local/gaia] Error 1 Or did you had something else in mind?
Flags: needinfo?(poirot.alex)
review/question ping
Flags: needinfo?(mwu)
No - prefs can be handled separately. No need to change the profile.tar.gz path.
Flags: needinfo?(mwu)
Attachment #8490423 - Flags: review?(yurenju)
Assignee: nobody → mwu
Attachment #8490425 - Flags: review?(dhylands)
I tried applying the 2 patches (to gaia and gonk-misc) and I'm not seeing any significant differences when doing: ./build.sh gecko It still seems to build gaia (I was comparing the build output with and without the patches).
Doh - I need all 3
All three? There should be only two patches necessary.
(In reply to Michael Wu [:mwu] from comment #17) > All three? There should be only two patches necessary. OK - I was confused. But - after applying the above 2 patches, and doing: rm gaia/profile.tar.gz ./build/sh gecko then its still building gaia/profile.tar.gz and I'm seeing stuff like: [app] building music app... [app] building collection app... [app] building browser app... [app] building wallpaper app... [app] building gallery app... [app] building system app... [app] building calendar app... [app] building homescreen app... in my build log.
profile.tar.gz isn't even a dependency anymore if you applied the two patches though. What device are you building for? - maybe there's something else there adding a dependency?
I'm doing an eng non-debug build for flame. What I see in the build log is this: 1 - make[1]: Leaving directory `/home/work/B2G-flame/objdir-gecko-eng-b2g-inbound' 2 - make -C /home/work/B2G-flame/gaia GAIA_DEV_PIXELS_PER_PX=1.5 preferences 3 - make[1]: Entering directory `/home/work/B2G-flame/gaia' 4 - Test SDK directory: /home/work/B2G-flame/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 5 - test -f /home/work/B2G-flame/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell 6 - run-js-command gaia/preferences 7 - make[1]: Leaving directory `/home/work/B2G-flame/gaia' 8 - make -C /home/work/B2G-flame/gaia GAIA_DEV_PIXELS_PER_PX=1.5 profile 9 - make[1]: Entering directory `/home/work/B2G-flame/gaia' line 2 from above is the change due to the patch. line 8 is what's building the profile. I'm trying to figure out where that's coming from.
line 8 is from inside the $(LOCAL_PATH)/profile.tar.gz recipie. So now to figure out where the dependency is coming from.
It's from device/qcom/msm8610/msm8610.mk.
Comment on attachment 8490425 [details] [review] [PR] Depend on gaia-prefs rather than profile.tar.gz This looks good to me. For the flame, I had to also change device/qcom/msm8610/msm8610.mk -gaia/profile/defaults/pref/lmk.js: gaia/profile.tar.gz +gaia/profile/defaults/pref/lmk.js: gaia-prefs in order for it to not build gaia. Although the recipie in msm8610.mk still seems to be incorrect. It has: > gaia/profile/defaults/pref/lmk.js: gaia-prefs > echo 'pref("hal.processPriorityManager.gonk.BACKGROUND.KillUnderMB", 10);' > $@ > echo 'pref("hal.processPriorityManager.gonk.notifyLowMemUnderMB", 9);' >> $@ and gaia/profile is hardcoded (it might be gaia/profile-debug or gaia/profile-test).
Attachment #8490425 - Flags: review?(dhylands) → review+
Comment on attachment 8490423 [details] [review] [PR] Add preferences only target to gaia's Android.mk really great for me!!
Attachment #8490423 - Flags: review?(yurenju) → review+
Gaia build target merged first - https://github.com/mozilla-b2g/gaia/commit/56fe922c59253b1c0f902ab94e2d5b572dec4d71 The gonk-misc change will land later since it depends on this target.
Attachment #8437026 - Flags: review?(mwu)
m1 - FYI, this makes it so we don't try to build all of gaia every time we try to build gecko. This doesn't work on CAF based devices due to the lmk.js rule. It can be fixed per comment 23. If you're interested, I can also request approval to land on 2.1 / 2.0.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
gonk-misc part backed out. Apparently we're missing settings.json now. https://github.com/mozilla-b2g/gonk-misc/commit/5883a99b6528ced9dafaed8d3ca2405fb285537e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
:mwu, could you mentor :rickychien to work on this? What's blocking this to be fixed?
Flags: needinfo?(ricky060709)
Flags: needinfo?(mwu)
Sure. FWIW, I'm in the Taipei office this week and next week if you or Ricky want to talk about this in person. It looks like we need to be able to build settings.json by itself in the makefile and have gonk-misc/Android.mk depend on that rule.
Flags: needinfo?(mwu)
It seems not so hard for me. I'm going to do this. Maybe I'll need your help to test it (Not familiar with how to use ./build.sh) after I submitted my patch.
Flags: needinfo?(ricky060709)
Attachment #8522073 - Flags: review?(gduan)
Comment on attachment 8522073 [details] [review] Gaia PR for |make settings| r=gduan, it looks good to me!
Attachment #8522073 - Flags: review?(gduan) → review+
Comment on attachment 8522073 [details] [review] Gaia PR for |make settings| PR is updated for solving settings.json missing in profile/defaults
Attachment #8522073 - Flags: feedback?
Attachment #8522073 - Flags: feedback? → feedback?(mwu)
Comment on attachment 8522073 [details] [review] Gaia PR for |make settings| Works for me. Will land my gonk-misc change after this lands. Thanks!
Attachment #8522073 - Flags: feedback?(mwu) → feedback+
Thanks! Merged. https://github.com/mozilla-b2g/gaia/commit/391ebb892b03d0e4b653591d94fcdaa6a5ff2d4b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Sorry we should wait for gonk-mise and resolve bug. lol
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Almost emulators are green on try. I'm sure problem solved. Gaia PR merged again. https://github.com/mozilla-b2g/gaia/commit/95f69a805023760c42f243a46ca53256df64e6c0
Hi Michael, ni you for reminding you to land gonk-misc patch.
Flags: needinfo?(mwu)
Attachment #8490423 - Attachment is obsolete: true
Flags: needinfo?(mwu)
Attachment #8527763 - Attachment description: [PR] Add preferences only target to gaia's Android.mk (reland) → [PR] Depend on gaia-prefs rather than profile.tar.gz (reland)
Attachment #8490423 - Attachment is obsolete: false
Attachment #8490425 - Attachment is obsolete: true
https://github.com/mozilla-b2g/gonk-misc/commit/50ad16a280fe9cfa0716f8c6ba16afdf7f266b49
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: