1043953 - WebIDE should be able to build and push Gaia apps

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Alexandre Poirot [:ochameau]]

Now that bug 1000993 landed, we would need to put package.json files in gaia app folders.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: 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.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: 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.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 5

[Alexandre Poirot [:ochameau]]

Checking needed for pull request 27201, attachment 8545256 [details] [review].

Comment 6

[GitHub Autolander Bot]

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 7

[GitHub Autolander Bot]

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 8

[Alexandre Poirot [:ochameau]]

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!

Comment 9

[Alexandre Poirot [:ochameau]]

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!

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8544650 [details] [review]
pull request 27160

LGTM

Comment 12

[Alexandre Poirot [:ochameau]]

(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.

Comment 13

[David Flanagan [:djf]]

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.

Comment 14

[James Burke [:jrburke]]

Comment on attachment 8544650 [details] [review]
pull request 27160

r+ in the email app.

Comment 15

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8544650 [details] [review]
pull request 27160

For Dialer and Callscreen.

Comment 16

[Alexandre Poirot [:ochameau]]

(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

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8544650 [details] [review]
pull request 27160

r=me for settings app.

Comment 18

[Steve Chung [:steveck]]

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?

Comment 19

[Ricky Chien [:rickychien] (inactive)]

(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

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 :)

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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!

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Bug was filed! See bug 1125031.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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.

Comment 24

[Julien Wajsberg [:julienw]]

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.

Comment 25

[Dominic Kuo [:dkuo]]

Comment on attachment 8544650 [details] [review]
pull request 27160

The music part looks good to me, thanks!

Comment 26

[GitHub Autolander Bot]

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 27

[GitHub Autolander Bot]

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 28

[Alexandre Poirot [:ochameau]]

Ricky, I tweaked this patch in order to blacklist package.json files from zip files, according to comment 18 from Steve.

Comment 29

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8544650 [details] [review]
pull request 27160

+1 :)

Comment 30

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8544650 [details] [review]
pull request 27160

I leaved a comment to fix regexp. It seems no other problems to me.

Comment 31

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla-b2g/gaia/commit/80dc3ce34b016592c69fec85d2b22b5bb98f82c5
https://github.com/mozilla-b2g/gaia/commit/17015d5005998f4e03634f30951620f2360a60e4

Comment 32

[Ricky Chien [:rickychien] (inactive)]

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

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: 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

Comment 34

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8556402 [details] [review]
Pull request 27776

Let's land it!

Comment 35

[Alexandre Poirot [:ochameau]]

Landed the small followup:
  https://github.com/mozilla-b2g/gaia/commit/984a3c5d1613179de7f5cf770571d4c3dc75434d