944451 - Land simulator firefox addon in mozilla-central

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Simulator, defect)

Description

[Alexandre Poirot [:ochameau]]

In bug 935014, we are going to merge prothesis (i.e. the simulator tweaks made to b2g desktop) with b2g desktop.
Then, we will end up with the small firefox addon that ships b2g desktop and gaia and just register itself to the App manager via Simulator.jsm:
  https://github.com/mozilla/r2d2b2g/tree/master/addon/lib

We should also land this small addon in mozilla-central so that we can easily build it during b2g target.
During b2g desktop building, we end up with all b2g runtime files in $(objdir)/dist/bin. That's handy as we won't have to download/uncompress (dmg are hard to unpack) b2g desktop nightly builds.
That will also allow using the same gaia profile between the simulator and b2g desktop. So far, b2g desktop and the simulator build their own kind of gaia profile.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: land simulator addon into mozilla-central

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8342610 [details] [diff] [review]
land simulator addon into mozilla-central

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

So. Most of this patch is an uplift of the simulator addon from:
  https://github.com/mozilla/r2d2b2g/tree/master/addon
into mozilla-central. (main.js, simulator-process.js, package*, subprocess/*)

This addon doesn't do much. Its main goal is to offer an easy way to package 
and distribute b2g desktop+gaia to apps developers.
Addons are handy as it is easy to install on any platform once you have Firefox,
it is also very easy to package (just a zip).
Then the few code of this addon call Simulator.jsm, in order to tell the app manager
it exists and then receive orders to open or close b2g desktop.

I ended up hacking something in b2g desktop. Mostly because we need b2g desktop to be built
before being able to zip it into the addon xpi.
I'm all but a build system expert, so I took example on "GAIADIR"
and introduced a new variable, called "SIMULATOR".
That variable, once set to a 1 during a b2g desktop build,
is ordering the build system to also build the simulator addon.

I'm also reusing GAIADIR variable to build custom simulator profile.
(So SIMULATOR=1 implies GAIADIR to be set)

Speaking about gaia profile, I wish we could reuse the profile being built for b2g desktop.
Unfortunately, we expect different set of apps to be installed in the simulator (only production ones).
I would like to followup on this and try to find ways to share the same profile.
One path would be to have different set of apps based on the channel (all apps in nightly, and only production ones in aurora+).

Next steps would be to:
- continue working in bug 935014, in order to bring all simulator features directly into b2g desktop,
- set SIMULATOR=1 into b2g desktop nightly mozconfig and ask releng to put xpis on ftp (bug 920198 almost free).

ted, Could you review build system aspects? (again, I'm not used to tweak our build system, so feel free to suggest any kind of changes)

paul, Can you review this patch globally and especially the addon code?

vingtetun/fabrice, Can you validate our intrusion into b2g/ folder and register us (me+paul) as peers for this new /b2g/simulator folder?

Comment 3

[Ed Morley [:emorley]]

Comment on attachment 8342610 [details] [diff] [review]
land simulator addon into mozilla-central

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> ted, Could you review build system aspects? (again, I'm not used to tweak
> our build system, so feel free to suggest any kind of changes)

:gps is best bet for that now :-)
(https://wiki.mozilla.org/Modules/Core#Build_Config)

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Jonas, on the road to a unified dev environment, what do you think of that ?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8342610 [details] [diff] [review]
land simulator addon into mozilla-central

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

Since most of the code lives into a subdirectory of b2g/ and much of it has been reviewed by the simulator team, I'm not going to review it assuming that you guys are doing your work :)

You have my stamp to land that on b2g/ since that will also makes it easier to get the simulator / desktop build from Gaia and then start to work more closely. Let me know when it lands so I can do an update on the components of b2g/ to create the simulator/ subdirectory and add you guys as peers of it.

Though I would like this code to respect mozillla guidelines, a quick nits from paul on all the missing spaces, etc.. should not hurt. Also it is unclear to me why we need to rely on the Addon-SDK for that but I won't fight for that.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Fixed an issue on clobbered build.

Comment 7

[Gregory Szorc [:gps]]

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

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

So here's the deal. Make files are decently good at defining relationships between objects, the rules for building them, and building them. This is the problem space they were designed to solve.

However, the Makefile.in in this patch is a glorified shell script. IMO, make brings almost nothing to the table. The only thing it does is prevent package-overload.json from being regenerated if it hasn't changed (and this logic is flawed because APP_BUILDID isn't listed as a dependency) and makes package-overload.json build concurrently with the "profile" target. package-overload.json will build in under 10ms, so this isn't noticeable. Therefore, you really aren't using any useful features of make in this Makefile.

We are actively purging these shell-script-like make files from the tree. We are doing so by converting the logic in them to something else, where that something else 9 times out of 10 is Python - either a Python function under python/mozbuild, a standalone .py script, or a mach command.

This patch gets r- from the build system until the logic in Makefile.in is extracted into something not make. That something should be Python. That Python should use mozbuild.util.FileAvoidWrite and mozbuild.preprocessor.Preprocessor for generating package-overload.json and it should simply subprocess.check_call() the process invocations that make would have performed. You can invoke this Python from build.mk via $(PYTHON) path/to/script.py or $(PYTHON) -m module.name.

::: b2g/simulator/Makefile.in
@@ +6,5 @@
> +
> +include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
> +
> +APP_BUILDID = $(shell cat $(DEPTH)/config/buildid)
> +VERSION = $(shell echo "$(MOZ_B2G_VERSION)" | perl -p -e 's/^([0-9]+\.[0-9]+).*/\1/')

Perl in the build system is considered legacy and we're not accepting any new uses of Perl.

This variable is the kind of thing that should be defined in configure.

@@ +13,5 @@
> +
> +FTP_ROOT_PATH = /pub/mozilla.org/labs/fxos-simulator
> +UPDATE_PATH = $(VERSION)/$(MOZ_PKG_PLATFORM)
> +UPDATE_LINK = https://ftp.mozilla.org$(FTP_ROOT_PATH)/$(UPDATE_PATH)/$(XPI_NAME)
> +UPDATE_URL = https://ftp.mozilla.org$(FTP_ROOT_PATH)/$(UPDATE_PATH)/update.rdf

UPPERCASE variables are reserved for variables with special meanings to the build system. Please change these to lowercase variables.

@@ +23,5 @@
> +	# Finally set the full length addon version like 1.3.20131230
> +	sed -e 's/@@NUM_VERSION@@/$(VERSION)/' \
> +	    -e 's/@@SLASH_VERSION@@/$(subst .,_,$(VERSION))/' \
> +	    -e 's/@@FULL_VERSION@@/$(VERSION).$(APP_BUILDID)/' \
> +      $^ > $@

Please use the Python preprocessor - not sed - for doing all substitutions.

See the PP_TARGETS Makefile variable as in services/healthreport/Makefile.in.

::: b2g/simulator/moz.build
@@ +1,5 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# 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/.

You don't need a moz.build file in this directory since there is no *DIRS variable referencing this directory.

::: configure.in
@@ +176,5 @@
>  
> +if test -n "$SIMULATOR" -a -z "$GAIADIR" ; then
> +    AC_MSG_ERROR([SIMULATOR=1 requires GAIADIR to be defined])
> +fi
> +AC_SUBST(SIMULATOR)

"simulator" is a rather generic name. I think B2G_SIMULATOR, FXOS_SIMULATOR, or whatever this thing is, would be more appropriate.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: python instead of shell/makefile

I'm now using python Preprocessor, but I haven't found much usages for FileAvoidWrite
and I have no idea if I should have used some other stuff from mozbuild.
I don't know much about the internals of mozilla build system,
so do not hesitate to suggest better ways to organize this patch!

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8346029 [details] [diff] [review]
python instead of shell/makefile

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

This is better. I think.

glandium: I'm interested in your opinion regarding xpi creation. Do we have anything better they can use? Also, I figured you want to know about yet another extension requirement coming to the build system.

::: b2g/build.mk
@@ +9,5 @@
>  	@$(MAKE) -C b2g/installer
> +#ifdef FXOS_SIMULATOR
> +	$(PYTHON) $(srcdir)/b2g/simulator/simulator.py $(topsrcdir) $(DIST) \
> +			$(MOZ_B2G_VERSION) $(shell cat $(DEPTH)/config/buildid) \
> +			$(GAIADIR) $(MOZ_PKG_PLATFORM)

We should just read buildid from Python.

We can also read MOZ_B2G_VERSION and MOZ_PKG_PLATFORM from Python. We also know topsrcdir and $(DIST) in Python. Are there any arguments we actually need to pass here?

::: b2g/simulator/lib/main.js
@@ +1,1 @@
> +const { Cc, Ci, Cu } = require("chrome");

Need moar license header.

::: b2g/simulator/simulator.py
@@ +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/.
> +
> +import sys, os, re, subprocess

Please add a comment indicating the purpose of this script. Can we also name it build_simulator_xpi.py or something more descriptive?

@@ +10,5 @@
> +class GaiaBuilder(MozbuildObject):
> +    def __init__(self, topsrcdir, gaia_path):
> +        obj = MozbuildObject.from_environment()
> +        MozbuildObject.__init__(self, obj.topsrcdir, obj.settings, obj.log_manager, obj.topobjdir)
> +        self.gaia_path = gaia_path

The API is a bit hacky. But the proper way to do this is:

obj = MozbuildObject.from_environment()
gb = obj._spwan(GaiaBuilder)
gb.gaia_path = gaia_path

You could also do:

gb = GaiaBuilder.from_environment()
gb.gaia_path = gaia_path

(don't do it in this __init__ of course)

This is also the 2nd review today where overloading __init__ on MozbuildObject has been problematic. This API really sucks and needs to be fixed.

@@ +21,5 @@
> +
> +    def override_prefs(self, srcfile):
> +        # cat srcfile >> profile/user.js
> +        with open(os.path.join(self.gaia_path, "profile", "user.js"), "a") as userJs:
> +            userJs.write(open(srcfile).read())

That's not idempotent.

@@ +24,5 @@
> +        with open(os.path.join(self.gaia_path, "profile", "user.js"), "a") as userJs:
> +            userJs.write(open(srcfile).read())
> +
> +def process_package_overload(src, dst, version, app_buildid):
> +    os.makedirs(os.path.dirname(dst))

This will raise OSError with e.errno == errno.ENOEXIST if the destination exists.

from mozbuild.util import ensureParentDir
ensureParentDir(os.path.dirname(dst))

@@ +25,5 @@
> +            userJs.write(open(srcfile).read())
> +
> +def process_package_overload(src, dst, version, app_buildid):
> +    os.makedirs(os.path.dirname(dst))
> +    pp = Preprocessor()

There is a slightly revised API for the preprocessor landing in bug 935987. You may want to use the changes if that lands soon.

@@ +65,5 @@
> +
> +    # The simulator uses a shorter version string,
> +    # it only keeps the major version digits A.B
> +    # whereas MOZ_B2G_VERSION is A.B.C.D
> +    version = re.match(r"([0-9]+\.[0-9]+).*", b2g_version).group(1)

Alternatively, you could load distutils.version.StrictVersion or distutils.version.LooseVersion and use that for parsing version strings.

Either way, you may want to handle failures more gracefully.

@@ +73,5 @@
> +    # - set custom prefs to enable devtools debugger server
> +    # - set custom settings to disable lockscreen and screen timeout
> +    # - only ship production apps
> +    builder = GaiaBuilder(topsrcdir, gaia_path)
> +    builder.clean()

Must we clean on every build? A goal of build systems is to do nothing if nothing has changed. This script always does work. That only slows builds down. Multiply by 1000x and you have Firefox's build system, which everyone complains about.

@@ +74,5 @@
> +    # - set custom settings to disable lockscreen and screen timeout
> +    # - only ship production apps
> +    builder = GaiaBuilder(topsrcdir, gaia_path)
> +    builder.clean()
> +    env = { \

You don't need the \.

@@ +80,5 @@
> +      "GAIA_APP_TARGET": "production",\
> +      "SETTINGS_PATH": os.path.join(srcdir, "custom-settings.json")\
> +    }
> +    builder.profile(env)
> +    builder.override_prefs(os.path.join(srcdir, "custom-prefs.js"))

I guess since you cleaned, you don't need to worry about the non-idempotent behavior then.

@@ +95,5 @@
> +    xpi_path = os.path.join(distdir, xpi_name)
> +    ftp_root_path = "/pub/mozilla.org/labs/fxos-simulator"
> +    update_path = "%s/%s" % (version, platform)
> +    update_link = "https://ftp.mozilla.org%s/%s/%s" % (ftp_root_path, update_path, xpi_name)
> +    update_url = "https://ftp.mozilla.org%s/%s/update.rdf" % (ftp_root_path, update_path)

These should probably come from configure. But I can be convinced to let it slide.

@@ +97,5 @@
> +    update_path = "%s/%s" % (version, platform)
> +    update_link = "https://ftp.mozilla.org%s/%s/%s" % (ftp_root_path, update_path, xpi_name)
> +    update_url = "https://ftp.mozilla.org%s/%s/update.rdf" % (ftp_root_path, update_path)
> +    subprocess.check_call([
> +      os.environ.get("PYTHON", "python"), os.path.join(topsrcdir, "addon-sdk", "source", "bin", "cfx"), "xpi", \

Generally speaking, if Python calls out to a new Python process, you're doing it wrong. It's much preferred to just import that Python file into the current interpreter and invoke a function natively. Is there a good reason we can't do that?

Actually, looking at how cfx adjusts sys.path, yes, that's a good reason not to do it. However, that script is just a wrapper around "import cuddlefish", so perhaps we can just add addon-sdk/source/python-lib/cuddlefish to the virtualenv. Then, you can import build_xpi from cuddlefish.xpi and call that natively.

If we keep a process call, just get the python binary path via:

  self.virtualenv_manager.python_path

@@ +116,5 @@
> +        print("""Usage:
> +  python {0} TOP_SRC_DIR DIST_DIR B2G_VERSION APP_BUILDID GAIA_PATH PLATFORM
> +""".format(sys.argv[0]))
> +        sys.exit(1)
> +    main(*sys.argv[1:])

Hmm. You might as well use argparse.ArgumentParser to document all the arguments.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I definitely think we should do this.

What I think we should do is to create a build of Firefox-desktop, but that uses a Gecko compiled with MOZ_B2G enabled such that we're using the same gecko as FFOS uses. This build should include the simulator addon so that we can get the features that it provides (I'm not entirely sure which features those are these days?)

We should then create an about:gaia page which opens shell.html such that it has chrome privileges.

We should finally add an app:// protocol handler which loads resources directly from a gaia source tree.


Obviously this is just a rough outline. I've talked in more details with various people involved in this effort. But I hope the above explains my thinking.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: addressed review from comment 9

> We can also read MOZ_B2G_VERSION and MOZ_PKG_PLATFORM from Python. We also
> know topsrcdir and $(DIST) in Python. Are there any arguments we actually
> need to pass here?

Right, except for MOZ_PKG_PLATFORM. It isn't in substs nor defines.
If you know how to retrieve it, I'd be happy to get rid of this last argument.

> @@ +116,5 @@
> > +        print("""Usage:
> > +  python {0} TOP_SRC_DIR DIST_DIR B2G_VERSION APP_BUILDID GAIA_PATH PLATFORM
> > +""".format(sys.argv[0]))
> > +        sys.exit(1)
> > +    main(*sys.argv[1:])
> 
> Hmm. You might as well use argparse.ArgumentParser to document all the
> arguments.

So I'm still using this, as I only have one argument left, and hopefully
we can get rid of it and this code.

> @@ +10,5 @@
> > +class GaiaBuilder(MozbuildObject):
> > +    def __init__(self, topsrcdir, gaia_path):
> > +        obj = MozbuildObject.from_environment()
> > +        MozbuildObject.__init__(self, obj.topsrcdir, obj.settings, obj.log_manager, obj.topobjdir)
> > +        self.gaia_path = gaia_path
> 
> The API is a bit hacky. But the proper way to do this is:

I no longer inherits from MozBuildObject and delegate instead.
It makes the whole thing simplier without any complex.
The only bad thing is that I'm calling _run_make which looks private.

> @@ +73,5 @@
> > +    # - set custom prefs to enable devtools debugger server
> > +    # - set custom settings to disable lockscreen and screen timeout
> > +    # - only ship production apps
> > +    builder = GaiaBuilder(topsrcdir, gaia_path)
> > +    builder.clean()
> 
> Must we clean on every build?

Gaia's build system is kind of worse than m-c, it always redo almost everything.
So cleaning doesn't cost much. It can save from issues during some iterative builds,
as we don't have any clobber flags in gaia.
I let the call to clean to prevent any clobber issue,
but I can get rid of it, if you feel it is better that way.

> @@ +80,5 @@
> > +      "GAIA_APP_TARGET": "production",\
> > +      "SETTINGS_PATH": os.path.join(srcdir, "custom-settings.json")\
> > +    }
> > +    builder.profile(env)
> > +    builder.override_prefs(os.path.join(srcdir, "custom-prefs.js"))
> 
> I guess since you cleaned, you don't need to worry about the non-idempotent
> behavior then.

The call to "make profile" in gaia, will always create a brand new user.js file.

> @@ +95,5 @@
> > +    xpi_path = os.path.join(distdir, xpi_name)
> > +    ftp_root_path = "/pub/mozilla.org/labs/fxos-simulator"
> > +    update_path = "%s/%s" % (version, platform)
> > +    update_link = "https://ftp.mozilla.org%s/%s/%s" % (ftp_root_path, update_path, xpi_name)
> > +    update_url = "https://ftp.mozilla.org%s/%s/update.rdf" % (ftp_root_path, update_path)
> 
> These should probably come from configure. But I can be convinced to let it
> slide.

It doesn't feel right to put production data in configure,
but you are right, it doesn't feel right to keep such data in middle of a build script.
Wouldn't it better store in a default mozconfig file or something similar?
In the meantime, I moved these strings at the python script header, as globals.

> @@ +97,5 @@
> > +    update_path = "%s/%s" % (version, platform)
> > +    update_link = "https://ftp.mozilla.org%s/%s/%s" % (ftp_root_path, update_path, xpi_name)
> > +    update_url = "https://ftp.mozilla.org%s/%s/update.rdf" % (ftp_root_path, update_path)
> > +    subprocess.check_call([
> > +      os.environ.get("PYTHON", "python"), os.path.join(topsrcdir, "addon-sdk", "source", "bin", "cfx"), "xpi", \
> 
> Generally speaking, if Python calls out to a new Python process, you're
> doing it wrong. It's much preferred to just import that Python file into the
> current interpreter and invoke a function natively. Is there a good reason
> we can't do that?
> 
> Actually, looking at how cfx adjusts sys.path, yes, that's a good reason not
> to do it. However, that script is just a wrapper around "import cuddlefish",
> so perhaps we can just add addon-sdk/source/python-lib/cuddlefish to the
> virtualenv. Then, you can import build_xpi from cuddlefish.xpi and call that
> natively.

I have no idea how to add cuddlefish to virtualenv. (Again I'm not a python expert)
The only reason I can give for calling a process is that right now, jetpack uses python
for its packaging tool but it can change to something else.
It is just a packaging program and is meant to be called as a command line app.
But as it is hosted on m-c, it seems ok to call its internals directly.


I addressed all other comments.

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8356160 [details] [diff] [review]
addressed review from comment 9

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

Oops I didn't meant to reset review flags!

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8356160 [details] [diff] [review]
addressed review from comment 9

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

r+ only covers the build bits.

::: b2g/simulator/build_xpi.py
@@ +25,5 @@
> +UPDATE_LINK = "https://ftp.mozilla.org" + ftp_root_path + "/%(update_path)s/%(xpi_name)s"
> +UPDATE_URL = "https://ftp.mozilla.org" + ftp_root_path + "/%(update_path)s/update.rdf"
> +XPI_NAME = "fxos-simulator-%(version)s-%(platform)s.xpi"
> +
> +class GaiaBuilder():

class GaiaBuilder(object):

@@ +66,5 @@
> +        ignore = False
> +        for path in blacklist:
> +            if dir_relpath.startswith(path):
> +                ignore = True
> +                break

startswith() can accept a tuple to compare against. So you can collapse this down to a one-liner if blacklist is a tuple. Hint: blacklist = tuple(iterable)

@@ +100,5 @@
> +    # - set custom settings to disable lockscreen and screen timeout
> +    # - only ship production apps
> +    gaia_path = build.config_environment.substs["GAIADIR"]
> +    builder = GaiaBuilder(build, gaia_path)
> +    builder.clean()

It feels hacky that we have to call "clean" as part of building something. IMO `make profile` should do the right thing and should no-op if a profile is present and up to date. But I understand there may be limitations in Gaia's build system preventing this.

::: configure.in
@@ +166,5 @@
>  fi
>  
> +if test -n "$GAIADIR" -a ! -d "$GAIADIR" ; then
> +    AC_MSG_ERROR([GAIADIR '$GAIADIR' isn't a valid directory])
> +fi

I can't believe we didn't have this check until now!

Comment 15

[Paul Rouget [:paul]]

Comment on attachment 8356160 [details] [diff] [review]
addressed review from comment 9

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

What's the original license of the subprocess code?
Can we remove the readme and add its content in an inline comment in the code?
Few files are missing license block.
Add a comment to explain sizeof_fileaction.c.
Strip down the comment blocks in subprocess*.js.
Make sure Jan Gerber gets credited.

> user_pref("devtools.debugger.enable-content-actors", true);
> user_pref("b2g.adb.timeout", 0);

Are these needed?

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Address comment 14 and 15.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch

(In reply to Paul Rouget [:paul] from comment #15)
> Comment on attachment 8356160 [details] [diff] [review]
> addressed review from comment 9
> 
> Review of attachment 8356160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's the original license of the subprocess code?

  http://hg.mozilla.org/ipccode/file/b22a621afdc5/modules
  Looks like MPL

> Can we remove the readme and add its content in an inline comment in the
> code?

  What is the benefit of getting rid of the readme file?
  Inlining the doc would just make the xpi size (insignificantly) bigger.

> Few files are missing license block.

  I haven't added license to json file as we can't easily put comment in this format.

> Add a comment to explain sizeof_fileaction.c.

  I got rid of this thanks to bug 936297 and updated simulator.js accordingly.

> Strip down the comment blocks in subprocess*.js.

  Do you mean the C signature of each mapped function, and/or the docs?

> Make sure Jan Gerber gets credited.

  Where? He is credited in the precise files where he contributed,
  and there is also Patrick who worked a lot on subprocess.

> 
> > user_pref("devtools.debugger.enable-content-actors", true);
> > user_pref("b2g.adb.timeout", 0);
> 
> Are these needed?

 Not really... I removed them.

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8356160 [details] [diff] [review]
> @@ +100,5 @@
> > +    # - set custom settings to disable lockscreen and screen timeout
> > +    # - only ship production apps
> > +    gaia_path = build.config_environment.substs["GAIADIR"]
> > +    builder = GaiaBuilder(build, gaia_path)
> > +    builder.clean()
> 
> It feels hacky that we have to call "clean" as part of building something.
> IMO `make profile` should do the right thing and should no-op if a profile
> is present and up to date. But I understand there may be limitations in
> Gaia's build system preventing this.

I kept the call to clean as I'm more confident cleaning it especially as we are using the gaia dir two times: from /b2g/gaia/Makefile.in when building the b2g desktop profile. And another time here in the python script. The issue is that in both cases, gaia build system is going to use to same gaia "objdir", the profile directory. But both are built with different setup and I'm not 100% sure gaia build system is going to ensure not mixing up between the two setups.

I'd like to followup on this and share the same profile between b2g desktop and the simulator one.
But this is a lot of moving parts and b2g desktop profile is also used in tests, it will require a bunch of coordination to make it happen.

Comment 19

[Paul Rouget [:paul]]

Comment on attachment 8370757 [details] [diff] [review]
patch

+++ b/b2g/simulator/custom-settings.json
@@ -0,0 +1,6 @@
+{
+  "devtools.debugger.remote-enabled": true,

Isn't it "debugger.remote-mode": "adb-devtools" ?

> subprocess*.js

Collapse the big comment block into a 3 lines header.
Add credits to the json file.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: patch

Address comment 19:
Moved the example to README, reduced comments and fixed the setting.

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8370889 [details] [diff] [review]
patch

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

Mike, I'm sorry to insist, but gps, paul and vingtetun already r+ this patch.
Gps wanted you feedback in comment 9:

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8346029 [details] [diff] [review]
> 
> glandium: I'm interested in your opinion regarding xpi creation. Do we have
> anything better they can use? Also, I figured you want to know about yet
> another extension requirement coming to the build system.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8370889 [details] [diff] [review]
patch

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

You don't need glandium's review. XPI's are hacky. That's our problem, not yours. Just don't be surprised if build peers come in and overhaul the build bits of this patch.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/81f210d3ad52

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=34419684&tree=B2g-Inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/a4260bb263b6

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8370889 [details] [diff] [review]
patch

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

::: b2g/build.mk
@@ +10,5 @@
>  package:
>  	@$(MAKE) -C b2g/installer
> +#ifdef FXOS_SIMULATOR
> +	$(PYTHON) $(srcdir)/b2g/simulator/build_xpi.py $(MOZ_PKG_PLATFORM)
> +#endif

Oops, sorry about the test failure.

I was really expecting this patch to NOT do anything new thanks to these ifdefs...
But it looks like ifdef in Makefiles shouldn't start with `#`!!

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: patch, fixed build.mk ifdef

https://tbpl.mozilla.org/?tree=Try&rev=afe0ee60e1ef

Comment 27

[Paul Rouget [:paul]]

Alex, should we reland this patch?

Comment 28

[Alexandre Poirot [:ochameau]]

Yes, let's land it.
I was waiting for sdk bug 900288 to reach m-c, but it is still not here.
I'll remove subprocess once it's available.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1e85f3f0b525

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e85f3f0b525