WebIDE should be able to build and push Gaia apps

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: paul, Assigned: ochameau)

Tracking

Trunk
Firefox 38
x86_64
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Depends on: 1023081
(Reporter)

Updated

4 years ago
Blocks: 1043937
Depends on: 1000993
No longer depends on: 1023081
Now that bug 1000993 landed, we would need to put package.json files in gaia app folders.
Assignee: nobody → poirot.alex
Created attachment 8544650 [details] [review]
pull request 27160

This pull request add all the package.json files to each gaia apps
and also ensure setting `origin` attribute in webapp manifests.
Created attachment 8545256 [details] [review]
pull request 27201 - set origin attribute in each app manifest

Ricky, this patch is necessary in order to tell WebIDE which origin to use when pushing the app. I cleaned up histocal code that was trying to help supporting Fx Nightly and also remove very old comments.
Attachment #8545256 - Flags: review?(ricky060709)
Comment on attachment 8545256 [details] [review]
pull request 27201 - set origin attribute in each app manifest

Patch looks fine. I'm curious in what's different between manifest.webapp and update.webapp. It looks like we didn't use update.webapp for a while right?
Attachment #8545256 - Flags: review?(ricky060709) → review+
Checking needed for pull request 27201, attachment 8545256 [details] [review].
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Updated

4 years ago
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.
Comment on attachment 8544650 [details] [review]
pull request 27160

Gaia owners,

This patch is going to allow you pushing your apps directly from WebIDE,
without having to go throught the command line and running $ APP=foo make install-gaia.
Also, it will work on both simulators and devices!

WebIDE just need these package.json files living in app folders in order to know which command to run to build the app and where to find the built package.

I'm asking review from every owners so that you learn about this file and agree putting such new file in your app!
Attachment #8544650 - Flags: review?(crdlc)
Comment on attachment 8544650 [details] [review]
pull request 27160

Gaia owners,

This patch is going to allow you pushing your apps directly from WebIDE,
without having to go throught the command line and running $ APP=foo make install-gaia.
Also, it will work on both simulators and devices!

WebIDE just need these package.json files living in app folders in order to know which command to run in order to build the app and where to find the built package.

I'm asking review from every owners so that you learn about this file and agree putting such new file in your app!
Attachment #8544650 - Flags: review?(timdream)
Attachment #8544650 - Flags: review?(sfoster)
Attachment #8544650 - Flags: review?(schung)
Attachment #8544650 - Flags: review?(m)
Attachment #8544650 - Flags: review?(jrburke)
Attachment #8544650 - Flags: review?(guilherme.p.gonc+bmo)
Attachment #8544650 - Flags: review?(gaye)
Attachment #8544650 - Flags: review?(francisco)
Attachment #8544650 - Flags: review?(felash)
Attachment #8544650 - Flags: review?(drs.bugzilla)
Attachment #8544650 - Flags: review?(dkuo)
Attachment #8544650 - Flags: review?(dflanagan)
Attachment #8544650 - Flags: review?(arthur.chen)
Can WebIDE take the zip instead of build stage dir? There are still difference between the content of build_stage and the actual files in the zip.

https://github.com/mozilla-b2g/gaia/blob/155461a49/build/webapp-zip.js#L130-L172

If you read webapp-zip.js you will find it still do final changes to the files. I know they should all have moved to other steps and changes build_stage directly (Ricky?) but I know at least the |getCompression| part is something only nsIZipWriter can do explicitly (see bug 959047).

Other than that I don't see any problem with the approach here.
Flags: needinfo?(ricky060709)
Comment on attachment 8544650 [details] [review]
pull request 27160

LGTM
Attachment #8544650 - Flags: review?(sfoster) → review+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> Can WebIDE take the zip instead of build stage dir? There are still
> difference between the content of build_stage and the actual files in the
> zip.

Not today, but that's something we can add. It's not easy as WebIDE expect sources unzipped for a validation step where we do various sanity checks before pushing the app and it will be harder doing it from a zip file or we would have to unzip it again :o I'll get back to you about that.
Attachment #8544650 - Flags: review?(guilherme.p.gonc+bmo) → review+
Attachment #8544650 - Flags: review?(m) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

Looks good to me. I didn't try testing it, though.

I suspect that there really shouldn't be a package.json file for apps/sharedtest however, since that directory exists only to host tests. There isn't really an app there.
Attachment #8544650 - Flags: review?(dflanagan) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

r+ in the email app.
Attachment #8544650 - Flags: review?(jrburke) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

For Dialer and Callscreen.
Attachment #8544650 - Flags: review?(drs.bugzilla) → review+
(In reply to David Flanagan [:djf] from comment #13)
> I suspect that there really shouldn't be a package.json file for
> apps/sharedtest however, since that directory exists only to host tests.
> There isn't really an app there.

Ok, I'll remove it!
Comment on attachment 8544650 [details] [review]
pull request 27160

r=me for settings app.
Attachment #8544650 - Flags: review?(arthur.chen) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

Fine for the network alerts. Just wondering if this part is necessary or removable for production build?
Attachment #8544650 - Flags: review?(schung) → review+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> If you read webapp-zip.js you will find it still do final changes to the
> files. I know they should all have moved to other steps and changes
> build_stage directly (Ricky?)

Patch has problem since build_stage is not currently in final stage. (but it should be)
After finishing build_stage, our workflow continually execute webapp-zip to archive files.
It calls isExcludedFromZip() [1] in order to exclude unnecessary files and adds new l10n files [2] into zip.
That means the result of build_stage is different with application.zip, so there still remain some further works before landing this patch.

[1] https://github.com/mozilla-b2g/gaia/blob/155461a49/build/webapp-zip.js#L49-L128
[2] https://github.com/mozilla-b2g/gaia/blob/155461a49/build/webapp-zip.js#L149-L172
Flags: needinfo?(ricky060709) → needinfo?(poirot.alex)
I don't really worry about extra files being included in WebIDE generated zip -- it certainly can be improved but that does not block this bug. The l10n part [3] is especially worrying -- it looks like we will be replace the HTML with the ones got pre-translated -- that means pre-translation will not be picked up by WebIDE.

Should we consider that as a blocker to this feature here? Gandalf? Stas?

[3] https://github.com/mozilla-b2g/gaia/blob/155461a49/build/webapp-zip.js#L156-L157

BTW, Ricky, could you file bugs on all these? Things don't happen automagically with a "it should be" comment :)
Flags: needinfo?(stas)
Flags: needinfo?(ricky060709)
Flags: needinfo?(gandalf)
Comment on attachment 8544650 [details] [review]
pull request 27160

Patch looks good assuming there isn't any considerable impact as stated above. Thanks for working on this!
Attachment #8544650 - Flags: review?(timdream) → review+
Bug was filed! See bug 1125031.
Flags: needinfo?(ricky060709)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> I don't really worry about extra files being included in WebIDE generated
> zip -- it certainly can be improved but that does not block this bug. The
> l10n part [3] is especially worrying -- it looks like we will be replace the
> HTML with the ones got pre-translated -- that means pre-translation will not
> be picked up by WebIDE.

You are correct. Without that, we will not have a pretranslated file so we'll be testing non-default locale scenario even for default locale.

In the worst case, you will develop experiencing the startup performance that every non-default locale user does experience. The perf hit currently is minimal to non-existing, so I would say it's not a blocker.
Flags: needinfo?(gandalf)
Attachment #8544650 - Flags: review?(crdlc) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

r=me for the added file.

Note that I'll likely not use this much, but because I have some existing habits using custom tools that work for me, not because this is useless ;) I think this could especially be useful for new contributors that don't like the command line much.
Attachment #8544650 - Flags: review?(felash) → review+

Comment 25

4 years ago
Comment on attachment 8544650 [details] [review]
pull request 27160

The music part looks good to me, thanks!
Attachment #8544650 - Flags: review?(dkuo) → review+
Flags: needinfo?(poirot.alex)
Attachment #8545256 - Flags: review+
Keywords: checkin-needed, leave-open
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Updated

4 years ago
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.
Attachment #8544650 - Flags: review?(francisco) → review+
Attachment #8544650 - Flags: review?(ricky060709)
Ricky, I tweaked this patch in order to blacklist package.json files from zip files, according to comment 18 from Steve.
Flags: needinfo?(stas)
Comment on attachment 8544650 [details] [review]
pull request 27160

+1 :)
Attachment #8544650 - Flags: review?(gaye) → review+
Comment on attachment 8544650 [details] [review]
pull request 27160

I leaved a comment to fix regexp. It seems no other problems to me.
Attachment #8544650 - Flags: review?(ricky060709) → review+
Landed:
https://github.com/mozilla-b2g/gaia/commit/80dc3ce34b016592c69fec85d2b22b5bb98f82c5
https://github.com/mozilla-b2g/gaia/commit/17015d5005998f4e03634f30951620f2360a60e4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Keywords: leave-open
Oops! I found there shouldn't create a browser/package.json since browser app has been removed, right?

https://github.com/RickyChien/gaia/tree/master/apps/browser
Flags: needinfo?(poirot.alex)
Created attachment 8556402 [details] [review]
Pull request 27776

(In reply to Ricky Chien [:rickychien] from comment #32)
> Oops! I found there shouldn't create a browser/package.json since browser
> app has been removed, right?
> 
> https://github.com/RickyChien/gaia/tree/master/apps/browser

Yes
Flags: needinfo?(poirot.alex)
Attachment #8556402 - Flags: review?(ricky060709)
Comment on attachment 8556402 [details] [review]
Pull request 27776

Let's land it!
Attachment #8556402 - Flags: review?(ricky060709) → review+

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.