Closed
Bug 1029385
Opened 9 years ago
Closed 9 years ago
Rewrite app-makefile rule in javascript
Categories
(Firefox OS Graveyard :: Gaia::Build, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S2 (15aug)
People
(Reporter: yurenju, Assigned: yurenju)
References
Details
(Whiteboard: [p=5])
Attachments
(1 file)
some apps such as keyboard, email, clcok... etc use r.js or other build process to copy files from source directory to build_stage. we should be explicit which app we want to copy in its own build script to avoid parent build script copy it again. for now, we copy from gaia build script if an app does not have Makefile but it's so implicit. and my idea is add "copy" field to metadata.json and we can set it to false if we don't want gaia build script copy files for certain app, for this proposal we need to rewrite app-makefiles in javascript, that will have some pros and cons: pros: 1. we will have more js build script and less Makefile 2. copy process will be implemented in js, I think it will be fast on Windows. (will confirm it) 3. we will have pre-app, app-builder (or another better naming) and post-app and after we rewrite other Makefiles in javascript, we can try to re-enable gaia-build extension again. con: 1. we will lose feature for parallel building app. Alex & Tim, what do you think?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(timdream)
Flags: needinfo?(poirot.alex)
Comment 1•9 years ago
|
||
Sounds good, and I think parallel building apps can be fixed by running JS in multiple threads at the same time. I was afraid that as an API, having a |copy| property in metadata.json is too verbose and prone to error (e.g. if an app copy to false but has no build.js or did not copy itself correctly), but Yuren convince me that this can be avoid by doing some checks after the step. He also said there is no use case to split app build hooks into two "copy" and "build" steps -- app either copy & build itself or just build the copied files in build.js.
Flags: needinfo?(timdream)
Comment 2•9 years ago
|
||
I don't have a really clear picture of your proposal but it is about doing something around existing app makefiles, I'm not sure it is worth spending time on that. I think it is better to work on converting them into js build scripts. In parrallel we can pull out of app-makefiles this code: echo "copy $(1) to build_stage/" ; \ cp -LR "$(2)" $(STAGE_DIR) && \ if [ -r "$(2)$(SEP)build$(SEP)build.js" ]; then \ echo "execute $(1)/build/build.js"; \ export APP_DIR=$(2); \ $(call run-js-command,app/build); \ fi; \ And instead, invoke a single JS script that would call all app build.js and do the copy. It should be faster as we are going to call xpcshell only once instead of once per app having build.js script. Once we have such script, we can finally address bug 969215 (we should really do something about that one!), with something like what I suggested in bug 1015868 comment 2. At the end what I'm suggesting here may just be what you suggested as it matches the pros/cons. The only thing is that I wouldn't care about apps that are still using Makefile, instead I would let them as-is, until we manage to convert them to build.js only. In the meantime, we would improve the experience with all other apps!
Assignee | ||
Comment 3•9 years ago
|
||
the reason I filed this bug is I found we use Makefile which only have few lines in keyboard app to avoid copying files from gaia Makefile, so if we remove Makefile in keyboard, we will need to fix this issue to avoid copying files from gaia Makefile. but I also agree we should implement it in javascript, so first we can rewrite all remain Makefiles in javascript for each app except Keyboard app then we can fix this bug without any makefile code.
Updated•9 years ago
|
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 4•9 years ago
|
||
we also can provide a function |copyToStage(options)| in utils.js and we don't need use use a configuration file for copy rule.
Assignee | ||
Comment 6•9 years ago
|
||
change title to majar task we will do on this bug.
Status: NEW → ASSIGNED
Summary: Explicit copy rule when building app → Rewrite app-makefile rule in javascript
Assignee | ||
Comment 7•9 years ago
|
||
got segfault on xulrunner-30a1 but works well on xulrunner-30, upgrading to xulrunner-33 on bug 1034565 may fix this issue.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → yurenju.mozilla
Assignee | ||
Updated•9 years ago
|
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S2 (15aug)
Assignee | ||
Comment 8•9 years ago
|
||
pull request: https://github.com/mozilla-b2g/gaia/pull/22723 try server: https://tbpl.mozilla.org/?rev=d046d1db76f52bc2e8c5cc033376089a78accb41&tree=Gaia-Try
Assignee | ||
Comment 9•9 years ago
|
||
build time measurement this pull request: 36.862s master: 48.362s :-o
Assignee | ||
Comment 10•9 years ago
|
||
Alex, could you review this pull request? I will also add a test case for app.js and we should file another bug for migrating pre-app, app and post-app.
Attachment #8471477 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 11•9 years ago
|
||
unit test cases has been added into pull request.
Assignee | ||
Comment 12•9 years ago
|
||
https://tbpl.mozilla.org/?rev=ae73138323a57eeb9dc72c8e55ab66875df140f3&tree=Gaia-Try
Comment 13•9 years ago
|
||
Comment on attachment 8471477 [details] [review] github PR: https://github.com/mozilla-b2g/gaia/pull/22774 The patchs looks good, but I have some doubt about camera. We still have 3 apps with a Makefile: camera, keyboard and calendar. It looks like keyboard and calendar Makefile are just some alias to build.js, so that's ok. But it looks like camera is doing more than just an alias, it really looks like a copy of build.js but in Makefile... We should get rid of it before proceeding with this patch. (Keeping an alias is fine, keeping a duplicated implementation isn't) We would need to ask a camera owner for review to ensure they stop adding logic to Makefile. For me it stripped down build time from 43s to 37s. -15% \o/
Attachment #8471477 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 14•9 years ago
|
||
Alex, I have discussed this with camera app peer/owner on bug 1028085 and email, they have a plan to move camera app to another repository outside gaia, so they want to have an standalone build process. Makefile of camera app and build.js basically do the same thing, but after landing this bug, we no longer execute Makefile of camera app in gaia build process, that file will only use for camera app team and it will be completely rewrote accroding bug 1028085 comment 4. so I would like keep Makefile there.
Flags: needinfo?(poirot.alex)
Comment 15•9 years ago
|
||
I disagree with the choice made over bug 1028085 comment 5: > having two ways for same purpose will take extra effort to maintain them, > but it can help us solve this conflict now. That's only reasonable if temporary. Otherwise it is the best way to be in trouble where the app is going to have a different behavior when built individually or via gaia build system. If camera app want to diverge and have its own build system, not based on build.js scripts, then, we should keep calling its Makefile and warn the developers that by choosing this path, their app won't be part of the firefox addon.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 16•9 years ago
|
||
djf & Diego, we are discussing camera app build system here. in this bug we will rewrite app-makefiles rule from Makefile to Javascript, that means we will use |require(APP_BUILD_JS).execute()| from commonjs to execute build.js for each app and this is a blocker for building gaia in firefox addon on bug 1022516 but for camera app we have two ways for building app: via Makefile or camera/build/build.js. Alex have concern that would be a trobule if we have two ways to build certain app. So I would like to know if you still have plan to use Makefile for building camera app currently we provide build.js[1] which do 100% same thing as camera/Makefile such as using r.js to optimize and copy file to certain place but it's a javascript implementation. if you still do have plan for using makefile to build camera app, we will keep calling |make| to execute camera/Makefile, but it also means camera app will not be available in firefox addon since we don't assume user have make command in their computer. if not -- that would be great, we will remove camera/Makefile and execute build.js in camera to build it. what do you think? can you share your plan for camera build system? [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/build/build.js
Flags: needinfo?(dmarcos)
Flags: needinfo?(dflanagan)
Comment 18•9 years ago
|
||
(In reply to Diego Marcos [:dmarcos] from comment #17) > I'm fine with moving the camera to use build.js Great! Thanks. (In reply to Yuren [:yurenju] from comment #16) > if not -- that would be great, we will remove camera/Makefile and execute > build.js in camera to build it. Note that you will still be able to have a Makefile if you want to ease building it from an external repo. But this makefile should just bootstrap necessary environment to call the build.js script. The issue we have today is that the Makefile is a duplicated implementation of build.js in bash/makefile. The important point is about keeping the build logic in build.js scripts.
Assignee | ||
Comment 19•9 years ago
|
||
Great! thank you, Diego. I will update my pull request and request a review again.
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8471477 [details] [review] github PR: https://github.com/mozilla-b2g/gaia/pull/22774 Alex, could you review this pull request again? I just changed two things: 1. remove camera/Makefile 2. remove |cp| command in calendar/Makefile
Attachment #8471477 -
Flags: review?(poirot.alex)
Comment 21•9 years ago
|
||
Comment on attachment 8471477 [details] [review] github PR: https://github.com/mozilla-b2g/gaia/pull/22774 I forgot to mention this feature miss about BUILD_APP_NAME in previous review. The patch looks good, could you just fix this regexp thing? Thanks!
Attachment #8471477 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 22•9 years ago
|
||
I will fix it in app.js however using regular expression in APP variable does not work for whole build process since we broke it when build system was refactoring. I would like to fix it on another bug to avoid regression.
Assignee | ||
Comment 23•9 years ago
|
||
bug 1054958 is filed for regular expression support for whole build system.
Assignee | ||
Comment 24•9 years ago
|
||
https://tbpl.mozilla.org/?rev=fb7a1ecaedea056bec7d07e544c59fea3eb9f85f&tree=Gaia-Try
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8471477 [details] [review] github PR: https://github.com/mozilla-b2g/gaia/pull/22774 Alex, your comments has been addressed, please review again!
Attachment #8471477 -
Flags: review?(poirot.alex)
Comment 26•9 years ago
|
||
Comment on attachment 8471477 [details] [review] github PR: https://github.com/mozilla-b2g/gaia/pull/22774 Thanks for addressing my numerous review comments ;)
Attachment #8471477 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 27•9 years ago
|
||
try for windows build: https://tbpl.mozilla.org/?tree=Try&rev=da0960c6ca2a
Assignee | ||
Comment 28•9 years ago
|
||
merged. https://github.com/mozilla-b2g/gaia/commit/1c961814d754a0af16f66d57c9c2b8127f9f64b3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 29•9 years ago
|
||
b2g-inbound: https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=a9280dbb21b8
Comment 30•9 years ago
|
||
Reverted for gaia-unit timeouts in wappush_test.js: https://tbpl.mozilla.org/php/getParsedLog.php?id=46239898&tree=B2g-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=46241401&tree=B2g-Inbound These failures were present in the gaia-try run: https://tbpl.mozilla.org/?rev=5e180187bcc2e5c75f76702817176bea1f8c0470&tree=Gaia-Try https://github.com/mozilla-b2g/gaia/commit/458391846629ccced87d10fc4d0584b3010f7890
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 31•9 years ago
|
||
it should be unrelated with my commit, I'll investigate it.
Assignee | ||
Comment 32•9 years ago
|
||
just compaure all content of wappush zip file and those are same as previous.
Assignee | ||
Comment 33•9 years ago
|
||
send a new pull request. https://tbpl.mozilla.org/?rev=0603acaa5cb3143df1789cdae299452453e79005&tree=Gaia-Try
Assignee | ||
Comment 34•9 years ago
|
||
ths cause is xpcshell-common.js is included in httpd extension but we don't have BUILD_CONFIG there so httpd extension crashed.
Assignee | ||
Comment 35•9 years ago
|
||
apply patch and trigger gaia try again. https://tbpl.mozilla.org/?rev=fa0ad5819605897a30557ec8d229ce708501f131&tree=Gaia-Try
Assignee | ||
Comment 36•9 years ago
|
||
and I will read try server report more carefully, thanks, Ed.
Assignee | ||
Comment 37•9 years ago
|
||
all looks good, we got some intermittent issue for Gb that will be fixed on another bug. https://tbpl.mozilla.org/?rev=a127b4e28b278cf4e0c7b328002451ca05419257&tree=Gaia-Try
Assignee | ||
Comment 38•9 years ago
|
||
merged. https://github.com/mozilla-b2g/gaia/commit/7855a89bc5e17ecc62e377b1bb7aab83ce881bc3
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 39•9 years ago
|
||
b2g-inbound: https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=a7fc2b98b19e
Comment 40•9 years ago
|
||
Sorry, I had to revert this for breaking our CSSLint inside the pre-commit hook. https://github.com/mozilla-b2g/gaia/commit/e1ff29a282de919c5e4f01845bed35d782954747 With this change, https://github.com/mozilla-b2g/gaia/blob/e1ff29a282de919c5e4f01845bed35d782954747/tools/pre-commit#L100-L102 is failing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 41•9 years ago
|
||
thanks, I'll fix it.
Assignee | ||
Comment 42•9 years ago
|
||
the cause is we require csslint in pre-commit but options.GAIA_APPDIRS does not exist, so add an if condition |if (options && options.GAIA_APPDIRS)| can solve this issue. I also grep all files which are using xpcshell-commonjs.js: * build/common.mk: for run-js-command, it would be fine since we use a lot in build tests. * tools/pre-commit: for csslint, it's fine if add the if condition * tools/extensions/http/bootstrap.js: for getting |require| function for commonjs pattern. I have checked all of them so it should be fine now.
Assignee | ||
Comment 43•9 years ago
|
||
https://tbpl.mozilla.org/?rev=51901c73d6cb4b6404f20e73d098a1b68298f897&tree=Gaia-Try
Assignee | ||
Comment 44•9 years ago
|
||
merged again. https://github.com/mozilla-b2g/gaia/commit/a6fb70c187c798736091d2279c29f9c091681c32
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•