Closed Bug 897352 Opened 11 years ago Closed 10 years ago

[gaia build system] Transition to a build_stage for assembling apps

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Build
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g-v1.4 unaffected, b2g-v2.0 fixed)

Status:
RESOLVED FIXED
Milestone:
1.4 S5 (11apr)
Tracking Flags:
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed

People

(Reporter: jrburke, Assigned: yurenju)

References

Details

Attachments

(3 files)

github PR: https://github.com/mozilla-b2g/gaia/pull/16604
(deleted), text/x-github-pull-request
gduan
: review+
ochameau
: review+
Details
colon.patch
(deleted), patch
Details | Diff | Splinter Review
github PR: https://github.com/mozilla-b2g/gaia/pull/17802
(deleted), text/x-github-pull-request
ochameau
: review+
julienw
: feedback+
Details 
This is a follow on bug to 890182, where :ochameau suggested transitioning all app builds to using a staging directory for assembly. Here are the relevant notes from that bug:

On using `build_stage/$(app)` for all apps:

* Call app's Makefile with "copy_stage" build rule. If the app has custom copy logic, as email does, it would implement that rule. If no "copy_stage" in the Makefile, default general behavior is to copy the app to build_stage/$(app). If this worked out the gaia_build.json hook that is used just by email now could be removed.

* After copying to the stage is done (either by main Makefile or by app's Makefile) if the app has a custom Makefile in build_stage/$(app), run it (to match the custom app Makefile behavior that exists today).

* Do rest of build tasks using build_stage/$(app).

What I am unsure about is how the DEBUG=1 pathway should work, as that seems to point to the source directories, even though today, all app Makefiles are run.

Also, it seems like modifying all the apps that have a custom Makefile may need some coordination to work out the "call copy_stage optionally, then call into Makefile after the staged directory is set" logic. In particular, should that Makefile be run in the stage directory in triggered from the source area, particularly given the DEBUG=1 nuances.
Assignee: nobody → yurenju.mozilla
and we should always assign build_stage/$APP to webapp.buildDirectoryFile in |utils.js:makeWebappsObject()|
try to use copy.js both for node.js and addon-sdk in camera app, but seems we need to spend some time to add addon-sdk support.

I will be fallback to copy.mk solution for this issue.
Blocks: 973786
No longer depends on: 973786
Tim, could you review this pr? thank you.
Attachment #8381266 - Flags: review?(timdream)
Hi Diego, now we have two stage for assembling apps, one is copy stage, another is assemble stage, since Makefile in camera app use r.js to concat all javascript to main.js, I think it whould be in copy stage.

for this, I need to rename Makefile to copy.mk to align all apps in gaia, which means you need to build camera app with |make -f copy.mk| in camera directory, is it okay to you?
Flags: needinfo?(dmarcos)
I figure out a solution for copy.mk, will update soon.
Flags: needinfo?(dmarcos)
Attachment #8381266 - Flags: review?(timdream)
after try some approach to keep your original workflow but doesn't work, so the same question here on comment 4.
Flags: needinfo?(dmarcos)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

finally figured out how to keep original workflow for camera app.
Attachment #8381266 - Flags: review?(timdream)
Flags: needinfo?(dmarcos)
I'm working on multilocale error issue.
we have integration test cases to cover this refactoring.
- build/test/integration/build.test.js
- build/test/integration/distribution.test.js
- build/test/integration/multilocale.js
Flags: in-testsuite+
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Alex, could you review this pull request? thank you!
Attachment #8381266 - Flags: review?(timdream) → review?(poirot.alex)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

George, could you also take a look? thank you!
Attachment #8381266 - Flags: review?(gduan)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Great work, and the makefile rule to automatically update changed files is awesome! Thanks!

r=gduan
Attachment #8381266 - Flags: review?(gduan) → review+
modified accroding first round review!
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

I don't know exactly what you are aiming for this particular bug. That would be great if you can describe where you are in your refactoring plan and what are the next steps.

If that's only about build stage folder, I wish we define the per-app stage folder (often called BUILD_DIR in app Makefiles) in root Makefile and propagate it to app makefiles as well as js build scripts. So that it is defined only in one place and we never wild guess or hardcode this path anywhere.
If we want to go deeper into the copy.mk story, I imagine we can drop all naive app Makefile and call app's build.js from the root makefile.


r- for red travis, keyboard exception and settings issue mentioned here:
https://github.com/mozilla-b2g/gaia/pull/16604/files#r10257099
All refactoring can be kept for followups, but feel free to tweak this patch and request another review!
Attachment #8381266 - Flags: review?(poirot.alex) → review-
Hi Alex,

I would like to handle copy rule and build.js stuff in this bug, so let's fix issues which you mentioned on github. There are some action items:

1. define BUILD_DIR per app in root Makefile
2. execute $(APP)/build/build.js for each app if exists
3. remove |@$(call run-app-js-command, build)| from copy.mk because (2)
4. remove Makefile for each app if it's unecessary
5. fix travis, keyboard and settings issue

what do you think?
Flags: needinfo?(poirot.alex)
Sounds like a perfect plan!
Flags: needinfo?(poirot.alex)
figure out build process for all apps:

Makefile should be removed:
- browser, communications, homescreen, keyboard, sms, system, wallpaper, wappush

Makefile should be kept and migrate with copy.mk:
- calendar: execute |build.js| to get presets.js and another rule for downloading web assets (keep it and add copy stuffs into Makefile)
- camera, clock: execute build.js to get config.js then use r.js to optimize and concat scripts into main.js (keep)
- email: use r.js to optmize and concat (keep)
- gallery: concat scripts in Makefile (keep)
- settings: execute |build.js| and use r.js (keep)

and copy rule & app-makefile rule will be migrated to one rule because some apps need generating some files before optimizing by r.js, and other apps only need to execute build.js, below is the pseudo code:

> for app in apps:
>   if app.makefile exists:
>     execute app.makefile // Makefile should execute build.js if necessary
>   else:
>     copy app to stage
>     if app.build.js exists:
>       execute app.build.js
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Hi Alex, I fixed issues which you mentioned, could you take a quick look? I'll do more testing tomorrow.
Attachment #8381266 - Flags: feedback?(poirot.alex)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

I'll request a review later since I have tested most cases today morning.
Attachment #8381266 - Flags: feedback?(poirot.alex)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Hi Alex, I have updated pull request, could you review again?
Attachment #8381266 - Flags: review- → review?(poirot.alex)
pull request is updated for reviewer's comments
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Yay for all these Makefile that go away!

I still see some build folders and build.txt file in app packages.
Gallery's build.js has issues with concatenated files.
Also we should ensure replacing Makefiles by something simple,
build.js files looks very cryptic with these getFile's.
Attachment #8381266 - Flags: review?(poirot.alex) → review-
Summarize for these changes:
1. Move variables definition from Makefile to build/common.mk to unify run-js-command for app
2. assembling APP_DIR, STAGE_APP_DIR in xpcshell-common.js
3. remove all assembling APP_DIR, STAGE_APP_DIR from each app
and I didn't modify concat part, I would like to file another follow up bug since this pull request is too large :-/
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

let's try to review again.
Attachment #8381266 - Flags: review- → review?(poirot.alex)
travis-ci passed.
Diego, this pull request will break workflow for |make| in camera directory because we don't want to have duplicate code between gaia build system and app build system

you can use |APP=camera make| in root directory of gaia to build camera app only.
Flags: needinfo?(dmarcos)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Cancel review flag because it breaks USE_LOCAL_XULRUNNER_SDK workflow.
Attachment #8381266 - Flags: review?(poirot.alex)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

issue for USE_LOCAL_XULRUNNER_SDK has been fixed, set review again.
Attachment #8381266 - Flags: review?(poirot.alex)
Comment on attachment 8381266 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/16604

Thanks Yuren, that's a significant step forward for the build system!

I just added some more tweaks comment but the patch looks good overall and app packaged as well as source tree are clean now!!
My only concern with last changesets is the big move of Makefile code to common.mk, which may disturb too many developers for no clear value.
Attachment #8381266 - Flags: review?(poirot.alex) → review+
I found a way to keep variables definition in Makefile and leave commands such as run-js-command in common.mk, so let's move them back.
Component: Gaia → Gaia::Build
Merged \o/

Thank you Alex!!!!

https://github.com/mozilla-b2g/gaia/commit/88c8c35d3cbf0fca98df8d62d46ce230f464e182
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
backout since it breaks windows build.

https://tbpl.mozilla.org/php/getParsedLog.php?id=36425013&tree=B2g-Inbound

commit: 06d3242182370ba93db9acbe54cbe6cc8f7b239f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
landed again since windows build on try server was passed.

https://github.com/mozilla-b2g/gaia/commit/f99259252afc87a8ec443d4848108fcc184d2fc4

https://tbpl.mozilla.org/?tree=Try&rev=cf4d13e17313
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
had to revert this again because of breaking windows builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=36435051&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Could it be that try was running make and b2g-inbound is using pymake?
it has been ran on try server (see comment 37), do you have any suggestion for this situation?
Flags: needinfo?(cbook)
Hi sheriffs, my pushing for mozilla-central & b2g-inbound on try server are green, can I land this commit?

Kyle suggest to take a "clobber" to work but I don't know how to do this for a gaia commit, do you have any suggestion?

thanks
Flags: needinfo?(sheriffs)
discussed with Timcat and clobbered, so landed again.

https://github.com/mozilla-b2g/gaia/commit/ad3ff4d8515803a5467957f3c39faf83653b5510
Flags: needinfo?(sheriffs)
Flags: needinfo?(cbook)
s/Timcat/Tomcat/g, sorry.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
sorry this broke b2g-i again :( like https://tbpl.mozilla.org/php/getParsedLog.php?id=36596222&tree=B2g-Inbound - same error again

glandium, khuey: any idea whats going on here ? Even clobbered the tree but this didn't fixed the problem
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(khuey)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The problem likely comes from the use of backslashes as path separator in make targets/dependencies/rules. That it worked so far is almost miraculous.
Flags: needinfo?(mh+mozilla)
Sorry, but I don't really have time to help on build stuff these days.
Flags: needinfo?(khuey)
suddenly I can reproduce on my windows box, the updated commit will be landed again and I will keep an eye on try server.
landed again

https://github.com/mozilla-b2g/gaia/commit/11052eb1b9f5d5352b280d6e96701b75dd837fb2

try server link for b2g-inbound will be posted.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
got another problem, backout.

99711204d5b01eea61f04dab9a254856ff08a64e
sorry had to revert his again for https://tbpl.mozilla.org/php/getParsedLog.php?id=36729314&tree=B2g-Inbound

seems this failed with

/bin/bash: rsync: command not found
/bin/bash: rsync: command not found
make[6]: *** [c:\\builds\\moz2_slave\\b2g-in-w32_g-00000000000000000\\build\\gaia\\apps\\bluetooth\\test\\unit\\_sandbox.html] Error 127
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
oh my god...
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
merged.

https://github.com/mozilla-b2g/gaia/commit/db1f7fba8e166807e6b82ef2d9b4b729176d0b09
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
revert again, WHY I ALWAYS CAN'T REPRODUCE IT ON MY TRY PUSHING?????

f86254858e1e47171e2323dc1afde7feab5fc0a3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
rewrite test-agent-bootstrap in javascript.

https://github.com/mozilla-b2g/gaia/commit/dfb58e4ff9026666e232c5081dc63636c240905f
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Note that there is some typos in test-agent-bootstrap.js
  ingetegrationDir -> integrationDir
very sorry but had to revert again, seems it was running into problems with test-agent-bootstrap like https://tbpl.mozilla.org/php/getParsedLog.php?id=36799771&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
landed.

https://github.com/mozilla-b2g/gaia/commit/dd8317e07e1b6eda07b997686559ebcda99f7bea
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
So, this fails for me with:

Makefile:515: *** multiple target patterns.  Stop.
I have no way to debug this... so if you have any idea I'm very open to hear them. In the mean time I'm reverting locally the patch.
I think my problem comes from the fact that I have accuweather in my external-apps directory, from [1]

see how it has a "cache/http:" directory (which I think is expected for the apps that will preload an app cache). You should escape ":" characters because they're invalid as target prerequisites.

[1] https://github.com/telefonicaid/firefoxos-gaia-latam/tree/master/external-apps/accuweather/
Depends on: 989353
Filed Bug 989353 with a patch there.
Depends on: 989306
Sorry to bring bad news, but this was backed out. The reason that this was backed out was due to bug 989306. The commit in bug 885545 could not be backed out without backing out this one, and also I believe there was some issue with the CSS Linter.

To reland please ensure that the following works:
1 - Launching FTU with PRODUCTION=1 make reset-gaia
2 - CSS linting of files.

https://github.com/mozilla-b2g/gaia/commit/8ad27d2f7d8b26d5d7551f232ce4a3650bcc1503
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 938896
Please also ask for review before retrying to land.
We have to spend some more time testing this patch.
Gregor asked this bug to be landed on early morning in Taipei hours next time, so development of Pacific timezone won't be blocked by tree closure should that happen again.

Thanks.
(In reply to Kevin Grandon :kgrandon from comment #71)
> Sorry to bring bad news, but this was backed out. The reason that this was
> backed out was due to bug 989306. The commit in bug 885545 could not be
> backed out without backing out this one, and also I believe there was some
> issue with the CSS Linter.
> 
> To reland please ensure that the following works:
> 1 - Launching FTU with PRODUCTION=1 make reset-gaia
> 2 - CSS linting of files.

More info about this: the hook running the CSS linter uses the "require" shim that's removed by this patch. As a result, we couldn't commit CSS anymore ;)

2 solutions:
* keep the "require" shim
* rewrite the hook to use node instead of xpcshell

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #73)
> Gregor asked this bug to be landed on early morning in Taipei hours next
> time, so development of Pacific timezone won't be blocked by tree closure
> should that happen again.

Yeah, well, there are developers all around the globe anyway.


Thanks!
Attached patch colon.patch (deleted) — — Splinter Review 
Hi Alex & Julien, 

I found Makefile can't handle colon for a rule, so I write a workaround to clean dependency if there is a colon in filename and use .PHONY to enfore executing rule.

what do your guys think? do we have better solution?
Attachment #8399251 - Flags: feedback?(poirot.alex)
Attachment #8399251 - Flags: feedback?(felash)
Yuren, why the approach that I took in Bug 989353 doesn't work for you?

Filenames with colons are valid filenames (and as a matter of fact we use this for preload appcaches !), so I don't understand your patch ;)
Julien, it doesn't work for me because makefile can't find the directory with ":":

make: *** No rule to make target `/Users/yurenju/Documents/workspace/gaia/external-apps/accuweather/cache/http\:', needed by `/Users/yurenju/Documents/workspace/gaia/build_stage/accuweather'.  Stop.

that error occured on my mac
Do you know if you use GNU make or BSD make here? ("make --version" will tell you)

It works fine for me here on Linux :(

If you put that file in .PHONY, then the makefile rules for this app will always be executed. Which can be ok for now, it's still better than what we have now. If we can resolve this on Mac I'd be happier but this can be for another patch. Then please file a new bug for this issue only and add a comment on your code pointing to this bug.
Attachment #8399251 - Flags: feedback?(poirot.alex)
Attachment #8399251 - Flags: feedback?(felash)
this pull request is updated and solve different issue for each commit

Alex, could you review it? thank you.

Julien & Kevin, I also set feedback flag to you, could you take a look? and it would be great if you can test it with your daily workflow.

Thank you guys.
Attachment #8399315 - Flags: review?(poirot.alex)
Attachment #8399315 - Flags: feedback?(kgrandon)
Attachment #8399315 - Flags: feedback?(felash)
Comment on attachment 8399315 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/17802

Yuren, I tried to do:

  make
  touch apps/sms/js/link_helper.js
  make build_stage/sms

If I'm not wrong, it's supposed to rebuild the target build_stage/sms, right ? Well, it's not doing it... I also tried to edit the file and verified it was not copying the new file.

feedback- because of this but I may have missed something.

I also left comments on github.
Attachment #8399315 - Flags: feedback?(felash) → feedback-
Comment on attachment 8399315 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/17802

I didn't do in-depth testing, but some basic workflows appeared to work for me. I think Julien has this covered though. Thanks for checking with me!
Attachment #8399315 - Flags: feedback?(kgrandon)
Julien, it worked without last commit for colon, I'm investigating.

Thank you guys!
Comment on attachment 8399315 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/17802

Hi Julien, I fixed issues which you mentioned, could you take a look again? thanks!

BTW, because we use [[ to use ~= in if condition, but other two if condition without ~= were replaced by [.
Attachment #8399315 - Flags: feedback- → feedback?(felash)
Comment on attachment 8399315 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/17802

I tested many many things and everything looks good.
I tested various reset-gaia, incremental build with APP=foo make.
I had some weird behavior where make reset-gaia ended up with black screen,
but I'm no longer able to reproduce. (Can you give it a try, PRODUCTION=1 make reset-gaia?)

Re-reviewed closely root Makefile modification and critical apps modifications.
Attachment #8399315 - Flags: review?(poirot.alex) → review+
About the next step and incremental app build. I've seen a more elegant way of writing the $(eval xxx) thing:
makefile_fragment:
  program-that-emits-makefile-fragment > $@

include makefile_fragment

The benefit of that, is that if we have such "program-that-emits-makefile-fragment" we can run it to see its output and help debugging Makefile exceptions.
will test the latest version today
(In reply to Alexandre Poirot (:ochameau) from comment #87)
> About the next step and incremental app build. I've seen a more elegant way
> of writing the $(eval xxx) thing:
> makefile_fragment:
>   program-that-emits-makefile-fragment > $@
> 
> include makefile_fragment
> 
> The benefit of that, is that if we have such
> "program-that-emits-makefile-fragment" we can run it to see its output and
> help debugging Makefile exceptions.

Oh right, this seems powerful :) But you need to give prerequisites to "makefile_fragment" or it won't be remade.

see http://www.gnu.org/software/make/manual/make.html#Automatic-Prerequisites and http://www.gnu.org/software/make/manual/make.html#Remaking-Makefiles for all necessary information, I didn't know make could remake makefile :)
Comment on attachment 8399315 [details]
github PR: https://github.com/mozilla-b2g/gaia/pull/17802

Did basic tests but this seems to work for me, thanks !
Attachment #8399315 - Flags: feedback?(felash) → feedback+
passed on travis
https://travis-ci.org/mozilla-b2g/gaia/builds/22416208

and tested it on Linux, Windows & Mac.

ready to land.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S5 (11apr)
Flags: needinfo?(dmarcos)
Just a quick question: is there a reason this bug removed the .gitignores in subdirectories? This change is adding a lot of build-generated files to the untracked files list in git for media apps (we concatenate some of our files during build).

We should probably add these .gitignores back *somewhere*, and in the app's directory seems like the best place as far as encapsulation goes, but I wanted to make sure this wasn't verboten for some reason.
Yeah, we need these back. I was wondering why these files started appearing. Thanks for bringing this up Jim.
it would be better if we can put that concated file in build_stag, I'm discussing this on IRC with Jim, let's have a quick fix.
I don't see these anymore, so I think I was getting hit by swapping around between old branches and building. Sorry for the noise! (I really wish there were a way to clean up dead files like this automatically...)
(In reply to Jim Porter (:squib) from comment #97)
> I don't see these anymore, so I think I was getting hit by swapping around
> between old branches and building. Sorry for the noise! (I really wish there
> were a way to clean up dead files like this automatically...)

`git clean -f` ? :)
(In reply to Julien Wajsberg [:julienw] from comment #98)
> `git clean -f` ? :)

Neat!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: