886162 - mach build testing/ results in a full tree build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Gregory Szorc [:gps]]

/testing doesn't contain a Makefile. The logic for |mach build| will walk parent dirs until it finds a Makefile. In this case, it grabs the top-level Makefile.

Unless you have an idea for better behavior, I'm tempted to mark this WONTFIX.

Comment 2

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #1)
> /testing doesn't contain a Makefile. The logic for |mach build| will walk
> parent dirs until it finds a Makefile. In this case, it grabs the top-level
> Makefile.

I see.

> Unless you have an idea for better behavior, I'm tempted to mark this
> WONTFIX.

A better behavior would be to do what the user would expect, and build only the subdirectories of testing/.

Comment 3

[Gregory Szorc [:gps]]

And what happens if you |mach build services/healthreport/tests/xpcshell|? This directory has no Makefile nor child directories. Are you suggesting we traverse either up or down depending on whether child directories are present?

Comment 4

[(no longer active)]

(In reply to comment #3)
> And what happens if you |mach build services/healthreport/tests/xpcshell|? This
> directory has no Makefile nor child directories. Are you suggesting we traverse
> either up or down depending on whether child directories are present?

Hmm, good question.  I don't know, what I would expect from a sane build utility would be able to take any given path name, and build the minimum set of things in order to take the effect of changes to that directory into account.  Does that sound reasonable?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

IMO it's reasonable to just report an error in the case where walking upwards to find a Makefile takes you to the root - just something simple like "mach can't find a suitable Makefile in directory '%s' - perhaps you meant to build one of its children?".  IMO, no need to elaborate or do anything smarter...

Comment 6

[Gregory Szorc [:gps]]

(In reply to Mark Hammond (:markh) from comment #5)
> IMO it's reasonable to just report an error in the case where walking
> upwards to find a Makefile takes you to the root - just something simple
> like "mach can't find a suitable Makefile in directory '%s' - perhaps you
> meant to build one of its children?".  IMO, no need to elaborate or do
> anything smarter...

That works for me. Code is at https://hg.mozilla.org/mozilla-central/file/default/python/mozbuild/mozbuild/mach_commands.py#l279 if you want to have a go at it!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: avoid accidentally rebuilding the tree

something like this?  I'm really not sure if the 'if os.path.isdir(target)' is necessary or if the simple check for falsey make_dir and make_target is all we need.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 774512 [details] [diff] [review]
avoid accidentally rebuilding the tree

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

Looks good!

::: python/mozbuild/mozbuild/mach_commands.py
@@ +296,5 @@
>  
> +                    # See bug 886162 - we don't want to "accidentally" build
> +                    # the entire tree (if that's really the intent, it's
> +                    # unlikely they would have specified a directory.)
> +                    if os.path.isdir(target) and not make_dir and not make_target:

I /think/ the os.path.isdir() can be omitted.

@@ +300,5 @@
> +                    if os.path.isdir(target) and not make_dir and not make_target:
> +                        print("Can't find a suitable makefile in '%s' - "
> +                              "perhaps you meant to build one of its children?"
> +                              % target)
> +                        return 1

I would rephrase slightly to let people know exactly what happened. e.g.

"The specified directory doesn't contain a Makefile and the first parent directory with one is the root of the tree. I'm assuming you don't want to build the whole tree and have cancelled the build. Please specify a directory with a Makefile or run |mach build| if you want to build the entire tree."

Maybe that can be more concise?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: drop isdir check and updated message

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 775060 [details] [diff] [review]
drop isdir check and updated message

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +299,5 @@
> +                    # unlikely they would have specified a directory.)
> +                    if not make_dir and not make_target:
> +                        print("The specified directory doesn't contain a "
> +                              "Makefile and the first parent with one is the "
> +                              "root of the tree.  Please specify a directory "

Nit: Only need single space after period. FWIW, writing style guidelines formerly recommend double spaces but have since changed in the last 5-10 years.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f22202174cea

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f22202174cea