Closed Bug 969215 Opened 6 years ago Closed 5 years ago

execute |make| should only build specific apps we modified

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yurenju, Assigned: rickychien)

References

Details

(Whiteboard: [gaia-build])

Attachments

(2 files, 2 obsolete files)

46 bytes, text/x-github-pull-request
gduan
: review+
ochameau
: review+
Details | Review
56 bytes, text/x-github-pull-request
Details | Review
Ideally we would do "make" and the build system is only going to rebuild the app we modified.
Summary: execute |make| should only build specific apps whcih we modified → execute |make| should only build specific apps we modified
Blocks: 973512
for copy rule, make will detect which apps were modified on bug 897352
isolate webapp-optimize step for each app, so we can depend on certain apps

WIP here:
https://github.com/yurenju/gaia/commit/6fce889288f781d1391e4965962bba8c38ced063
Whiteboard: [gaia-build]
Component: Gaia → Gaia::Build
Assignee: nobody → yurenju.mozilla
Alex,

I made a WIP commit for it, I would love to have your feedback for it :)

https://github.com/yurenju/gaia/commit/dd255404a67c6b27917d1453e58ec00548ad91d2

this commit is based on bug 1053703 so I cherry pick commit of bug 1053703 to this branch.
Flags: needinfo?(poirot.alex)
I imagine you have seen it, but /home/alex/gaia/dev_apps/test-agent/config.json is always different.

file.getRelativeDescriptor() shouldn't be used for such purpose:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#454
Unfortunately there is no safe API around nsIFile to compute a relative path.
The only one API I know is the following:
  var path = require('sdk/fs/path');
  var relativePath = path.relative(file.path, dir.path);
(sdk/fs/path has the same API than nodejs path module)

I think this is a quite huge gain and we should move forward with this patch!
It strips down incremental build time from 25s to 11s,
and we should followup on why it takes 11s... it should be way faster than that.

Otherwise I've seen some exception about a variable "previous" being undefined,
it may happen when switching between revisions.
Flags: needinfo?(poirot.alex)
What is the status of your patch?
Do you think you will land it this week?
Flags: needinfo?(yurenju)
I'm blocking on bug 1053703, that should not be a complex issue but I get twice backout because try server bustage :-/

I will try my best but I don't give too much hope to land it in this week.
Flags: needinfo?(yurenju)
Take this because Yuren has leaved.
Assignee: yurenju → ricky060709
Status: NEW → ASSIGNED
Alexandre, do you have an experience about using path = require('sdk/fs/path')?

Though I'm sure there is exist sdk/fs/path.js in mozilla-central, I couldn't require it successfully no matter which b2g versions I took. It seems that b2g has different issues when I switched its version.
Flags: needinfo?(poirot.alex)
Otherwise as you mentioned on comment 5, I found there are two files:

1. apps/calendar/js/presets.js
2. dev_apps/test-agent/config.json

rebuild every time that lead to rebuild.js rebuild calendar and test-agent.

I want to add a filter in rebuild.js for skipping some auto-generated files but still have no better idea about how to do that. Ideas are welcome. :)
I'm not sure that it's no problem with skipping 'apps/calendar/js/presets.js' when rebuild gaia, but I can file another bug for test-agent to move 'dev_apps/test-agent/config.json' out of test-agent for skipping it when rebuild.
ni Gareth for comment 11.
Flags: needinfo?(gaye)
I think we could use an ignore list to fix this issue.
Thanks.
Flags: needinfo?(gaye)
Flags: needinfo?(poirot.alex)
Flags: needinfo?(poirot.alex)
(In reply to Ricky Chien [:rickychien] from comment #9)
> Alexandre, do you have an experience about using path =
> require('sdk/fs/path')?
> 
> Though I'm sure there is exist sdk/fs/path.js in mozilla-central, I couldn't
> require it successfully no matter which b2g versions I took. It seems that
> b2g has different issues when I switched its version.

Hum... That is unfortunate... I thought this sdk/fs/path module wasn't using any dependencies.
But I was wrong, it pull some other modules that end up loading an XPCOM that doesn't exists on b2g (@mozilla.org/xre/app-info;1)
Here is a hack to make it work:
  https://github.com/ochameau/gaia/commit/e456adf76fa2e4c6baf9741441a7e03ca92ad91e
Feel free to pull it in your own changeset.


(In reply to Ricky Chien [:rickychien] from comment #10)
> Otherwise as you mentioned on comment 5, I found there are two files:
> 
> 1. apps/calendar/js/presets.js
> 2. dev_apps/test-agent/config.json
> 
> rebuild every time that lead to rebuild.js rebuild calendar and test-agent.
> 
> I want to add a filter in rebuild.js for skipping some auto-generated files
> but still have no better idea about how to do that. Ideas are welcome. :)

We don't have to be perfect before landing this patch. I would prefer landing it sooner and rebuild some apps every time than delaying this patch even more and rebuild *all apps, always*!!

Otherwise, we could followup with a blacklist, but I think we shouldn't have any generated file in the source tree in first place!! All these files should live in the stage directory...
Flags: needinfo?(poirot.alex)
Thank you Alex! I'm appreciate your help.

>>>Here is a hack to make it work:
>>>  https://github.com/ochameau/gaia/commit/e456adf76fa2e4c6baf9741441a7e03ca92ad91e

This hack works properly for loading 'sdk/fs/path.js' but it stops post-app process without obvious errors. Moreover, I cannot understand this hack and figure out what's going on.

Another question is that I prefer file.getRelativeDescriptor() which works fine to me; however, I want to know why it is a bad idea for us?
Flags: needinfo?(poirot.alex)
Attached file Gaia PR (obsolete) —
Attachment #8505925 - Flags: feedback?(gduan)
Comment on attachment 8505925 [details] [review]
Gaia PR

Patch is coming!
At present, I removed Alex's hack and it work properly on my machine.
I'm going to fix those treeherder fails.
Attachment #8505925 - Flags: feedback?(gduan) → review?(gduan)
And blacklist feature has been added! Putting a new blacklist in build/config/build-config.json skips those auto-generated files so that we don't have to rebuild calendar and test-agent.
Our integration testing will check the behavior of rebuild through caching previousTimeStamp and currentTimeStamp to ensure someone create a new auto-generated file but forget to put it into blacklist.
(In reply to Ricky Chien [:rickychien] from comment #15)
> Thank you Alex! I'm appreciate your help.
> 
> >>>Here is a hack to make it work:
> >>>  https://github.com/ochameau/gaia/commit/e456adf76fa2e4c6baf9741441a7e03ca92ad91e
> 
> This hack works properly for loading 'sdk/fs/path.js' but it stops post-app
> process without obvious errors. Moreover, I cannot understand this hack and
> figure out what's going on.

I haven't been able to reproduce the error locally?
But I found why it is failing on try and can submit a fix for that.

> file.getRelativeDescriptor()

This function works by luck. It is not meant to be used as-is. Today it works, but the API explicitely says it may change for something else than a string. It can have weird behavior on some platform and may break if platform code changes.
Flags: needinfo?(poirot.alex)
Hi Alex,
I simply copy your build/xpcshell-commonjs.js to master and try to execute |make clean| + |make| and found out the same issue as mentioned in comment 15.

I guess there's something wrong when building settings app with your xpcshell-commonjs.js. Below is the error message when executing |make APP=settings| on my mac. Can't you reproduce it ?


[cmd] /usr/bin/git --git-dir=/Users/tuangeorge/code/gaia-hd/.git log -1 --format=%H%n%ct HEAD
Exception: TypeError: platformDefaults[gXulRuntime.OS.toLowerCase(...)] is undefined
getPlatformValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/system/child_process/subprocess.js:6:25
subprocess.call@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/system/child_process/subprocess.js:36:14
Commander/this.runWithSubprocess@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/build/utils-xpc.js:974:13
SettingsAppBuilder.prototype.writeGitCommit@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/apps/settings/build//build.js:164:5
SettingsAppBuilder.prototype.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/apps/settings/build//build.js:174:3
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/apps/settings/build//build.js:183:4
buildApps/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/build/app.js:30:9
buildApps@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/build/app.js:17:3
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/tuangeorge/code/gaia-hd/build/app.js:49:3
CommonjsRunner.prototype.run@/Users/tuangeorge/code/gaia-hd/build/xpcshell-commonjs.js:149:5
run@/Users/tuangeorge/code/gaia-hd/build/xpcshell-commonjs.js:164:3
@-e:1:1
Flags: needinfo?(poirot.alex)
I don't know why, but no, I'm not able to reproduce it on linux.
But I see what is wrong, here is a new changeset that should fix this exception:
  https://github.com/ochameau/gaia/commit/f4c61596ec4cba590a216ea8811859f4d9501496
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> I don't know why, but no, I'm not able to reproduce it on linux.
> But I see what is wrong, here is a new changeset that should fix this
> exception:
>  
> https://github.com/ochameau/gaia/commit/
> f4c61596ec4cba590a216ea8811859f4d9501496

Oops! new patch works fine in running |rebuild| but another fail arose in unit test.
Running keyboard unit tests crash firefox immediately with these errors:

-----------------------------------------------------------------------------
Full message: TypeError: toolDefinition is null
Full stack: Toolbox.prototype.fireCustomKey@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:555:1
gDevToolsBrowser.selectToolCommand/<@resource:///modules/devtools/gDevTools.jsm:642:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

*************************
TypeError: responsive is undefined: initResponsiveDesign/<@resource://gre/modules/addons/XPIProvider.jsm -> file:///Users/Ricky/Documents/gaia/profile-debug/extensions/browser-helper@gaiamobile.org/bootstrap.js:107:1
EventEmitter_emit@resource://gre/modules/devtools/event-emitter.js:137:11
RUI_unload@resource://app/modules/devtools/responsivedesign.jsm:318:5
-----------------------------------------------------------------------------
Flags: needinfo?(poirot.alex)
I'm not sure this error is related, as build/xpcshell-commonjs.js shouldn't be used in app unit tests.
It should only be used to build the profile. So it would most likely mean that the profile hasn't been correctly created.

But here is an tweaked version, more conservative:
  https://github.com/ochameau/gaia/commit/15b44485942d10457260768e1012c93fd019da6c
If that happen to fix the test, I would like to know why xpcshell-commonjs ends up being loaded in app tests!
Flags: needinfo?(poirot.alex)
Comment on attachment 8505925 [details] [review]
Gaia PR

Good job! I left some comments for integration tests. I think it's ok to merge once gaia-try and try (for different env) pass, and we also need to fix unit test as you mentioned in comment 22.


After it's merged, I think we should also add one more method of utils.gaia that will only generate modified app information. And let webapp-shared.js, webapp-optimise, web-zip to use it. That will save more time when rebuilding.
Attachment #8505925 - Flags: review?(gduan) → review+
Comment on attachment 8505925 [details] [review]
Gaia PR

As offline discussed, let's modify utils.gaia.instance according to rebuildWebapps array and apply to webapp-shared / webapp-optimize/ webapp-zip. Please set r? to me once finish!
Attachment #8505925 - Flags: review+
Comment on attachment 8505925 [details] [review]
Gaia PR

Hi George, PR has updated!
Attachment #8505925 - Flags: review?(gduan)
Here the rebuild time on my machine

before: 32.014s
after : 2.544s
Comment on attachment 8505925 [details] [review]
Gaia PR

32s -> 2s ! great job! ,r=gduan!
Attachment #8505925 - Flags: review?(gduan) → review+
Merged.

https://github.com/mozilla-b2g/gaia/commit/cc5b3e9bf8298ca042894d5c08b79606815025a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
sorry had to revert this change for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=684504&repo=b2g-inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please Ricky, watch try results carefully before merging, this failure was visible in the pull request run:
  https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9989aa028739
Attached file Gaia PR 2 (obsolete) —
Problem caught! 

b2g-inbound execute |make preference| and I forgot to put config.rebuildAppDirs = config.rebuildAppDirs || []; at [1].

Patch has updated!

[1] https://github.com/mozilla-b2g/gaia/pull/25433/files#diff-2b31def028024b968babe1d78830dfafR398
Comment on attachment 8510130 [details] [review]
Gaia PR 2

r=gduan, please land it once test complete and pass.

I think preferences.js doesn't need to get the whole gaia.getInstance object, it takes time to be generated. We can file a follow-up bug for it.
Attachment #8510130 - Flags: review?(gduan) → review+
Merged.

https://github.com/mozilla-b2g/gaia/commit/6fbcd67366f40d62d27afb7185ac62f14b9b74e9
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Ricky, this patch broke incremental builds by emptying profile/webapps.json :/
We have to back it out as it break the productivity of all gaia contributors...

Can you please provide a new patch and ensure that incremental build are working fine?
(Please do some "diff -r" on two profile to ensure that incremental are working correctly)
George, Ricky, please take some more looking at tests and verifying build system patches.
Such regression can kill the productivity of many developers. The backout rate of build system patches are way to high.
sorry had to revert this again for causing bug 1087967
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 1087967
Issue has found and patch has been updated!

However, I still have to provide a new test for ensuring incremental build.
Attached file Gaia PR
I shouldn't skipped the test for checking webapps.json in last patch.

Now I added an additional check in tests, comparing previous and current contents of profile/webapps/webapps.json for ensuring that such a matter won't happen again.
Attachment #8505925 - Attachment is obsolete: true
Attachment #8510130 - Attachment is obsolete: true
Attachment #8511481 - Flags: review?(gduan)
After discussing offline with George, there are other issues for rebuild with different commands like multilocale, customization and keyboard-layout (or even more). We don't have any tests for them to ensure that get an expected result.

Rebuild is a huge feature and the better way is to disable by default |make| and makes it as a separate make target such as |make rebuild|.
Flags: needinfo?(poirot.alex)
After discussing offline with Tim and George, we could save BUILD_CONFIG into timestramp.json to detect config changes. If these build config has been changed, we should skip rebuild process.
Flags: needinfo?(poirot.alex)
Unfortunately, we found a big problem in our rebuild feature.

We allow every app to maintain a own app/build/build.js for customized build. If someone puts |utils.getFile| to load external resources, we are unable to detect those changes so that build script won't rebuild again.

In such case, we can force rebuild these apps which contain their own build.js, and design a method for skipping rebuild processes if they are not loading those external resources and want to build faster.

Hi Alex, what is your suggestion?
Flags: needinfo?(poirot.alex)
I don't see why using getFile would break incremental builds?
Most of getFile refers to files in the app source directory or app stage directory.
If any file in app source directory is modified, it will rebuild the app and its stage directory, so it should work.
The only issue I can image is if the getFile refers to some files in shared/ or anything outside of apps directory... but it seems rare? At least I haven't seen any such usage easily?

Saving BUILD_CONFIG is the cache file sounds like a great idea! If you do, I think it would be worth using another more generic name for the cache file, like build.cache or something.

Otherwise it sounds perfectly fine landing a new target 'rebuild' and work in followups to make it better before switching the default rules to it!
Flags: needinfo?(poirot.alex)
Comment on attachment 8511481 [details] [review]
Gaia PR

please set r? once issues are addressed.
Attachment #8511481 - Flags: review?(gduan)
Comment on attachment 8511481 [details] [review]
Gaia PR

BUILD_CONFIG detection has been added.

For ensuring not to break our system, we detect BUILD_CONFIG through whether user change their |make ARG1=A ARG2=B| from last input, and detect specified ARGS such as GAIA_DISTRIBUTION_DIR, LOCALE_BASEDIR, LOCALES_FILE, GAIA_KEYBOARD_LAYOUTS and GAIA_DEV_PIXELS_PER_PX because of which loading external resources.

If there is any app that provides build.js, we're going to rebuild them for reducing the risks.
Attachment #8511481 - Flags: review?(gduan)
Update Gaia-node-modules in order to remove walk_dir.js and replace dive with diveSync since there are some issues in walk_dir.js caused fails when reading non-exist files.
If it's necessary, file another bug for it, set dependency and try to merge it first.
(In reply to Ricky Chien [:rickychien] from comment #49)
> Created attachment 8515559 [details] [review]
> Gaia-node-modules PR
Flags: needinfo?(ricky060709)
We can submit a follow-up bug to update gaia-node-modules.revision after landing both Gaia and Gaia-node-modules PRs.

I often do in this way in test-agent. :)
Flags: needinfo?(ricky060709)
Comment on attachment 8511481 [details] [review]
Gaia PR

r=gduan, 2nd make has been reduced from 46 to 36 seconds.

When execute 2nd |make| ,
1. when app's file has changed, it will only rebuild the app.
2. when app has its own build.js, it will be rebuilt.
   > let app decide to rebuild, since it might include external resource.
3. when any shared file has changed, it will rebuild all apps.
4. when build config has changed, it will rebuild all apps.
5. when assigning GAIA_DISTRIBUTION_DIR, LOCALE_BASEDIR, and LOCALES_FILE it will rebuild all apps.
   > these config might include external resource, so we rebuild all in case.

I'm not sure if there's any condition we haven't considered yet if we put it in default |make|.

Alex, could you also help to review this patch?
Thanks.
Attachment #8511481 - Flags: review?(poirot.alex)
Attachment #8511481 - Flags: review?(gduan)
Attachment #8511481 - Flags: review+
Comment on attachment 8511481 [details] [review]
Gaia PR

It looks mostly good, I made some comment to the pull request, you already fixed most of them.
But I'm seeing some app disappearing when doing incremental builds.
The one with the GUID name as folder name, like {16bfe813-6c56-4fad-8cf7-1b1a98a417cd}... (mochitest and in app payment apps).

They are correctly built the first time I run make, but the second time, they disappear from profile/webapps/.

This is related to bug 1020259. Some apps, have a random id which ends up being different each time you call make. It ends up introducing differences in webapps.json each time you call make. These apps have a new id, new folder and new entry in webapps.json. We may just want to fix bug 1020259, in order to fix this issue in your bug. Feel free to take over my patch...
Attachment #8511481 - Flags: review?(poirot.alex) → review-
Comment on attachment 8511481 [details] [review]
Gaia PR

Thank you alex!

I've updated patch but I haven't put your fix into it. I prefer to handle REPRODUCIBLE feature in separate bug and, moreover, I don't want to mess up every thing here.
Attachment #8511481 - Flags: review- → review?(poirot.alex)
Comment on attachment 8511481 [details] [review]
Gaia PR

Thanks, one coment and you are good to land!
Attachment #8511481 - Flags: review?(poirot.alex) → review+
Merged.

https://github.com/mozilla-b2g/gaia/commit/7004ccfd16e2ad20739bd04b72fa1672ee58686f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
$ rm -rf profile-mulet-prod && make PRODUCTION=1 MOZILLA_OFFICIAL=1 NOFTU=1 PROFILE_FOLDER=profile-mulet-prod profile-mulet-prod
Test SDK directory: /home/alex/codaz/Mozilla/b2g/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01
test -f /home/alex/codaz/Mozilla/b2g/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell
run-js-command gaia/app
System JS : ERROR file:///home/alex/codaz/Mozilla/b2g/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/components/nsHandlerService.js:120 - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
Exception: TypeError: previous is undefined
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/alex/codaz/Mozilla/b2g/gaia/build/rebuild.js:92:1
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/alex/codaz/Mozilla/b2g/gaia/build/app.js:44:30
CommonjsRunner.prototype.run@/home/alex/codaz/Mozilla/b2g/gaia/build/xpcshell-commonjs.js:160:5
run@/home/alex/codaz/Mozilla/b2g/gaia/build/xpcshell-commonjs.js:175:3
@-e:1:1
Revert.

https://github.com/mozilla-b2g/gaia/commit/98fc7a1800a550634f73c8a23d51b5bba1a3a3a2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ricky, I'm not sure it was needed to revert: I was just reporting my error. So far, it seems to be working after a |make clean really-clean|.
Do you modify anything in shared/?

I saw there is a bug I forgot to fix for preventing "previous is undefined".

I'm sure that was a bug. I'll make a quick fix and land it again.
Nope, I just had Gaia at master with your fix and the changes from https://github.com/mozilla-b2g/gaia/pull/25923.
Merge again.

https://github.com/mozilla-b2g/gaia/commit/1decc7ae607a7774b590192242a12502f614c84f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Guys, we really need to land gaia-node-module changes *first* before updating package.json in gaia. Please remember this for the future. I'm trying to sync the two now. 

Updated gaia-node-modules: https://github.com/mozilla-b2g/gaia-node-modules/commit/fea08545f9069af55d66edc764d967819161e75f

I'm also running the PR to update the node modules revision now: https://github.com/mozilla-b2g/gaia/pull/26056
Is this something that we should cover in the Firefox OS build documentation on MDN?
Sure! I'm going to do this.
You need to log in before you can comment on or make changes to this bug.