Closed
Bug 1154582
Opened 10 years ago
Closed 9 years ago
Build system configure - adding configure.js to generate strong dependency all.mk and execute it
Categories
(Firefox OS Graveyard :: Gaia::Build, defect)
Firefox OS Graveyard
Gaia::Build
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rickychien, Assigned: gduan)
References
Details
Attachments
(2 files, 1 obsolete file)
Depend on our new build system plan [1]. Milestone 1 has to implement configure.js to tackle with build definition and offer a task dependency convention to generate a proper Makefile dynamically. [1] https://wiki.mozilla.org/Gaia/Build/NewBuildSystem
Reporter | ||
Updated•10 years ago
|
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8593245 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Hi Tim, Ricky and Greg configure.js works now. I've pushed to try server and it seems all green. As you might see, new build system doesn't have ability to do incremental build, so I skip two related build tests for now. Please kindly advise on this patch for anything you think of. Thanks.
Attachment #8593245 -
Flags: feedback?(timdream)
Attachment #8593245 -
Flags: feedback?(ricky060709)
Attachment #8593245 -
Flags: feedback?(gweng)
Comment 3•10 years ago
|
||
Comment on attachment 8593245 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master The most difficult part is it lacks comments about each function and the overall intentions of the config. This hinders me to look into the semantics, rather than shallow syntax issues.
Attachment #8593245 -
Flags: feedback?(gweng)
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Sorry,branches removed by accident .. set ni again.
Attachment #8596382 -
Flags: feedback?(timdream)
Attachment #8596382 -
Flags: feedback?(ricky060709)
Attachment #8596382 -
Flags: feedback?(gweng)
Assignee | ||
Updated•10 years ago
|
Attachment #8593245 -
Attachment is obsolete: true
Attachment #8593245 -
Flags: feedback?(timdream)
Attachment #8593245 -
Flags: feedback?(ricky060709)
Comment 6•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Discussed offline.
Attachment #8596382 -
Flags: feedback?(timdream)
Assignee | ||
Comment 7•10 years ago
|
||
As offline disscussed, items as below would be implemented in next commit, 1. common config and define can put it in common.mk/common.in (or some other names) and let other app/generated makefile to include. 2. generated app makefile should be put in build_stage/app's folder 3. let executeMakefile to 'make -c |target makefile|' instead of include them in Makefile 4. execute app's own makefile simply by 'make -c |app's own makefile|' instead of include them 5. if configure.js is changed, we should make sure config step would be rerun.
Assignee | ||
Comment 8•10 years ago
|
||
I worried about how to parse -jN and the flags( such as GAIA_OPTIMIZE=1 APP=xx) to sub-make. However, after some simple tests, I found parent make will automatically bring those variable to sub-make.
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master The main problem in this patch we might concern that would be how users customize build system to support new features. In my suggestion, I'd like to extract these PRE_APP_BUILD_STEPS and POST_APP_BUILD_STEPS modules out of configure.js so that we can put these logic in previous step and do not pollute configure phase. Please file a follow-up bug for this and set r? to me again once you fix all of these comments on Github.
Attachment #8596382 -
Flags: feedback?(ricky060709) → feedback+
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Hi Ricky, all comments are addressed, please kindly check again. Hi Tim, I've updated most issues we've discussed offline last time (also see comment 5). The only missing one is 5th item of comment 5, we'd like to let bug 1154583 to manage all the situations we need to re-generate build backend instead of treating it as a dependency in makefile. That would help us to remain the flexibility in the future with different form of backend. Besides that, we re-generate build backend every time when we execute make now.
Attachment #8596382 -
Flags: review?(ricky060709)
Attachment #8596382 -
Flags: feedback?(timdream)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Good job George! r=me. In my opinion, everything relates to configure should be moved into configure folder including configure.js and build-conig.in and remember to modify your comment consistently with this changes.
Attachment #8596382 -
Flags: review?(ricky060709) → review+
Comment 12•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Good work except some nits.
Attachment #8596382 -
Flags: feedback?(gweng) → feedback+
Updated•10 years ago
|
Attachment #8596382 -
Flags: review+
Attachment #8596382 -
Flags: feedback?(timdream)
Attachment #8596382 -
Flags: feedback+
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Hi Tim and Ricky, could you review this patch again? This patch has implemented items as below: 1. add configure step when ENABLE_CONFIGURE_STEP=1 2. configure step will generate Makefile to build_stage/ and build_stage/app 3. execute 'make -C build_stage' will generate profile 4. execute 'make -C build_stage/app' will generate profile/app 5. configure.js will trigger "make -C build_stage" by itself with '-j#', # is default to the number of CPU cores.
Attachment #8596382 -
Flags: review?(timdream)
Attachment #8596382 -
Flags: review?(ricky060709)
Reporter | ||
Comment 14•10 years ago
|
||
What I was concerned that is the accessibility of DependencyGraph API. As a developer, to create a configure for customizing build script must include 'require('./configure/dependency-graph')' and put insertTask, insertDep logics in their configure. 1. nit: we should put a checking step in insertTask for skipping duplicated target. 2. The order of insertTask would be pretty important since developers could feel confused about the target order of Makefile are reversed of invoking order. 3. It seems that the last one target will be treat as major dependencies of all target by default, I'd like to do this explicitly by myself. It could make our rule more clear and controllable. Here is my WIP branch for the use-case of webapp-shared refactoring: webapp-shared.js: https://github.com/RickyChien/gaia/blob/webapp-shared-configure/build/webapp-shared.js#L389-L412 webapp-shared.mk: https://github.com/RickyChien/gaia/blob/webapp-shared-configure/webapp-shared.mk all.mk: https://github.com/RickyChien/gaia/blob/webapp-shared-configure/all.mk
Reporter | ||
Comment 15•10 years ago
|
||
And another suggestion is 4. I prefer to put a all: target at the top of Makefile by default, does it make sense for you?
Reporter | ||
Comment 16•10 years ago
|
||
Additionally, we could even write all empty PHONY target such as all, install, clean...etc in Makefile by default so that developers could fill them if necessary.
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Thanks your good work! I've leaved some comments on Github and there are some of nits need to be fixed. I can see a checking of rerun Makefile was added in latest patch. However, I was still thinking it doesn't provide enough resources / checking parameters for deciding whether we're going to re-trigger configure or not. I'm fine to finish such feature in bug 1154583.
Attachment #8596382 -
Flags: review?(ricky060709) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Thanks, I just fixed below items, 1. rerun fail: recently Email app has include shared/elements so its buildscript will generate its own gaia_shared.json. Since the new build will not clear build_stage's data, so webapp-shared.js will adopt the gaia_shared.json and use it as a reference to copy file from share folder. It sounds correct, but the path which shared_utils wrote is wrong. So, I correct it and put it in my 2nd commit. 2. reorder the task order 3. use all to collect all necessary tasks 4. print out error if user want to execute build_stage's makefiles directly and it's modified.
Assignee | ||
Comment 20•9 years ago
|
||
In order to make sure below items, 1. incremental build with new build system will be faster than original one 2. successful refactoring SOP for current build module and app build 3. fixed architectural (not much different than current commit ) please refer to https://wiki.mozilla.org/Gaia/Build/NewBuildSystem newbuild will work on my personal branch https://github.com/cctuan/gaia/tree/config-build until we can make sure above three items works.
Comment 21•9 years ago
|
||
Comment on attachment 8596382 [details] [review] [gaia] cctuan:1154582 > mozilla-b2g:master Do we still need to land this?
Flags: needinfo?(gduan)
Attachment #8596382 -
Flags: review?(timdream)
Attachment #8596382 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
No, we won't land it to master alone. New build will affect master after we finish all the requirements of bug 1168287. Before that, we'll still work on this branch https://github.com/cctuan/gaia/tree/config-build .
Flags: needinfo?(gduan)
Reporter | ||
Comment 23•9 years ago
|
||
We will land this in master: * If evaluation of new architecture looks great. (Evaluation will target on webapp-shared, webapp-optimize and multi locale modules) * Transition of splitting configuring and building step for all of modules are finished And now, we've to close this bug since we've already achieved requirements.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 24•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•