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)
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.
Assignee | ||
Comment 1•10 years ago
|
||
Now that bug 1000993 landed, we would need to put package.json files in gaia app folders.
Assignee: nobody → poirot.alex
Assignee | ||
Comment 2•10 years ago
|
||
This pull request add all the package.json files to each gaia apps
and also ensure setting `origin` attribute in webapp manifests.
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Checking needed for pull request 27201, attachment 8545256 [details] [review].
Keywords: checkin-needed
Comment 6•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
Comment on attachment 8544650 [details] [review]
pull request 27160
LGTM
Attachment #8544650 -
Flags: review?(sfoster) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8544650 -
Flags: review?(guilherme.p.gonc+bmo) → review+
Updated•10 years ago
|
Attachment #8544650 -
Flags: review?(m) → review+
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
Comment on attachment 8544650 [details] [review]
pull request 27160
r+ in the email app.
Attachment #8544650 -
Flags: review?(jrburke) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8544650 [details] [review]
pull request 27160
For Dialer and Callscreen.
Attachment #8544650 -
Flags: review?(drs.bugzilla) → review+
Assignee | ||
Comment 16•10 years ago
|
||
(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 17•10 years ago
|
||
Comment on attachment 8544650 [details] [review]
pull request 27160
r=me for settings app.
Attachment #8544650 -
Flags: review?(arthur.chen) → review+
Comment 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
(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)
Comment 20•10 years ago
|
||
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 21•10 years ago
|
||
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+
Comment 23•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8544650 -
Flags: review?(crdlc) → review+
Comment 24•10 years ago
|
||
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•10 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+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(poirot.alex)
Attachment #8545256 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed,
leave-open
Comment 26•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Comment 27•10 years ago
|
||
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•10 years ago
|
Attachment #8544650 -
Flags: review?(francisco) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8544650 -
Flags: review?(ricky060709)
Assignee | ||
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
Comment on attachment 8544650 [details] [review]
pull request 27160
+1 :)
Attachment #8544650 -
Flags: review?(gaye) → review+
Comment 30•10 years ago
|
||
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+
Assignee | ||
Comment 31•10 years ago
|
||
Landed:
https://github.com/mozilla-b2g/gaia/commit/80dc3ce34b016592c69fec85d2b22b5bb98f82c5
https://github.com/mozilla-b2g/gaia/commit/17015d5005998f4e03634f30951620f2360a60e4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
(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 34•10 years ago
|
||
Comment on attachment 8556402 [details] [review]
Pull request 27776
Let's land it!
Attachment #8556402 -
Flags: review?(ricky060709) → review+
Assignee | ||
Comment 35•10 years ago
|
||
Landed the small followup:
https://github.com/mozilla-b2g/gaia/commit/984a3c5d1613179de7f5cf770571d4c3dc75434d
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•