Closed Bug 840690 Opened 7 years ago Closed 7 years ago

"mach build ." in a subdirectory should build only that directory

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
With this patch, if you run "mach build" in a subdirectory of your objdir or topsrcdir, and pass it a path starting with ".", then mach will expand the path relative to your current subdirectory.

For example, if your working directory is either $objdir/browser or $topsrcdir/browser, then "mach build ." will run pymake in $objdir/browser.  Similarly, "mach build ./metro" will run pymake in $objdir/browser/metro.

Paths that do *not* start with "." are still treated as relative to the objdir, so you can do things like "mach build . toolkit/library" to build in your current directory *and* in in /toolkit/library.

This is very useful with the wrapper script in bug 840588.
Attachment #713090 - Flags: review?(gps)
Comment on attachment 713090 [details] [diff] [review]
patch

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +70,5 @@
> +            # only that subdirectory.
> +            if target.startswith(os.curdir):
> +                abspath = os.path.abspath(target)
> +                if abspath.startswith(os.path.abspath(self.topobjdir)):
> +                        target = os.path.relpath(abspath, self.topobjdir)

Indentation appears to be off here
(In reply to :Ms2ger from comment #1)
> Indentation appears to be off here

Thanks; fixed locally.
Comment on attachment 713090 [details] [diff] [review]
patch

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

Sorry for letting this linger. Please coordinate with Nick Alexander and bug 836208 and ensure the refactoring of the "resolve string to make target" is CWD aware.
Attachment #713090 - Flags: review?(gps)
Attached patch WIP (obsolete) — Splinter Review
This builds on bug 836208 and also applies the same logic to the xpcshell-test command.  It should be easy to adapt any other commands that take filename arguments.  I'm still experimenting with how the code should be organized and how best to write tests for it.
The latest patch depends on the patch in bug 848447.
Depends on: 848447
Attachment #713090 - Attachment is obsolete: true
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Created attachment 721801 [details] [diff] [review]
> WIP
> 
> This builds on bug 836208 and also applies the same logic to the
> xpcshell-test command.  It should be easy to adapt any other commands that
> take filename arguments.  I'm still experimenting with how the code should
> be organized and how best to write tests for it.

There is currently no test coverage inside individual mach_commands.py files. My goal has always been to have as much code as possible live not in mach @Command methods or @CommandProvider classes but in other classes and modules not directly tied to mach. Generally speaking, we haven't done a very good job at this, preferring expedience to architectural correctness. In cases like /testing/xpcshell/mach_commands.py, you'll see a generic XPCShellRunner class in that file. This adheres to the goal of keeping the mach command handlers minimal proxy methods. But it stops short of locating the generic logic in its own file/module and having tests of it.

As we see code duplication among mach_commands.py files, I will start insisting that common logic be located in shared modules and we start establishing test coverage. The problem is the location of those shared modules isn't always clear. mozbuild is primarily intended for build system foo. We certainly could make that overarching to include parts of tests. I dunno. It might be best to drop in #build before you make any big changes and solicit an opinion from Ted and/or me.
Attached patch patch v2 (obsolete) — Splinter Review
This adds a shared class for parsing filesystem path arguments and returning the corresponding srcdir, objdir, or relative paths.  It needs some tests, but I wanted feedback on the code itself first.

I'm not sure about the class and method names; please feel free to suggest improvements.
Attachment #721801 - Attachment is obsolete: true
Attachment #721877 - Flags: review?(gps)
Comment on attachment 721877 [details] [diff] [review]
patch v2

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

This patch makes me happy. Very close to r+. I'd like to see a final patch so I can test locally before giving r+. (I don't doubt you've tested - but I'd feel better if we had more manual test coverage of this before it lands.)

::: python/mozbuild/mozbuild/base.py
@@ +284,5 @@
> +        self.arg = arg
> +        self.topsrcdir = topsrcdir
> +        self.topobjdir = topobjdir
> +
> +    def relpath(self):

Docstring please. Have it answer "relpath from where?"

@@ +289,5 @@
> +        path = self.arg
> +
> +        # If the path starts with "." then treat it as a path relative to the
> +        # current directory.
> +        if path.startswith(os.curdir):

You should pass os.curdir into the constructor.

(I'm sensitive to global state leaking into library APIs like this. If nothing else, it should make testing easier since you won't need to chdir in the tests.)

::: python/mozbuild/mozpack/path.py
@@ -35,5 @@
>  
>  def normpath(path):
>      return posixpath.normpath(normsep(path))
>  
> -

What's with this empty chunk?

::: testing/mochitest/mach_commands.py
@@ +96,1 @@
>                  raise Exception('No manifest file was found at %s.' % test_file)

I think this error message is wrong. Would you mind fixing it while you are here?
Attachment #721877 - Flags: review?(gps) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 721877 [details] [diff] [review]
> > +    def relpath(self):
> 
> Docstring please. Have it answer "relpath from where?"

Done.

> You should pass os.curdir into the constructor.
> 
> (I'm sensitive to global state leaking into library APIs like this. If
> nothing else, it should make testing easier since you won't need to chdir in
> the tests.)

os.curdir is actually a constant; it is the string ".".

However, it could be useful to pass os.getcwd() into the constructor, and use that to construct abspath.  I have added an option to do that, for use in tests.

> ::: python/mozbuild/mozpack/path.py
> What's with this empty chunk?

I made a change to this file, then accidentally removed a blank line when reverting that change.  Dropped this hunk from the patch.

> ::: testing/mochitest/mach_commands.py
> @@ +96,1 @@
> >                  raise Exception('No manifest file was found at %s.' % test_file)
> 
> I think this error message is wrong. Would you mind fixing it while you are
> here?

Done.
Attachment #721877 - Attachment is obsolete: true
Attachment #722907 - Flags: review?(gps)
(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> > You should pass os.curdir into the constructor.
> > 
> > (I'm sensitive to global state leaking into library APIs like this. If
> > nothing else, it should make testing easier since you won't need to chdir in
> > the tests.)
> 
> os.curdir is actually a constant; it is the string ".".

Oh. I got confused there :/ Glad to see you knew what I meant though!
This patch is failing for xpcshell-tests:

cd services/
gps@gps-mbp:~/src/mozilla-central-hg/services$ mach xpcshell-test healthreport/tests/xpcshell/test_healthreporter.js
healthreport/tests/xpcshell/test_healthreporter.js
An xpcshell.ini could not be found for the passed test path. Please select a path whose directory contains an xpcshell.ini file. It is possible you received this error because the tree is not built or tests are not enabled.


This should work, no?
(In reply to Gregory Szorc [:gps] from comment #11)
> This patch is failing for xpcshell-tests:
> 
> cd services/
> gps@gps-mbp:~/src/mozilla-central-hg/services$ mach xpcshell-test
> healthreport/tests/xpcshell/test_healthreporter.js

You'll need to use "." to write a path relative to the current directory.  Either of these commands should work:

  mach xpcshell-test ./healthreport/tests/xpcshell/test_healthreporter.js

  mach xpcshell-test services/healthreport/tests/xpcshell/test_healthreporter.js

This is intentional, so you can do things like "mach build . toolkit/library" to build both the current directory and some other directory.  However, I'm aware it is not immediately obvious...

> healthreport/tests/xpcshell/test_healthreporter.js

Oops, I accidentally left some "print" statements in relpath.  Fixed locally.
Attached patch patch v4 (obsolete) — Splinter Review
Removed debugging print calls.
Attachment #722907 - Attachment is obsolete: true
Attachment #722907 - Flags: review?(gps)
Attachment #722926 - Flags: review?(gps)
(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> (In reply to Gregory Szorc [:gps] from comment #11)
> > This patch is failing for xpcshell-tests:
> > 
> > cd services/
> > gps@gps-mbp:~/src/mozilla-central-hg/services$ mach xpcshell-test
> > healthreport/tests/xpcshell/test_healthreporter.js
> 
> You'll need to use "." to write a path relative to the current directory. 
> Either of these commands should work:
> 
>   mach xpcshell-test ./healthreport/tests/xpcshell/test_healthreporter.js
> 
>   mach xpcshell-test
> services/healthreport/tests/xpcshell/test_healthreporter.js
> 
> This is intentional, so you can do things like "mach build .
> toolkit/library" to build both the current directory and some other
> directory.  However, I'm aware it is not immediately obvious...

Hmmm. This is somewhat unfortunate. To me, the allure of relative directory support is the ability to tab-complete paths in my shell. I suppose we can bolt it on later. We'll have to figure out how to resolve ambiguous targets. But, we'll figure out something, I'm sure.
(In reply to Gregory Szorc [:gps] from comment #14)
> Hmmm. This is somewhat unfortunate. To me, the allure of relative directory
> support is the ability to tab-complete paths in my shell. I suppose we can
> bolt it on later. We'll have to figure out how to resolve ambiguous targets.
> But, we'll figure out something, I'm sure.

You can still tab-complete your relative paths but you need to remember to type "./" manually first.  I agree it's unfortunate and it'll confuse people especially at first, though personally I got used to it after using it regularly.

On the plus side, commands from your history like "mach build browser" will do the same thing no matter where you run them.  And "mach build ." is easy to type.

We could improve the usability by writing a bash-completion script for mach (and probably zsh completion too).  Then it could intelligently tab-complete both curdir-relative and basedir-relative paths, as well as command names and options.  I've been planning to do this for a while, but recently I'm stuck in mysys land where modern versions of bash-completion won't work.  :(
Attached patch patch v5Splinter Review
This strips out the confusing "." distinction, and just treats all relative paths as relative-to-cwd.  On IRC we discussed various possible ways to handle use cases like "mach build toolkit/library" but agreed that those could be put off for later.

This also adds basic tests for the PathArgument class.
Attachment #722926 - Attachment is obsolete: true
Attachment #722926 - Flags: review?(gps)
Attachment #724001 - Flags: review?(gps)
Comment on attachment 724001 [details] [diff] [review]
patch v5

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

LGTM. We can iterate on additional path finding semantics later.
Attachment #724001 - Flags: review?(gps) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/effe4b6bca9c
OS: Windows 8 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/effe4b6bca9c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.