Closed Bug 1043953 Opened 10 years ago Closed 10 years ago

WebIDE should be able to build and push Gaia apps

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: paul, Assigned: ochameau)

References

Details

Attachments

(3 files)

46 bytes, text/x-github-pull-request
crdlc
: review+
gaye
: review+
djf
: review+
sfoster
: review+
dkuo
: review+
steveck
: review+
arthurcc
: review+
timdream
: review+
mcav
: review+
julienw
: review+
jrburke
: review+
ggp
: review+
arcturus
: review+
drs
: review+
rickychien
: review+
Details | Review
46 bytes, text/x-github-pull-request
rickychien
: review+
ochameau
: review+
Details | Review
46 bytes, text/x-github-pull-request
rickychien
: review+
Details | Review
No description provided.
Depends on: 1023081
Blocks: 1043937
Now that bug 1000993 landed, we would need to put package.json files in gaia app folders.
Assignee: nobody → poirot.alex
Attached file 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.
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.
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 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+
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.
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+
Status: NEW → RESOLVED
Closed: 10 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)
Attached file 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+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: