1000993 - Support pre-push hooks / build step

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[Paul Rouget [:paul]]

We want the app manager to be able to run a custom script before pushing an app.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Filter on 86b7095e-2bd0-499e-a704-d00f2524aeef / PAUL STOP SETTING QA CONTACT TO THE DEVTOOLS COMPONENT'S WATCHERS EMAIL FOR BUGS YOU FILE :)

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

This patch allows specifying a "webide.json" file in the app folder, that looks like this:
{
  "command": {
    "path": "/usr/bin/make",
    "env": ["APP=system"],
    "cwd": "../.."
  },
  "stage": "../../build_stage/system/",
  "appId": "system.gaiamobile.org"
}

It contains no more, nor less than what is needed to build a gaia app.
It may end up being easier to build a cordova app...
The main challenge of gaia is that you can't just run a generic command in the app dir.
You need to APP=$(appname) and to run make from gaia root folder!

Next steps:
 - test this against a cordova app
 - figure out if we can cover this with a test
 - see if we can display command output somewhere in WebIDE UI,
   in this patch it is just thrown to the Browser console.

Otherwise, I don't think this depends on the project agnostic cleanup at all.
It will surely benefit from this cleanup, but we don't have to wait on it to move forward!
But as this feature isn't naive, may be we could move part of it to its own "build" module?

I tested this against gaia and it is very pleasant to use!!
It even works with the system app (with bug 1105782 applied).
That makes gaia contribution workflow waaaayyy faster and easier.
You also easily get tempted to use WebIDE internal editor to hack gaia :o

Comment 4

[Alexandre Poirot [:ochameau]]

Ricky, George, this is the feature we talked about, that would allow running gaia build system from WebIDE.

Comment 5

[Paul Rouget [:paul]]

So the user needs to provide a webide.json file.
I was thinking these fields could be stored in the project object we store in indexeddb.
Does it need to be a separate file?
If it's not a separate file, we need a way to define these fields (a UI in WebIDE), or guess them depending on the type of project being imported.

I'm not against this approach, but we need to be damn sure it's the way we want to do it. We don't want future projects to become invalid because we change our mines.

Comment 6

[Alexandre Poirot [:ochameau]]

We could also store that in indexedDB, but that looks painful if the only way to set the data is some UI, we would need quite a lot of UI, not really fun to fill. There is quite a bunch of parameters to specify here and unless we introduce a special "gaia app" type with various specifics just for gaia we will always need to know about these required parameters.
But that doesn't mean we can't do both, we could also have a UI counterpart, that would store some naive fields in the webide storage.
This manifest is just a way to tell webide about a set of parameters. WebIDE can also just grab it and save it to the storage... it is just an easy way to not involve a complex UI.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I hope to review this soon, most likely Monday.  I've been trying to think carefully about what this design will mean in the future in the intervening time.  Sorry for the delay.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8529785 [details] [diff] [review]
patch v1

Review of attachment 8529785 [details] [diff] [review]:
-----------------------------------------------------------------

I think placing this info in a metadata file is a good approach overall.  Other IDEs like Xcode and IntelliJ allow you to do the same thing, though some of them create separate files for static settings usable by the whole team vs. private per-user settings (to avoid checking in the per-user ones).  There's no need for each developer to tune these settings so far, so I can imagine some projects could check in the file (if desired), and then for a multiple developer project, WebIDE will just work for everyone.  If a project doesn't want to check it in, then it's still okay, as they can add it to the project's ignore list.

As for the details...  Would it be better to be part of an existing metadata file?  For example, npm suggests tools should use package.json for their metadata[1] (see "Use package.json for metadata").  Gaia does not have a per-app package.json, but Gaia is definitely a special case I would think.  Many complex front-end apps today would likely already have a package.json to make use of Gulp, Grunt, etc.  We could add our own sub-section, or use part of the scripts block.  What do you think about this?  Do you feel strongly that we should use our own file?  Also, look at other IDEs and workflows to see what they do here.  Form a strong opinion so we can bikeshed for a while... :)

For the build command, instead of the key "command", let's use a name specific to when the command is run, such as "prepackage", since it's before WebIDE assembles packaged zip for you.

I would like to support entering the command as would on the shell.  So, if the command is a single string, use it directly, otherwise it's a more complex object to set env and cwd if they are needed.  Is it possible to have env args in the string too?  I suppose then you have to parse them out of the string somehow, but it sounds nicer to work with for the user.

I think there is a better name for |stage|, such as |out| or |package| or something.

Shouldn't |appId| be part of the real app |manifest.webapp|?  Why is it needed in this new file?

And yes, let's make a new build module.  app-manager already does too many things.

So, lots to improve and discuss, but I like the approach overall!

As for whether there is a UI, I agree with Alex that we could later add a UI to mess with this file for WebIDE to present a special view of it if we want, but for now it's quite simple, and it's easy to just edit it.

[1]: http://blog.npmjs.org/post/101775448305/npm-and-front-end-packaging

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> Comment on attachment 8529785 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8529785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think placing this info in a metadata file is a good approach overall. 
> Other IDEs like Xcode and IntelliJ allow you to do the same thing, though
> some of them create separate files for static settings usable by the whole
> team vs. private per-user settings (to avoid checking in the per-user ones).
> There's no need for each developer to tune these settings so far, so I can
> imagine some projects could check in the file (if desired), and then for a
> multiple developer project, WebIDE will just work for everyone.

Except that here, the executable path has to be absolute,
so it is most likely going to be per-user specific :/
Subprocess doesn't support shell command line strings.
Supporting shell command line strings is non-trivial,
especially if you want to make it work on multiple OS.
The easiest way to implement it is pulling env["SHELL"] and execute shell
with arguments instead of the original program.
There is something equivalent on windows to retrieve cmd.exe, env["ComSpec"].
But I'm wondering how resiliant are these env variables...

Do you think I should support smart command string in this patch?
I can try adding this shell logic...

> As for the details...  Would it be better to be part of an existing metadata
> file?  For example, npm suggests tools should use package.json for their
> metadata[1] (see "Use package.json for metadata").
> Gaia does not have a
> per-app package.json, but Gaia is definitely a special case I would think. 
> Many complex front-end apps today would likely already have a package.json
> to make use of Gulp, Grunt, etc.  We could add our own sub-section, or use
> part of the scripts block.  What do you think about this?  Do you feel
> strongly that we should use our own file?  Also, look at other IDEs and
> workflows to see what they do here.  Form a strong opinion so we can
> bikeshed for a while... :)

package.json kind of conflicts with manifest.webapp as many fiels would be the same
(name, description, ...).
But manifest.webapp contains what looks like only data useful at runtime.
Adding any information that is only used at build time looks wrong
as would automatically end up on device where we don't need it.
So keeping another manifest, for all non-runtime data looks useful.
I don't care much how it is called. And your point about Grunt and all makes sense.

I moved to a "webide" property within package.json.

> For the build command, instead of the key "command", let's use a name
> specific to when the command is run, such as "prepackage", since it's before
> WebIDE assembles packaged zip for you.

Ok.

> I would like to support entering the command as would on the shell.  So, if
> the command is a single string, use it directly, otherwise it's a more
> complex object to set env and cwd if they are needed.  Is it possible to
> have env args in the string too?  I suppose then you have to parse them out
> of the string somehow, but it sounds nicer to work with for the user.
> 
> I think there is a better name for |stage|, such as |out| or |package| or
> something.

Ok. I'm now using 'out'.
Note that if this attribute isn't specified, we use the source directory.
I'm wondering if this attribute should be mandatory?
Will there be a usecase where we have a build step and want to use the source tree?
(If that happen to be the case, we will still be able to specify `.`,
 but at least it would be explicit)

> Shouldn't |appId| be part of the real app |manifest.webapp|?  Why is it
> needed in this new file?

In order to override the uuid application id used by WebIDE.
This application ID is generated by gaia build system, so it is outside of the app scope :/
The domain `gaiamobile.org` is a variable in root Makefile.

> And yes, let's make a new build module.  app-manager already does too many
> things.

Done: browser/devtools/webide/modules/build.js

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Created attachment 8534984 [details] [diff] [review]
> patch v2
> 
> (In reply to J. Ryan Stinnett [:jryans] from comment #8)
> > Comment on attachment 8529785 [details] [diff] [review]
> > patch v1
> > 
> > Review of attachment 8529785 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think placing this info in a metadata file is a good approach overall. 
> > Other IDEs like Xcode and IntelliJ allow you to do the same thing, though
> > some of them create separate files for static settings usable by the whole
> > team vs. private per-user settings (to avoid checking in the per-user ones).
> > There's no need for each developer to tune these settings so far, so I can
> > imagine some projects could check in the file (if desired), and then for a
> > multiple developer project, WebIDE will just work for everyone.
> 
> Except that here, the executable path has to be absolute,
> so it is most likely going to be per-user specific :/

When I said it's "not per-user", I meant just that there's nothing like "/home/alex/..." in the file, which would obviously need to change per person.

You are right that currently the program's path is absolute, though.  It's likely to work for multiple people, but still not quite as general as it could be.

> Subprocess doesn't support shell command line strings.
> Supporting shell command line strings is non-trivial,
> especially if you want to make it work on multiple OS.
> The easiest way to implement it is pulling env["SHELL"] and execute shell
> with arguments instead of the original program.
> There is something equivalent on windows to retrieve cmd.exe, env["ComSpec"].
> But I'm wondering how resiliant are these env variables...
> 
> Do you think I should support smart command string in this patch?
> I can try adding this shell logic...

I think we should try it.  It makes this much nicer to work with.

On Linux and Mac, I believe you could pass everything as args to /usr/bin/env, right?

So, you'd specify "APP=system make" as the command in package.json, and you would just pass all of that as command args.  You wouldn't pass anything as "real" environment args to subprocess.

On Windows... I am not sure.  Let's investigate and see what can be done.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8534984 [details] [diff] [review]
patch v2

Review of attachment 8534984 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 10 for my command suggestions.

For Gaia apps, I think it will be okay to add a package.json file, since it can start with just a "webide" section only, if Gaia team does not want to repeat the name, etc. from other files.

I'd also like some tests with example manifests.  Just make sure we "would" call out the build step correctly and package the right dir.

A test for the Gaia setup and one for a "normal" app that runs a command in the same directory would be good.

Looking good overall.  I am excited to have this!

::: browser/devtools/webide/modules/app-manager.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const {Cu, Cc, Ci} = require("chrome");

I think you can revert this.

@@ +9,5 @@
>  const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>  const {Services} = Cu.import("resource://gre/modules/Services.jsm");
>  const {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm");
>  const EventEmitter = require("devtools/toolkit/event-emitter");
> +const {TextDecoder, TextEncoder, OS}  = Cu.import("resource://gre/modules/osfile.jsm", {});

I think you can revert this.

@@ +466,5 @@
>        }
>  
>        let response;
>        if (project.type == "packaged") {
> +        let stage = yield ProjectBuilding.build(project);

Name this var |packageDir|.

@@ +468,5 @@
>        let response;
>        if (project.type == "packaged") {
> +        let stage = yield ProjectBuilding.build(project);
> +
> +        let path = stage || project.location;

packageDir = packageDir || project.location;

I think defaulting like this is fine in case there is a build step but not |packageDir| set.  The pre-package step could be super-simple and people just want things to work without defining extra bits.

::: browser/devtools/webide/modules/build.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {Cu, Cc, Ci} = require("chrome");
> +
> +let { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

Nit: Use destructuring spaces consistently in the file.

@@ +9,5 @@
> +const {TextDecoder, TextEncoder, OS}  = Cu.import("resource://gre/modules/osfile.jsm", {});
> +const Subprocess = require("sdk/system/child_process/subprocess");
> +
> +const ProjectBuilding = exports.ProjectBuilding = {
> +  fetchPackageManifest: project => Task.spawn(function * () {

See syntax note below for |build|.

@@ +30,5 @@
> +    return manifest;
> +  }),
> +
> +  // If the app depends on some build step, run it before pushing the app
> +  build: project => Task.spawn(function* () {

I think

build: Task.async(function* (project) {

is the less cryptic version of this.

@@ +42,5 @@
> +
> +    // Expose a way for the app to override the random app id generated
> +    // by WebIDE.
> +    if (manifest.appId) {
> +      project.packagedAppOrigin = manifest.appId;

manifest.webapp has the "origin" field that can be used to achieve this.

Why can't Gaia use that?  I see what you mean that GAIA_DOMAIN is a Makefile var...  But why?  When would you ever change it?

If there is some reason for Gaia to have this variable, could the build process at least set "origin" in the manifest.webapp in the output dir?  Then you could check that manifest.webapp and update the value here, instead of needing it in this file.

The current setup seems like a workaround for something that would only ever help Gaia, so let's try to fix Gaia to not need it if we can.

@@ +56,5 @@
> +                      manifest.prepackage.path + "'");
> +    }
> +
> +    let args = manifest.prepackage.args || [];
> +    args = args.map(a => a.replace(/%app%/g, project.location));

What about %project% as the token, since it gets replaced with WebIDE's project location?

@@ +73,5 @@
> +      // First check if the path is absolute
> +      // Ignore relative ones immediately
> +      if (!path.startsWith(".")) {
> +        let exists = yield OS.File.exists(path);
> +        if (exists && !path.startsWith("..")) {

Why is |!path.startsWith("..")| repeated here, but with another dot?

@@ +78,5 @@
> +          return path;
> +        }
> +      }
> +      // Then check for relatives
> +      let rel = OS.Path.join(project.location, path);

I think you can skip the block above and just call this.  If |path| is absolute, OS.Path.join[1] seems to handle it correctly.

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/ospath_unix.jsm#86

@@ +93,5 @@
> +    if (!cwd) {
> +      cwd = project.location;
> +    }
> +
> +    console.log("Running pre-package hook '" + manifest.prepackage.path + "' " +

I really wish we had some UI to say this in WebIDE...  It will be very confusing if something takes a long time with no apparent message to the user.

@@ +125,5 @@
> +      throw new Error('Unable to run pre-package command ' + manifest.prepackage.path +
> +        ' ' + args.join(' ') + ":\n" + (e.message || e));
> +    }
> +
> +    if (manifest.out) {

I'm changing my mind again, sorry... Let's name this field |packageDir| instead of |out|, it's "the directory that will become the package", so hopefully that name is clear enough.

@@ +130,5 @@
> +      let exists = yield OS.File.exists(manifest.out);
> +      if (exists) {
> +        return manifest.out;
> +      }
> +      let out = OS.Path.join(project.location, manifest.out);

Probably another place where you can always |join| first.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

+tests, +command line string

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=806f30a8de05

If tests work fine on CI... I'm pretty confident about the SHELL/ComSpec feature!
/usr/bin/env doesn't work as you can't pass a command line argument to it,
but only a list of arguments. It is too complex to interpret a command line string
in order to split it in a list of arguments. It is better to call a shell program.

In reply to J. Ryan Stinnett [:jryans] from comment #11)
> Comment on attachment 8534984 [details] [diff] [review]
> @@ +468,5 @@
> >        let response;
> >        if (project.type == "packaged") {
> > +        let stage = yield ProjectBuilding.build(project);
> > +
> > +        let path = stage || project.location;
> 
> packageDir = packageDir || project.location;
> 
> I think defaulting like this is fine in case there is a build step but not
> |packageDir| set.  The pre-package step could be super-simple and people
> just want things to work without defining extra bits.

I got confused by this already 2 times, I think it would be clearer if we display this path somewhere in UI.
If you push a gaia app from the source directory it is often completely broken.

> @@ +42,5 @@
> > +
> > +    // Expose a way for the app to override the random app id generated
> > +    // by WebIDE.
> > +    if (manifest.appId) {
> > +      project.packagedAppOrigin = manifest.appId;
> 
> manifest.webapp has the "origin" field that can be used to achieve this.
> 
> Why can't Gaia use that?  I see what you mean that GAIA_DOMAIN is a Makefile
> var...  But why?  When would you ever change it?
> 
> If there is some reason for Gaia to have this variable, could the build
> process at least set "origin" in the manifest.webapp in the output dir? 
> Then you could check that manifest.webapp and update the value here, instead
> of needing it in this file.
> 
> The current setup seems like a workaround for something that would only ever
> help Gaia, so let's try to fix Gaia to not need it if we can.
> 

Its not only gaia, it allows overloading existing apps.
I'm not sure `origin` attribute is a real attribute of manifest.webapp.
I think it is only interpreted by gaia build system and devtools webapps actor.
I looked at the webapps codebase and I haven't found any usage of it in the runtime.
So ideally this value wouldn't pollute the runtime manifest.webapp file that we push to the device,
and instead be kept in such build/setup time data file.
I think we should continue our discussion about this.

> @@ +73,5 @@
> > +      // First check if the path is absolute
> > +      // Ignore relative ones immediately
> > +      if (!path.startsWith(".")) {
> > +        let exists = yield OS.File.exists(path);
> > +        if (exists && !path.startsWith("..")) {
> 
> Why is |!path.startsWith("..")| repeated here, but with another dot?

No idea... While writing the patch I got confused by the fact that OS.File.exists('./..') was true. Most likely a leftover of this discovery...

> @@ +93,5 @@
> > +    if (!cwd) {
> > +      cwd = project.location;
> > +    }
> > +
> > +    console.log("Running pre-package hook '" + manifest.prepackage.path + "' " +
> 
> I really wish we had some UI to say this in WebIDE...  It will be very
> confusing if something takes a long time with no apparent message to the
> user.

Me too, but I wouldn't block on this. I would like to followup in order to display a console with the build output!

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8537219 [details] [diff] [review]
patch v3

Forgot to reask the review... this is ready for another round!
There is some failure on Windows, but I think that because of quote escaping.
It looks like the package.json command:
  echo "{\"name\":\"hello\"}" > manifest.webapp
Doesn't need \" in echo string on windows :/
Backslashes end up in the manifest file!

Comment 14

[Kevin Grandon :kgrandon]

Just chiming in to say that this looks great. I think that this can probably land functionality wise as-is, but in the future I'd like to make sure we can support some "watcher" workflow, which is becoming popular with developers, and we have implemented here: https://github.com/fxos/build

For this workflow, developers like to run some command like `gulp` or `grunt` in the background to automatically build projects as files are saved. We could have another section in the `webide` conifg. This would probably need to start running once developers start debugging the project, and exit when they stop debugging the project. E.g., 

"webide": {
  "prepackage": false,
  "watcher": "gulp watch"
}

I'm not sure how we're surfacing the output of these tasks to the user, but that's also something we might need to do. Thanks for the work here, I'll file a bug for this.

Comment 15

[Alexandre Poirot [:ochameau]]

Another try with a windows-specific patch:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=849d5bcbd03b

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8537219 [details] [diff] [review]
patch v3

Review of attachment 8537219 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, great work! :D

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> If tests work fine on CI... I'm pretty confident about the SHELL/ComSpec
> feature!
> /usr/bin/env doesn't work as you can't pass a command line argument to it,
> but only a list of arguments. It is too complex to interpret a command line
> string
> in order to split it in a list of arguments. It is better to call a shell
> program.

Cool, that's quite nice!  Thanks for adding this. :)

> In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > Comment on attachment 8534984 [details] [diff] [review]
> > @@ +468,5 @@
> > >        let response;
> > >        if (project.type == "packaged") {
> > > +        let stage = yield ProjectBuilding.build(project);
> > > +
> > > +        let path = stage || project.location;
> > 
> > packageDir = packageDir || project.location;
> > 
> > I think defaulting like this is fine in case there is a build step but not
> > |packageDir| set.  The pre-package step could be super-simple and people
> > just want things to work without defining extra bits.
> 
> I got confused by this already 2 times, I think it would be clearer if we
> display this path somewhere in UI.
> If you push a gaia app from the source directory it is often completely
> broken.

Yes, please file a follow up to show the package directory in the project details view.

> > @@ +42,5 @@
> > > +
> > > +    // Expose a way for the app to override the random app id generated
> > > +    // by WebIDE.
> > > +    if (manifest.appId) {
> > > +      project.packagedAppOrigin = manifest.appId;
> > 
> > manifest.webapp has the "origin" field that can be used to achieve this.
> > 
> > Why can't Gaia use that?  I see what you mean that GAIA_DOMAIN is a Makefile
> > var...  But why?  When would you ever change it?
> > 
> > If there is some reason for Gaia to have this variable, could the build
> > process at least set "origin" in the manifest.webapp in the output dir? 
> > Then you could check that manifest.webapp and update the value here, instead
> > of needing it in this file.
> > 
> > The current setup seems like a workaround for something that would only ever
> > help Gaia, so let's try to fix Gaia to not need it if we can.
> > 
> 
> Its not only gaia, it allows overloading existing apps.
> I'm not sure `origin` attribute is a real attribute of manifest.webapp.
> I think it is only interpreted by gaia build system and devtools webapps
> actor.
> I looked at the webapps codebase and I haven't found any usage of it in the
> runtime.
> So ideally this value wouldn't pollute the runtime manifest.webapp file that
> we push to the device,
> and instead be kept in such build/setup time data file.
> I think we should continue our discussion about this.

What's enough proof to consider "manifest.origin" an official property? :)

Here is the evidence I have gathered so far:

* 2013-03-19: potch files a bug "allow packaged app to have an origin" (bug 852720)
* 2013-05-31: fabrice lands[1] first mentions of manifest.origin for signed apps (bug 852720)
* 2013-07-02: markg added "origin" to the App Manifest MDN page[2] (bug 878094)
* 2013-07-25: You filed a bug about webapps actor support for origin (bug 897969)
* 2013-09-16: You landed webapps actor support for origin (bug 897969)

[1]: http://hg.mozilla.org/mozilla-central/rev/5330a588cb14
[2]: https://developer.mozilla.org/en-US/Apps/Build/Manifest$compare?locale=en-US&to=450329&from=437265

Given that it is used by Webapps.jsm with signed apps from the Marketplace, it seems pretty real to me.

> > @@ +93,5 @@
> > > +    if (!cwd) {
> > > +      cwd = project.location;
> > > +    }
> > > +
> > > +    console.log("Running pre-package hook '" + manifest.prepackage.path + "' " +
> > 
> > I really wish we had some UI to say this in WebIDE...  It will be very
> > confusing if something takes a long time with no apparent message to the
> > user.
> 
> Me too, but I wouldn't block on this. I would like to followup in order to
> display a console with the build output!

I made a note about this bug's work in bug 1093193 about labeling the progress bar, to bubble up some log message like this into the UI.

::: browser/devtools/webide/modules/build.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {Cu, Cc, Ci} = require("chrome");
> +
> +let { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

require("promise")

@@ +41,5 @@
> +    manifest = manifest.webide;
> +
> +    // Expose a way for the app to override the random app id generated
> +    // by WebIDE.
> +    if (manifest.appId) {

See my general comments above.  I am currently not convinced we need this.

@@ +65,5 @@
> +      if (home) {
> +        env.push("HOME=" + home);
> +      }
> +
> +      function* resolvePath(path) {

Not sure you get much value from this nested function, but keep it if you prefer!

@@ +93,5 @@
> +                args.join(" ") +
> +                " with ENV=[" + env.join(", ") + "]" +
> +                " at " + cwd);
> +
> +    // Run the command trough a shell command in order to support non absolute

Nit: through

@@ +146,5 @@
> +      let exists = yield OS.File.exists(packageDir);
> +      if (exists) {
> +        return packageDir;
> +      }
> +      throw new Error("Unable to resolve application package directory: '" + manifest.packageDir+ "'");

Nit: space before +

::: browser/devtools/webide/test/test_build.html
@@ +29,5 @@
> +              let details = win.UI.projecteditor.window.frames[0];
> +              return !details.document.body.classList.contains("error");
> +            }
> +
> +            // # Test first package.json with just {webide: {prepackage: "command line string"}}

I guess you're going for comments in multiple languages? :P Or maybe it's a list...?

@@ +55,5 @@
> +            is(project.name, "hello", "Display name uses manifest name");
> +            is(project.manifest.name, project.name, "Display name uses manifest name");
> +
> +            yield OS.File.remove(OS.Path.join(packagedAppLocation, "manifest.webapp"));
> +            

Nit: trailing whitespace

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Addressed review comments.
I made a noticeable change: I'm running build command before validation,
so that we validate what we actually push to the phone.
Also build step may create/modify the manifest or important files being validated.
I also fixed tests on linux by saving and restoring CWD from build.js.
(Subprocess modifies it globally and doesn't restore the original value,
 some random test ended up failing :s)
And I'm still waiting for green results on windows.

Carrying over r+ as the modification is quite naive,
but feel free to comment this patch again!

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Another fix for windows, should be good this time:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=79e7a5118f9c

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

Finally works on windows \o/
I had to add some OS.Path.normalize here and there,
in order to convert relative paths like "./stage" into windows-compatible paths.
So that we can use unix separator in package.json.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e726b14aad0

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8542565 [details] [diff] [review]
patch v6

Review of attachment 8542565 [details] [diff] [review]:
-----------------------------------------------------------------

One last try to ensure I didn't broke other platform while fixing windows:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47edd0d0e366

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/381e48ab4396

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/381e48ab4396

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We should document this "webide" section of package.json so that app developers know how to use them.

Comment 24

[Will Bamberg [:wbamberg]]

So:

- "cwd" is the name of the directory from which to run the command
- "packageDir" is the output directory for the build command, in case we want the project in WebIDE to be the source from which we build a packaged app, and want to instruct WebIDE to install the app from that output directory rather than the project directory

Is that right?

Apart from that I think I understand this fine. What's a good use case for this feature? Using WebIDE to write Cordova apps (bug 1124326)? Using WebIDE to write Gaia apps? What sort of special pre-push work needs to be done in those cases?

Comment 25

[Alexandre Poirot [:ochameau]]

(In reply to Will Bamberg [:wbamberg] from comment #24)
> So:
> 
> - "cwd" is the name of the directory from which to run the command
> - "packageDir" is the output directory for the build command, in case we
> want the project in WebIDE to be the source from which we build a packaged
> app, and want to instruct WebIDE to install the app from that output
> directory rather than the project directory
> 
> Is that right?

Yes!

> Apart from that I think I understand this fine. What's a good use case for
> this feature? 

I decided to push foward this bug and submit a patch in order to allow gaia developers to be able to use WebIDE to push app updates. Before that, they all had to push updates manually, from command line with: APP=email make install-gaia.
I still plan to send a mail to dev-gaia in order to highlight this feature. (I still need to finish bug 1043953's patch in order to communicate about it)

> Using WebIDE to write Cordova apps (bug 1124326)?

We had this bug opened for multiple quarters in order to unlock cordova development via WebIDE.
So it sounds also important.

> Using WebIDE to write Gaia apps? What sort of special pre-push work needs to be done in
> those cases?

But yes, this feature applies to any app with a build system. It is more and more common these days to have some grunt (or equivalent) build step... One typical need is require.js that need to squash dependencies. I imagine the second typical usage is css/js minifying.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Cordova package.json example

Here's an example package.json for working with Cordova.  This one is simpler and uses less of the possible options.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Gaia package.json example

Here's an example package.json from the Gaia Settings app.  This demonstrates more of the available options.

Comment 28

[Will Bamberg [:wbamberg]]

Thanks, Alex and Ryan, for the clarifications and examples. I've added a section to the WebIDE docs: https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Running_a_custom_build_step.

(I guess if package.json sprouts many more things we should break it out into a separate "reference" page/section, but for now it seems more natural to fit it into the workflow.)

The table might be a bit odd, but I thought it a visual way to show the structure of the objects while still being able to describe each item.

OT, let me know if you think the page is starting to get too big/unwieldy and I'll split it up. I'm starting to do this for some of the other bug devtools pages (https://developer.mozilla.org/en-US/docs/Tools/Debugger).

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Will Bamberg [:wbamberg] from comment #28)
> Thanks, Alex and Ryan, for the clarifications and examples. I've added a
> section to the WebIDE docs:
> https://developer.mozilla.org/en-US/docs/Tools/
> WebIDE#Running_a_custom_build_step.
> 
> (I guess if package.json sprouts many more things we should break it out
> into a separate "reference" page/section, but for now it seems more natural
> to fit it into the workflow.)

Maybe it's helpful to mention that we did not make up this file?  It's the same file name used to package a node.js library[1].  So, there may in fact be many other keys in this file already, especially if your project already uses node for anything.  The phrasing "...currently contains a single property 'webide'" reads strangely with that in mind.

In case you are curious about this choice to use another tool's file, the npm blog has a section advocating that other tools do this, see "Use package.json for metadata"[2].

For the "packageDir" field, it's optional, but it's not quite clear to me reading the current text.  If it's not there, the root of the project is what will be packaged up (same as if no package.json is present at all).  Some projects might run some build command to e.g. minify your JS but still just package everything from the root directory.  Not sure which approach is more common for projects with build steps, but in any case, both are possible (root dir or separate dir).

[1]: https://docs.npmjs.com/files/package.json
[2]: http://blog.npmjs.org/post/101775448305/npm-and-front-end-packaging

> The table might be a bit odd, but I thought it a visual way to show the
> structure of the objects while still being able to describe each item.

I am okay with the table setup, I can't think of a better approach without jumping to something much more complex and interactive, so this seem fine to me.  Besides, there are also examples, so hopefully that will help too.

> OT, let me know if you think the page is starting to get too big/unwieldy
> and I'll split it up. I'm starting to do this for some of the other bug
> devtools pages (https://developer.mozilla.org/en-US/docs/Tools/Debugger).

It is getting a bit big!  Not quite sure how to best break it up for the moment...  Another thing to keep in mind is that we'll be adding more and more types of projects (add-on, regular web sites, etc.), so there may be some division by project type at some point.

Comment 30

[Will Bamberg [:wbamberg]]

Thanks for the feedback. I've updated the section. I'll mark this ddc, but please let me know if it needs more work.

For splitting the WebIDE page, I've raised bug 1127081.