Closed Bug 867817 Opened 10 years ago Closed 7 years ago

Add mach to B2G

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jgriffin, Assigned: ahal)

References

Details

(Whiteboard: [mach])

Attachments

(1 file, 2 obsolete files)

mach.diff
9.04 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review 
We'd like to use mach as a front-end for running tests in B2G; the current mechanism relies on an unmaintainable patchwork of shell scripts and hard-to-intuit command-line args.

In order to do this, I think we'd need a slightly modified version of 'mach' in the B2G repo, and then some modifications to the bootstrapping code to be B2G-aware.

I'll try to put together a POC.
I think you should put this in the b2g component somewhere.

Also, you may be interested in bug 865051. Once mach is part of PyPI, I shouldn't have to care what B2G does with it. Only if B2G wants to integrate with m-c's mach commands and m-c needs to change something will I need to be interested. Of course, I will be interested to see how it goes. But I shouldn't have to be!
Whiteboard: [mach]
Component: mach → General
Product: Core → Boot2Gecko
I'm not sure how the B2G repo structure looks, but I know the wrapper script (bug 840588) has some issues if you build inside a srcdir that's inside another srcdir (like the mozilla directory inside of comm-central); you may need to deal with that for B2G.
I just added mach to PyPI. I'm currently working to abstract away the Firefox/m-c specific bits to make it a truly generic CLI dispatching framework.

If nothing else, the first iteration of mach on B2G could simply wrap the existing .sh scripts with a unified interface. Over time, those .sh scripts could be rewritten in Python if so desired. mach doesn't really care, however.
ahal is going to take this bug, just for getting the test runners working via mach (no support for making builds for the time being)
Assignee: jgriffin → ahalberstadt
I see two options:
1) The easiest thing to do would be to basically just start over. Make a new bootstrap script and a completely new set of mach_commands.py scripts.

2) But seeing as we have access to a gecko source dir after running confing.sh, it seems it would be better if we could re-use a lot of the mach commands in the gecko tree. I.e the only difference between running mochitests on b2g vs desktop is a slightly different command line. This would be a bit more involved and would likely require some changes to mozbuild since it doesn't seem to work very well in B2G land (though I guess the mach driver could just explicitly create a MozbuildObject).

gps, any thoughts on B2G/m-c mach integration?
Flags: needinfo?(gps)
B2G will probably need its own mach_bootstrap.py. Ideally, it will be able to pull in the mach_bootstrap.py from the gecko source dir to supplement b2g's own mach_commands.py files.

We should aim to reuse the mach_commands.py and commands from gecko as much as possible - especially for tests: we already define how to invoke the test harnesses in too many places. If you need to add "if app is b2g" to the mach_commands.py files, I guess we'll have to do that.

FWIW, I'd like as much core logic as possible to live outside of mach_commands.py files - inside standalone, importable modules. mach_commands.py should only contain dispatching logic that takes input arguments, loads a module, invokes it, and formats output. We are pretty far from this ideal. If you feel you need to extract logic from mach_commands.py to share it with b2g, by all means.

If you need support for a mach command decorator that only works in certain environments (currently all commands in a file get loaded), we can look into that.

What I'm trying to say is mach can adapt to suit needs.
Flags: needinfo?(gps)
I have the actual mach driver/bootstrap pieces pretty much finished. They'll detect if the gecko source tree is there and load up the commands found in the gecko srcdir. Now I'm trying to actually get the gecko mach_commands to run. Basically the relevant file structure is:

\B2G
  .config
  mach (driver)
  \build
    mach_b2g_bootstrap.py
  \python
    \mach (core)
  \gecko
    \build
      mach_bootstrap.py
    <various mach commands>
  \gonk-misc
    default-gecko-config (mozconfig)

In terms of mozbuild, what is considered topsrcdir here, B2G or gecko? Obviously we'll need some kind of mozbuild for B2G. There are three solutions I can think of:

1) I could modify the existing mozbuild module to be "B2G aware", but this has complications. For example the default mozconfig gets it's MOZ_OBJDIR value by sourcing the .config file from the build.sh script. I.e the read_mozconfig() method won't find the objdir. Plus mozbuild will only exist if a ./config.sh was run.

2) I could subclass the existing mozbuild from within the B2G directory. Then I can modify the b2g specific parts without worry. Though again, if ./config.sh wasn't run then the original mozbuild won't exist. I could possibly fallback to a limited implementation of mozbuild in this case.

3) I could fork mozbuild and create a module that was designed specifically for B2G and has little to do with the gecko version.

I'm leaning towards either 2 or 3. Gps, do you have any thoughts on this?
Flags: needinfo?(gps)
First reaction is I prefer the subclass route. Although, the API in mozbuild isn't very stable, so it might be fragile and we might break b2g semi-frequently.

Since the common case has gecko/m-c present, I don't think you should worry about it too much. You can always hack up the mach driver or bootstrap script to check for gecko's presence and short circuit if a repo clone/pull is needed.

I would not fork mozbuild. I would consider "B2G aware" enhancements to code in m-c. I would prefer they be tightly scoped, if possible. Keep in mind the problem of integrating m-c's build tools is a generic one - clients like comm-central have the same problem and it would be nice to solve the problems for everyone, not just b2g.
Flags: needinfo?(gps)
Depends on: 901972
Depends on: 902494
Depends on: 902496
Depends on: 902581
Depends on: 908874
Depends on: 909888
Pull request: https://github.com/mozilla-b2g/B2G/pull/271

This is mostly just copying mach core over from m-c. The two files of interest are "mach" and "python/mach_b2g_bootstrap.py".

Gps, feel free to ignore this feedback, just flagged you in case you were interested. This should have zero impact on mozilla-central.

Once this patch lands, developers should pick up the mochitest/reftest mach command patches from m-c when it gets updated, and will be able to run them if they have an emulator build.
Attachment #796787 - Flags: review?(jgriffin)
Attachment #796787 - Flags: feedback?(gps)
Why are we copying mach instead of using the copy in gecko?
Seems like you might want to use mach before pulling Gecko--maybe even to pull in Gecko.
(In reply to :Ms2ger from comment #11)
> Seems like you might want to use mach before pulling Gecko--maybe even to
> pull in Gecko.

Nope. That's not a typical scenario - repo is used to pull all repositories. You can tell it to build your own m-c repo, but repo will always pull its own copy of Gecko.
(In reply to Michael Wu [:mwu] from comment #10)
> Why are we copying mach instead of using the copy in gecko?

Because we aren't guaranteed to have a gecko directory available (e.g config.sh hasn't been run). I recently patched mach to allow filtering commands at runtime, so this setup will be able to dynamically tell which commands are available based on the current configuration of a user's source tree. E.g no gecko directory? Mochitest/reftest/xpcshell commands won't show up.

This will also open the door for many other possibilities. For example repo is a python script which we currently invoke from shell scripts. Instead, we could just hook it up directly to mach and provide richer output (like how desktop firefox builds, we could have the currently dowloading repo displayed at the bottom).
(In reply to Andrew Halberstadt [:ahal] from comment #13)
> Because we aren't guaranteed to have a gecko directory available (e.g
> config.sh hasn't been run). I recently patched mach to allow filtering
> commands at runtime, so this setup will be able to dynamically tell which
> commands are available based on the current configuration of a user's source
> tree. E.g no gecko directory? Mochitest/reftest/xpcshell commands won't show
> up.
> 

I don't understand when that would be a plausible scenario.

> This will also open the door for many other possibilities. For example repo
> is a python script which we currently invoke from shell scripts. Instead, we
> could just hook it up directly to mach and provide richer output (like how
> desktop firefox builds, we could have the currently dowloading repo
> displayed at the bottom).

That's not a convincing possibility.
There might be mach commands we'd want to implement specific to gaia too, that don't have anything to do with gecko.

Do you have any particular reasons to your objections? Mach is an incredibly useful tool that will allow us to make the lives of everyone working on B2G a lot easier. And those who don't want to use it, don't have to. It won't break any existing workflows whatsoever.
Also, we can't infer the location of the B2G directory from an arbitrary gecko src dir; we need a mach script in the B2G directory itself for this.

We use the B2G directory for many purposes within the mach scripts.
(In reply to Andrew Halberstadt [:ahal] from comment #15)
> There might be mach commands we'd want to implement specific to gaia too,
> that don't have anything to do with gecko.
> 

In the cases where someone has a gaia project without a gecko repo, they also don't have a B2G repo. The gaia repo is either used standalone or as part of a checkout via repo in the B2G directory, which would also bring in the gecko repo.

> Do you have any particular reasons to your objections? Mach is an incredibly
> useful tool that will allow us to make the lives of everyone working on B2G
> a lot easier. And those who don't want to use it, don't have to. It won't
> break any existing workflows whatsoever.

We shouldn't have another copy of mach in the B2G repo if we don't need it.
(In reply to Jonathan Griffin (:jgriffin) from comment #16)
> Also, we can't infer the location of the B2G directory from an arbitrary
> gecko src dir; we need a mach script in the B2G directory itself for this.
> 

What do you need from the B2G directory? It's worth nothing that the B2G directory and the repo checkout root aren't necessarily equivalent in all configurations. The repo checkout root is saved during gecko configuration if that is what you're interested in ($ANDROID_SOURCE).
(In reply to Jonathan Griffin (:jgriffin) from comment #16)
> Also, we can't infer the location of the B2G directory from an arbitrary
> gecko src dir; we need a mach script in the B2G directory itself for this.
> 
> We use the B2G directory for many purposes within the mach scripts.

No this isn't quite true. The mach driver and bootstrap script will handle all of the b2g specific configuration and *will* need to be checked into the b2g repo. The mach core python module can conceivably come from anywhere (gecko, system packages, virtualenv, etc).

(In reply to Michael Wu [:mwu] from comment #17)
> We shouldn't have another copy of mach in the B2G repo if we don't need it.

I agree with this statement in general. I think what we are arguing over is whether we need it. There are several reasons I'd like to have mach available out of the box:

1) Mach is supposed to "just work" all the time. It is really hard to do this if we can't even guarantee that mach itself is available. The mach driver will be checked into the repo, so developers will get confused when running it just produces an error message. The whole point to mach is to make things stupid simple to do, and we aren't off to a good start if the first thing a dev see's is an error message.

2) Not including mach out of the box limits the type of commands we'll be able to implement. If we decide now that mach core should not live in B2G, I maintain that it will never live in B2G. And then we're going to think of all these cool ideas for various mach commands but run up against this wall. You said that a mach command for 'repo' is not very compelling, but I disagree. I think mach can bring many benefits to the existing commands, not the least of which is built-in discoverability and documentation.

3) I don't really see much of a downside to including it. I don't think mach core will need to be updated much. I'm thinking we would maybe want to sync it on the order of once a quarter, if that. Also, mach and mach commands would most likely fall under the responsibility of the a-team (unless someone else wanted to step up).

All that being said, I do understand where you're coming from. If you have more specific concerns, I'd love to hear them.
(In reply to Andrew Halberstadt [:ahal] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #17)
> > We shouldn't have another copy of mach in the B2G repo if we don't need it.
> 
> I agree with this statement in general. I think what we are arguing over is
> whether we need it. There are several reasons I'd like to have mach
> available out of the box:
> 
> 1) Mach is supposed to "just work" all the time. It is really hard to do
> this if we can't even guarantee that mach itself is available. The mach
> driver will be checked into the repo, so developers will get confused when
> running it just produces an error message. The whole point to mach is to
> make things stupid simple to do, and we aren't off to a good start if the
> first thing a dev see's is an error message.
> 

Our current tools generally point users in the right direction (run config.sh) if their repo isn't configured correctly. Making sure people figure out to get things checked out isn't hard.

> 2) Not including mach out of the box limits the type of commands we'll be
> able to implement. If we decide now that mach core should not live in B2G, I
> maintain that it will never live in B2G. And then we're going to think of
> all these cool ideas for various mach commands but run up against this wall.
> You said that a mach command for 'repo' is not very compelling, but I
> disagree. I think mach can bring many benefits to the existing commands, not
> the least of which is built-in discoverability and documentation.
> 

Commands in the B2G are inherently discoverable - they are executable files/scripts that say what they do on the label. Where appropriate, commands provide context specific documentation. We can certainly add more documentation though, but that's orthogonal to whether mach is used or not.

What other benefits does mach bring?
The reason we're specifically putting mach into B2G is to ease test execution.  Running tests on B2G builds is too difficult and error-prone right now.  There are a scattering of shell scripts, but these really aren't flexible or maintainable enough.  Mach makes them much more sane, and we have long terms plans to tie them into the same mechanism that's used to run tests in buildbot, which is another win.

We intend to land enough of mach in B2G to support this goal.  If there are parts of mach which we could get from gecko instead of copying, we should do that, but there will be some files that we'll need to land directly in B2G.
(In reply to Michael Wu [:mwu] from comment #20)
> Commands in the B2G are inherently discoverable

I disagree. I don't know how many times I accidentally ended up with a b2g18 repo because I forgot to put 'BRANCH=master' at the beginning (and if I still make this mistake, it's even more confusing for newcomers). Sure it's documented somewhere, or they could ask in #b2g.. but why do that when they could just run './mach help config' and every possible parameter would be listed, with an explanation of what it does and the default behaviour? The B2G repo is littered with random environment variables that affect how the scripts run, and these are not discoverable at all. And while the shell scripts themselves have self documenting names, a longer paragraph style description surely wouldn't hurt. To you and I, the process of configuring and building b2g is easy. To a new contributor however, it is not.. and every hurdle we make them cross is another chance for them to lose interest.

> What other benefits does mach bring?

One big benefit I haven't mentioned yet. Mach is written in python, so are all our automation libraries and tooling. Things like the system resource monitor, heka, structured logging, psutil etc.. Much of this stuff will be integrated by default. The ones that aren't will be a snap for mach_commands to implement. For example, if we wanted to track our system resource usage while performing builds, mach would make this easy.

I should probably let gps chime in here. He has done lots of blog posts on the topic (see http://gregoryszorc.com/blog/category/mach/ if you interested in learning more about mach and its motivations).

For me, it's mostly just about making the user experience (starting with tests, but not necessarily ending there) as easy as possible. And like with desktop Firefox, I have no intention of breaking any existing workflows or shoving mach down the throats of anyone who doesn't want to use it.
(In reply to Andrew Halberstadt [:ahal] from comment #22)
> (In reply to Michael Wu [:mwu] from comment #20)
> > Commands in the B2G are inherently discoverable
> 
> I disagree. I don't know how many times I accidentally ended up with a b2g18
> repo because I forgot to put 'BRANCH=master' at the beginning (and if I
> still make this mistake, it's even more confusing for newcomers). Sure it's
> documented somewhere, or they could ask in #b2g.. but why do that when they
> could just run './mach help config' and every possible parameter would be
> listed, with an explanation of what it does and the default behaviour? The
> B2G repo is littered with random environment variables that affect how the
> scripts run, and these are not discoverable at all. And while the shell
> scripts themselves have self documenting names, a longer paragraph style
> description surely wouldn't hurt. To you and I, the process of configuring
> and building b2g is easy. To a new contributor however, it is not.. and
> every hurdle we make them cross is another chance for them to lose interest.
> 

Yeah environmental variables are pretty horrible. I agree but these things are entirely fixable. BRANCH=master is a horrible hack from when we needed to force people to develop on 1.1 - it's long past its useful life.

> > What other benefits does mach bring?
> 
> One big benefit I haven't mentioned yet. Mach is written in python, so are
> all our automation libraries and tooling. Things like the system resource
> monitor, heka, structured logging, psutil etc.. Much of this stuff will be
> integrated by default. The ones that aren't will be a snap for mach_commands
> to implement. For example, if we wanted to track our system resource usage
> while performing builds, mach would make this easy.
> 

Do you have other examples? System resource usage does not sound like a strong example.

For the testing stuff you're doing (I took a quick glance at the dependent bugs), mach seems to make sense, but I still haven't heard of examples outside of that which makes mach worth integrating into the B2G repo.

> For me, it's mostly just about making the user experience (starting with
> tests, but not necessarily ending there) as easy as possible.

I agree. We should focus on that rather than mach specifically.
mach is more powerful than the current solution of shell scripts because mach and mach commands - being written in Python - allow much, much better user stories to play out. For example, you can hook into mach's command dispatching code path to "nag" users with helpful tips. While not yet implemented, we eventually want mach to nag users to periodically run |mach bootstrap| and |mach mercurial-setup| to help ensure their system configuration is up to date. Mach commands can also hook into test harnesses natively. This is huge. I've seen patches where the mach command fires up a web server (powered by Python), and test results are piped through a web socket and results are rendered in rich HTML. Doing this in shell is complicated (shell-level process management is absurdly complicated to do right). With Python, it "just works." Mach commands - being written in Python - more easily run anywhere. We are actively moving mozilla-central away from shell for this very reason. In our ideal world, we don't need a shell and users can build on Windows without the monolithic MozillaBuild environment we currently require - just a working Python interpreter.

As an aside, most of the arguments the mach-non-supporters are using are arguments I heard when I initially wrote mach and tried to land it in m-c. Fast forward a year and people are now saying things "I can't believe we relied on shell scripts and arcane make targets for so long - mach is so much more intuitive" and nearly all the MDN documentation has been updated to reference mach commands instead of the former way of doing things. I hypothesize people grow accustomed to the "old way", forget how bad it is, and are skeptical of change. Is this Stockholm Syndrome?
(In reply to Michael Wu [:mwu] from comment #23)
> For the testing stuff you're doing (I took a quick glance at the dependent
> bugs), mach seems to make sense, but I still haven't heard of examples
> outside of that which makes mach worth integrating into the B2G repo.

The reason I'm trying so hard to push for mach being checked into B2G isn't because I want to overhaul the entire build system overnight. But having mach available will let us make many small improvements, and over time the user story will get better and better.

Parts of the b2g build system are already using python (e.g repo), so I don't really understand what your opposition here is. Could you please clarify exactly what you are afraid is going to happen? (I'm looking for something more specific than 'we shouldn't use it if we don't need to').

I'm just trying to better understand where you're coming from.
(responding to gps)

Note that make targets cannot be entirely eliminated - the majority of our build system is inherited from Android and cannot realistically be changed without agreement from the upstream (Google).

Windows is not a concern for the B2G repo. Device builds simply do not support Windows. Windows as a host system is extremely unlikely to be supported unless upstream does it.

Generally though, I agree with eliminating the use of arbitrary build targets as a means of running scripts or programs. The B2G repo does not (and cannot) add build targets for any functionality. (gaia does! talk to them.) Also, shell scripts are certainly not the right tool for many things. Python scripts are accepted and currently used in the B2G repo. Consider it a collection of useful tools, some of which interact with the actual build systems.
(In reply to Andrew Halberstadt [:ahal] from comment #25)
> (In reply to Michael Wu [:mwu] from comment #23)
> > For the testing stuff you're doing (I took a quick glance at the dependent
> > bugs), mach seems to make sense, but I still haven't heard of examples
> > outside of that which makes mach worth integrating into the B2G repo.
> 
> The reason I'm trying so hard to push for mach being checked into B2G isn't
> because I want to overhaul the entire build system overnight. But having
> mach available will let us make many small improvements, and over time the
> user story will get better and better.
> 
> Parts of the b2g build system are already using python (e.g repo), so I
> don't really understand what your opposition here is. Could you please
> clarify exactly what you are afraid is going to happen? (I'm looking for
> something more specific than 'we shouldn't use it if we don't need to').
> 

The use of python isn't what's being objected to. We have tools besides repo which are written in python. The small improvements you're referring to do not seem to require mach, so I do not see why we should include it.
Is this a misunderstanding between repositories? Perhaps we should be asking to add mach to the gaia repo instead of the B2G repo? I think the underlying request is "Firefox OS developers should use mach, not random shell scripts, for performing common developer tasks [because it is a more intuitive and easier to maintain solution]." As long as we accomplish that, everyone wins.
Gaia may be a more appropriate place for mach. I don't do much work on the gaia build system other than to glue it to the Android build system, so I don't know how much I can comment. However, the testing stuff ahal is working on is stored in gaia or gecko rather than this B2G repo.

ahal was also talking about user interface problems with some of the scripts in the B2G repo, but running ./config.sh vs mach config does not change the core UI problems to be fixed (like using arguments instead of env variables).
(In reply to Michael Wu [:mwu] from comment #23)
> > For me, it's mostly just about making the user experience (starting with
> > tests, but not necessarily ending there) as easy as possible.
> 
> I agree. We should focus on that rather than mach specifically.

I don't really understand this line of reasoning. We've already got mach, it's working well for mozilla-central. Why should we invent something new for B2G rather than extend the thing we already have working? Specifically, for the cases ahal cares about right now (running tests), we have mach commands in mozilla-central that people use, so making similar commands work in the B2G repo seems like the right thing to do. Why make developers use something completely different just because they're working on B2G?

I don't think anyone's saying we should wrap everything in mach, or change the way things are currently done, just adding mach on as an easier-to-use interface like we have in m-c.
(In reply to Michael Wu [:mwu] from comment #29)
> ahal was also talking about user interface problems with some of the scripts
> in the B2G repo, but running ./config.sh vs mach config does not change the
> core UI problems to be fixed (like using arguments instead of env variables).

Actually it would. It would be trivial to write a mach command that wraps config.sh and lets you run "mach config emulator --branch master" (And 'mach help config' to see that the parameter exists). But this is neither here nor there, let's take a step back.

In comment 19 I listed two reasons I wanted mach in the B2G repo. So far we've only been arguing about reason #2. You did reply to reason #1 in comment 20, but you missed my point. I was referring to ease of use for the mach testing commands I'm implementing, not the pre-existing ones.

So back to #1.. even if I did use mach core from gecko, I still need to check in the mach driver (the 'mach' file) and the mach bootstrap script ('python/mach_b2g_bootstrap.py'). This isn't really negotiable, even the gecko specific test commands will not work without these two files in the B2G repo. Now we have a situation where there is a command in the top level of the B2G repository that does nothing except spew an error message out of the box. Every developer who comes to B2G from mozilla-central, will expect to be able to run "mach help" to try and figure out how to set things up (as this is what they are used to), and they will be disappointed when instead all they see is "Mach not found".

I spend a lot of my time hand holding people from the DOM, layout and graphics teams to try and get them to build an emulator and run mochitests/reftests to test out problems locally. I am probably in the best position to say that current steps we make them go through is awful. My goal for this quarter and probably next is to make this less awful. If mach core *isn't* checked into the B2G repo, the best I can do is a confusing half-baked solution that is inconsistent with how mozilla-central works. This is not something I'm willing to do without great impetus not to.

So let's just say you are right, and mach doesn't have many compelling things it can add to the pre-existing commands. In that case I still want to check it in. I don't have any intentions of replacing the current system, or breaking any existing workflows.

Can we make a compromise? I'll keep mach away from build commands, but I'd still like it in the repo to avoid the pain point of having a command that does nothing but error out. Is this an acceptable compromise to you? I'd greatly appreciate it.
Actually, I just thought of another reason why we need mach checked in. If we depend on the mach in gecko we won't be able to run tests against b2g18 (and we'll run into a whole bunch of similar version mismatch problems down the road). The b2g test commands depend on a whole bunch of features that are only in the mozilla-central copy of mach.

In light of this, the large number of ways we listed that mach will benefit us, and the lack of reasons as to why we shouldn't include mach, I'd like to proceed with this patch as is.
Comment on attachment 796787 [details] [diff] [review]
Patch 1.0 - add mach core to B2G, create b2g bootstrap script

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

This technically looks good. It would look less scary if you separated the mach core bulk import from the unique code for this repo. I also wish we didn't have to keep a near shallow copy of the bootstrap scripts, but meh. Not much we can do if m-c isn't pulled in from source yet.
Attachment #796787 - Flags: feedback?(gps) → feedback+
Attached patch Mach bootstrap only (obsolete) — — Splinter Review 
I had a vidyo chat with mwu about this, and we've decided for now just to go with the minimal bootstrap which is required to enable mach test commands.  We'll have another discussion during the b2g work week, but I'd like to land this before then so that developers have an easier way of running tests at the work week.

(This patch is courtesy of ahal, who e-mailed it to me, so I'm not reviewing my own patch...)
Attachment #796787 - Attachment is obsolete: true
Attachment #796787 - Flags: review?(jgriffin)
Attachment #798998 - Flags: review?(jgriffin)
Attachment #798998 - Flags: feedback?(gps)
Comment on attachment 798998 [details] [diff] [review]
Mach bootstrap only

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

::: python/mach_b2g_bootstrap.py
@@ +144,5 @@
> +        sys.path.insert(0, gecko_bootstrap_dir)
> +        import mach_bootstrap
> +
> +        global SEARCH_PATHS
> +        global MACH_MODULES

I'd use imp.load_source() to import the module. This will give you more control over the import and will allow direct access to the variables without polluting sys.path or the namespace.

@@ +170,5 @@
> +        if os.path.isfile(config_path):
> +            with open(config_path, 'r') as fh:
> +                for line in fh.readlines():
> +                    key, value = line.split('=', 1)
> +                    os.environ[key] = value

This arguably isn't necessary for the bootstrapper. But if it's necessary to configure the b2g environment, then it's necessary.
Attachment #798998 - Flags: feedback?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #35)
> Comment on attachment 798998 [details] [diff] [review]
> Mach bootstrap only
> 
> Review of attachment 798998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mach_b2g_bootstrap.py
> @@ +144,5 @@
> > +        sys.path.insert(0, gecko_bootstrap_dir)
> > +        import mach_bootstrap
> > +
> > +        global SEARCH_PATHS
> > +        global MACH_MODULES
> 
> I'd use imp.load_source() to import the module. This will give you more
> control over the import and will allow direct access to the variables
> without polluting sys.path or the namespace.

Fixed.

> 
> @@ +170,5 @@
> > +        if os.path.isfile(config_path):
> > +            with open(config_path, 'r') as fh:
> > +                for line in fh.readlines():
> > +                    key, value = line.split('=', 1)
> > +                    os.environ[key] = value
> 
> This arguably isn't necessary for the bootstrapper. But if it's necessary to
> configure the b2g environment, then it's necessary

We'd only need to do it this way if we were going to wrap config.sh/build.sh directly.  It's not clear whether we'll be doing that now, but it's harmless so I'll leave it in.
Attached patch mach.diff — — Splinter Review 
Use load_module per gps' suggestion, and move mach_bootstrap.py from /python to /tools.
Attachment #798998 - Attachment is obsolete: true
Attachment #798998 - Flags: review?(jgriffin)
Attachment #799165 - Flags: review?(jgriffin)
Comment on attachment 799165 [details] [diff] [review]
mach.diff

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

Works well with both default gecko and GECKO_PATH.  Currently, mochietst-remote is the only target supported, but with this patch we can easily add support for other targets.
Attachment #799165 - Flags: review?(jgriffin) → review+
bootstrap only landed as https://github.com/mozilla-b2g/B2G/commit/9d8610ea0f17861dbe07dc76f4bb4bd1013b00a0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
FYI:

./mach help
usage: mach [global arguments] command [command arguments]

Global Arguments:
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.

Testing:
  Run tests.

  crashtest-remote      Run a remote crashtest.
  mochitest-remote      Run a remote mochitest.
  reftest-remote        Run a remote reftest.

More test targets coming soon.
I'd like to keep this open as the original intent of this bug was to get mach core checked in which hasn't happened yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 10 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.