Closed Bug 840588 Opened 11 years ago Closed 11 years ago

Create a mach wrapper that searches up from $CWD for a topsrcdir

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(2 files, 5 obsolete files)

It would be nice to have a "mach" command that can be run from any directory and will execute the appropriate mach script for that directory.

I propose writing a wrapper script that can be installed in a system-wide location like /usr/bin/mach (or anywhere else in the user's $PATH).  The wrapper will search upward from the current working directory for a topsrcdir (i.e. any directory with a "mach" executable?), and will then execute the mach script for that srcdir.

For a "just works" experience, we can then include the wrapper in mozilla-build for Windows, and package it for Linux/Mac and install it from the bootstrap script.

Note: We should make sure that if you pass a path to the wrapper script, it's treated as relative to the working directory rather than topsrcdir.  For example, "mach build ." in a subdirectory should build just that subdirectory.
Attached file simple wrapper script (obsolete) —
This is a very, very simple implementation.

It's not super-robust (it can be fooled if it finds some other file named "mach" before finding the actual mach script, for example).  I tried checking for executable permissions, but that did not work on Windows because of http://bugs.python.org/issue10888

It doesn't handle relative paths correctly as suggested in comment 0.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Depends on: 840690
I've been using the attached script for a while now (along with the patch in bug 840690) and it is working great.  My next question is how we can deploy it for other developers.

For Windows users, should we add it to somewhere in the default PATH in mozilla-build?

For Mac and Linux, should we create deb/rpm packages and have bootstrap.py install them?  Or perhaps just check it into the tree and have bootstrap.py create a symlink in /usr/local/bin?
I'm looking to spin a new Mozillabuild to pick up an updated hg. Is it just a matter of putting the attached script in $PATH?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> I'm looking to spin a new Mozillabuild to pick up an updated hg. Is it just
> a matter of putting the attached script in $PATH?

Yes, if you can put the attached script anywhere in the $PATH (e.g. /usr/bin) then it should work out of the box.

Someone with more python/shell/build knowledge than me should probably review the script first.
Comment on attachment 713028 [details]
simple wrapper script

I don't have a plan to check this into the tree yet, but I'd like to get a review of the code itself.
Attachment #713028 - Flags: review?(gps)
Comment on attachment 713028 [details]
simple wrapper script

This seems to get the job done.

You need to preserve mach's exit code. mach should either sys.exit() itself or return the exit code. Just sys.exit(runpy.run_path(mach_path)).

With that, r+.

Please run checking this in past Ted or myself.

Now, that being said, this solution only solves the (common?) case of 1 objdir/mozconfig or the objdir being inside topsrcdir. Consider these scenarios:

* The user has an object directory outside of topsrcdir. They invoke this wrapper. mach isn't found.

* The user has multiple mozconfig files and object directories. They are working from objdirB. This wrapper is invoked without the MOZCONFIG environment variable defined and the mozconfig associated with objdirA is used instead.

These are some of the scenarios I have in mind as part of addressing bug 794506. My current line of thought is we should supply an "activation" script alongside mach (or at least an activation command). You do something like |./activate my-objdir|. A subshell is launched with environment variables set which "bind" the shell to that topsrcdir-objdir pair. "mach" is magically available as a command no matter where you are in the environment.

Furthermore, as you discovered, not all mach commands currently support execution from an arbitrary directory. This is likely a can of worms. We need the build team to supply better APIs for path resolution that are available across all mach commands. It is on my long list of wants :)

I'm not going to blackball this patch because it is a positive step forward. Just know there will be people whose needs it doesn't fulfill.
Attachment #713028 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #6)
> You need to preserve mach's exit code. mach should either sys.exit() itself
> or return the exit code. Just sys.exit(runpy.run_path(mach_path)).

This already works correctly; runpy.run_path executes the code within the same process, so if mach exits then the process will just terminate with that exit code and never returns to the wrapper script.

(And in the case where mach does not exit with an explicit code, run_path returns mach's global dictionary.)

> Now, that being said, this solution only solves the (common?) case of 1
> objdir/mozconfig or the objdir being inside topsrcdir.

Yes, certainly,  My own use case is running mach within the srcdir, since I'm typically editing files there and then running build/test commands.  The wrapper script is perfect for this, but not so good for running within the objdir.

> * The user has an object directory outside of topsrcdir. They invoke
> this wrapper. mach isn't found.
>
> * The user has multiple mozconfig files and object directories. They
> are working from objdirB. This wrapper is invoked without the MOZCONFIG
> environment variable defined and the mozconfig associated with objdirA
> is used instead.

What if the build system automatically wrote a config file to the top of the objdir, containing paths to both the topsrcdir and the mozconfig file (if there is one)?  Then the wrapper script could find this config file and work automatically, without any explicit activation step...

> Furthermore, as you discovered, not all mach commands currently support
> execution from an arbitrary directory. This is likely a can of worms. We
> need the build team to supply better APIs for path resolution that are
> available across all mach commands. It is on my long list of wants :)

Yup.  I can try to help with this work.  I think it would be useful for a variety of commands to have better handling of relative paths, possibly using the approach from bug 840690 (which adds relative path handling to the "build" command, but does not address similar issues for test commands and others).
Depends on: 845620
This adds the topsrcdir path to the mozinfo.json file, along with the $MOZCONFIG variable if it is set.  This lets us figure out the appropriate way to run mach commands if the current directory is within an objdir.
Attachment #718788 - Flags: review?(ted)
Attached file simple wrapper script (v2) (obsolete) —
This updates the wrapper script to to use the MOZCONFIG and topsrcdir values from mozinfo.json if you run it in an objdir.  (This depends on bug 845620.)

Note that this will currently use the MOZCONFIG value from mozinfo.json even if you have a different value set in your environment.  I'm not sure if we want that or not.
Attachment #713028 - Attachment is obsolete: true
Attachment #718789 - Flags: review?(mh+mozilla)
Attachment #718789 - Attachment is patch: false
Comment on attachment 718788 [details] [diff] [review]
add topsrcdir and mozconfig to mozinfo.json

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

I'm okay with this, but make sure this doesn't break Mac universal builds, because mozinfo.json gets unified when it gets put into the test package.
Attachment #718788 - Flags: review?(ted) → review+
Comment on attachment 718789 [details]
simple wrapper script (v2)

In an ideal world, it should do what i think most people would expect when the MOZCONFIG env variable is set, but that'd be more work. Feel free not to do it if you don't feel like it.

However, i think it would be better if the wrapper script was mach itself. That is, if copying $topsrcdir/mach somewhere else just worked, instead of having to rely on a separate wrapper script, and most of the contents of the current mach script would go in a mach-bootstrap.py module or something like that.
Attachment #718789 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #11)
> In an ideal world, it should do what i think most people would expect when
> the MOZCONFIG env variable is set, but that'd be more work. Feel free not to
> do it if you don't feel like it.

Fixed -- the script will no longer overwrite MOZCONFIG if it is already set.

> However, i think it would be better if the wrapper script was mach itself.
> That is, if copying $topsrcdir/mach somewhere else just worked, instead of
> having to rely on a separate wrapper script, and most of the contents of the
> current mach script would go in a mach-bootstrap.py module or something like
> that.

I really like this idea.  Here's the start of a patch to move the logic from my wrapper script into mach itself.  I still need to split the actual library- and command-loading code into a separate bootstrap file.
Attachment #718789 - Attachment is obsolete: true
I'm especially open to feedback about the code organization in this patch.
Attachment #719068 - Attachment is obsolete: true
Attachment #719076 - Flags: review?(gps)
Comment on attachment 719076 [details] [diff] [review]
make the mach script runnable from any directory

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

I /really/ like the concept. However, a goal of the mach core (/python/mach) - read the architecture section in https://developer.mozilla.org/en-US/docs/Developer_Guide/mach - is for it to be a generic command dispatching framework. We may possibly put it on PyPi someday!

With that in mind, putting bootstrap.py in /python/mach is not correct. However, creating /python/mozbuild/mach_bootstrap.py or /build/mach_bootstrap.py or /python/mach_bootstrap.py should be fine! Although, I'm not sure which is most appropriate. Anybody have opinions?

With that change, I'd be pretty close to r+ (would like to test this locally first).
Attachment #719076 - Flags: review?(gps) → feedback+
Moved the bootstrap module to /build/mach_bootstrap.py.
Attachment #719076 - Attachment is obsolete: true
Attachment #719111 - Flags: review?(gps)
(In reply to Mike Hommey [:glandium] from comment #11)

> However, i think it would be better if the wrapper script was mach itself.
> That is, if copying $topsrcdir/mach somewhere else just worked, instead of
> having to rely on a separate wrapper script, and most of the contents of the
> current mach script would go in a mach-bootstrap.py module or something like
> that.

I really like the concept as well and feel things points to something worth exploring.  While I agree with :gps that we shouldn't compromise architecture for immediate gains, I don't think the ideas are necessarily opposed...just, perhaps in need of a bit of thought. -- from the peanut gallery
Comment on attachment 719111 [details] [diff] [review]
make the mach script runnable from any directory (v2)

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

Just nits and the question about str(). Very close. I really want this checked in!

::: mach
@@ +38,5 @@
>      'testing/mochitest/mach_commands.py',
>      'testing/xpcshell/mach_commands.py',
>  ]
>  
> +def bootstrap(topsrcdir):

Nit: dir() is a built-in function. Please don't overwrite built-in symbols!

@@ +44,5 @@
> +    # user-friendly error message rather than a cryptic stack trace on module
> +    # import.
> +    if sys.version_info[0] != 2 or sys.version_info[1] < 7:
> +        print('Python 2.7 or above (but not Python 3) is required to run mach.')
> +        print('You are running Python', platform.python_version())

Nit: Use the "not in" operator. Yes, "in" has higher precedence than "not" so this does the right thing. But, Python has the "not in" operator: use it.

@@ +45,5 @@
> +    # import.
> +    if sys.version_info[0] != 2 or sys.version_info[1] < 7:
> +        print('Python 2.7 or above (but not Python 3) is required to run mach.')
> +        print('You are running Python', platform.python_version())
> +        sys.exit(1)

Why the str()? What doesn't like unicode here?

Also, please comment what the side-effect of setting env['MOZCONFIG'] does: IMO it's not entirely obvious to someone not familiar with how the build system works.
Attachment #719111 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #17)
> Nit: dir() is a built-in function. Please don't overwrite built-in symbols!

Changed the identifier to "dir_path".

> Nit: Use the "not in" operator.

Fixed.

> Why the str()? What doesn't like unicode here?

Unicode strings in os.environ cause "TypeError: environment can only contain strings" in _execute_child from subprocess.py, under Python 2.7.2 on Windows (see also bug 810435).  It looks like this is fixed in Python 2.7.3, so we can remove this workaround once everyone is upgraded.

I added a comment explaining this.

> Also, please comment what the side-effect of setting env['MOZCONFIG'] does:
> IMO it's not entirely obvious to someone not familiar with how the build
> system works.

Done.
Attachment #719111 - Attachment is obsolete: true
Attachment #720044 - Flags: review?(gps)
Comment on attachment 720044 [details] [diff] [review]
make the mach script runnable from any directory (v3)

Nit: os.path.exists(mozinfo_path) is not necessary since the os.path.isfile(mozinfo_path) after it will return False if the file doesn't exist!

Nit: Use b'MOZCONFIG' instead of str('MOZCONFIG') to define non-Unicode strings. Also, str() in Python 2 is binary but Unicode in Python 3! The str(info['mozconfig']) is probably acceptable for now. But, it will fail in Python 3 if info['mozconfig'] is bytes (the equivalent of str in Python 2). Yeah, Python 2 and 3 strings are confusing!

Also, that subprocess.Popen not like unicode has bitten us many times before. We already hacked around it in a few call sites (automatically do .encode('utf-8') on unicode values). All this code will need to get unhacked some day. Ugh.

You can check this directly into m-c if you want. Please use "DONTBUILD (NPOTB)" in the commit message regardless of where you commit to.
Attachment #720044 - Flags: review?(gps) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> I'm okay with this, but make sure this doesn't break Mac universal builds,
> because mozinfo.json gets unified when it gets put into the test package.

Built and passed tests successfully on Mac:
https://tbpl.mozilla.org/?tree=Try&rev=d9609742f59a

(In reply to Gregory Szorc [:gps] from comment #19)
> Nit: os.path.exists(mozinfo_path) is not necessary...
> Nit: Use b'MOZCONFIG' instead of str('MOZCONFIG')...

Fixed both nits and landed on inbound (without DONTBUILD, because of the writemozinfo.py change):

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0dfce25486
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ac597e55b6
https://hg.mozilla.org/mozilla-central/rev/ab0dfce25486
https://hg.mozilla.org/mozilla-central/rev/e7ac597e55b6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
So the end result here is that we should be able to take the "mach" script from m-c and put it in $PATH, and it should Just Work from anywhere in the srcdir?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> So the end result here is that we should be able to take the "mach" script
> from m-c and put it in $PATH, and it should Just Work from anywhere in the
> srcdir?

Right -- and from the objdir, too.  There may be some commands that have some problems because they assume $PWD is the topsrcdir, but we can fix those as we find them.
Hmm, I guess this bug is the reason why I cannot use mach for building comm-central apps like TB and SM anymore (since obviously there is no "mach" in comm-central, only below mozilla/). Too bad. :-(
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #24)
> Hmm, I guess this bug is the reason why I cannot use mach for building
> comm-central apps like TB and SM anymore (since obviously there is no "mach"
> in comm-central, only below mozilla/). Too bad. :-(

Could you file a bug for that, please?  It should be fairly straightforward to fix...
Attachment #720044 - Attachment is patch: true
Depends on: 870073
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.