1154582 - Build system configure - adding configure.js to generate strong dependency all.mk and execute it

Status: RESOLVED FIXED

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

Description

[Ricky Chien [:rickychien] (inactive)]

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

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] cctuan:1154582 > mozilla-b2g:master

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

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.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] cctuan:1154582 > mozilla-b2g:master

Comment 5

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8596382 [details] [review]
[gaia] cctuan:1154582 > mozilla-b2g:master

Sorry,branches removed by accident ..
set ni again.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8596382 [details] [review]
[gaia] cctuan:1154582 > mozilla-b2g:master

Discussed offline.

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 8

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 11

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8596382 [details] [review]
[gaia] cctuan:1154582 > mozilla-b2g:master

Good work except some nits.

Comment 13

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

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

Comment 15

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 16

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 17

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 19

[(Inactive after June) George Duan [:gduan] [:喬智]]

5. able to run configure step in node.

Comment 20

[(Inactive after June) George Duan [:gduan] [:喬智]]

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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8596382 [details] [review]
[gaia] cctuan:1154582 > mozilla-b2g:master

Do we still need to land this?

Comment 22

[(Inactive after June) George Duan [:gduan] [:喬智]]

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 .

Comment 23

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] cctuan:config-build > mozilla-b2g:master