Closed Bug 1029385 Opened 6 years ago Closed 6 years ago

Rewrite app-makefile rule in javascript

Categories

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

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S2 (15aug)

People

(Reporter: yurenju, Assigned: yurenju)

References

Details

(Whiteboard: [p=5])

Attachments

(1 file)

github PR: https://github.com/mozilla-b2g/gaia/pull/22774
46 bytes, text/x-github-pull-request
ochameau
: review+
Details | Review 
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?
Flags: needinfo?(timdream)
Flags: needinfo?(poirot.alex)
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)
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!
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.
Flags: needinfo?(poirot.alex)
we also can provide a function |copyToStage(options)| in utils.js and we don't need use use a configuration file for copy rule.
Duplicate of this bug: 1035722
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
got segfault on xulrunner-30a1 but works well on xulrunner-30, upgrading to xulrunner-33 on bug 1034565 may fix this issue.
Depends on: 1034565
Assignee: nobody → yurenju.mozilla
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S2 (15aug)
build time measurement

this pull request: 36.862s
master: 48.362s

:-o
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)
unit test cases has been added into pull request.
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)
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)
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)
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)
I'm fine with moving the camera to use build.js
Flags: needinfo?(dmarcos)
(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.
Great! thank you, Diego.

I will update my pull request and request a review again.
Flags: needinfo?(dflanagan)
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 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)
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.
bug 1054958 is filed for regular expression support for whole build system.
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 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+
merged.

https://github.com/mozilla-b2g/gaia/commit/1c961814d754a0af16f66d57c9c2b8127f9f64b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
it should be unrelated with my commit, I'll investigate it.
just compaure all content of wappush zip file and those are same as previous.
ths cause is xpcshell-common.js is included in httpd extension but we don't have BUILD_CONFIG there so httpd extension crashed.
and I will read try server report more carefully, thanks, Ed.
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
merged.

https://github.com/mozilla-b2g/gaia/commit/7855a89bc5e17ecc62e377b1bb7aab83ce881bc3
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
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 → ---
thanks, I'll fix it.
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.
merged again.

https://github.com/mozilla-b2g/gaia/commit/a6fb70c187c798736091d2279c29f9c091681c32
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.