./mach build <subdirectory> doesn't recurse for C/C++ compilations

RESOLVED FIXED in mozilla34

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Away for a while, Assigned: glandium)

Tracking

unspecified
mozilla34
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
I don't know when this broke, but `./mach build editor` for example no longer seems to try to compile anything under editor/, but it does however seem to run the export and linking stages of the build.
(Assignee)

Comment 1

4 years ago
See bug 1052516 comment 2
(Reporter)

Comment 2

4 years ago
(In reply to Mike Hommey [:glandium] from comment #1)
> See bug 1052516 comment 2

Neither of those workarounds are helpful in a whole bunch of cases, such as when the build config has changed (which throws off build binaries) or when having made changes to several sub-directories of a top-level directory.

Is this an intentional breakage?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 3

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > See bug 1052516 comment 2
> 
> such as when the build config has changed (which throws off build binaries)

how so?

> or when having made changes to several sub-directories of a top-level directory.

in which case you want the binaries target anyways.

> Is this an intentional breakage?

Yes.
Flags: needinfo?(mh+mozilla)
(Reporter)

Comment 4

4 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #2)
> > (In reply to Mike Hommey [:glandium] from comment #1)
> > > See bug 1052516 comment 2
> > 
> > such as when the build config has changed (which throws off build binaries)
> 
> how so?

By editing a moz.build file for example.

> > or when having made changes to several sub-directories of a top-level directory.
> 
> in which case you want the binaries target anyways.

I don't understand.  ./mach build binaries editor doesn't work either.

> > Is this an intentional breakage?
> 
> Yes.

Well, was this announced anywhere?  Why don't we just remove the ./mach build <subdirectory> syntax completely?  Keeping an existing command which may or may not give you what you want sounds terrible.
(Reporter)

Comment 5

4 years ago
(Also note that we're talking about breaking something which has worked for years, and people *do* rely on it.)
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 6

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #2)
> > > (In reply to Mike Hommey [:glandium] from comment #1)
> > > > See bug 1052516 comment 2
> > > 
> > > such as when the build config has changed (which throws off build binaries)
> > 
> > how so?
> 
> By editing a moz.build file for example.

How does that make it work less than mach build foo?

> > > or when having made changes to several sub-directories of a top-level directory.
> > 
> > in which case you want the binaries target anyways.
> 
> I don't understand.  ./mach build binaries editor doesn't work either.

./mach build binaries.

> > > Is this an intentional breakage?
> > 
> > Yes.
> 
> Well, was this announced anywhere?  Why don't we just remove the ./mach
> build <subdirectory> syntax completely?

Because it's still useful for all the cases where recursion is not required, and the cases that don't involve compilation.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> (Also note that we're talking about breaking something which has worked for
> years, and people *do* rely on it.)

For some value of "worked". And people were relying on it for years for compilation because mach build binaries didn't exist.

Although I guess we could reinstate it for incremental builds, but i'm not thrilled by keeping footguns.

Arguably, we could have a rule to just build the .o files instead of also linking, which I guess is what some people are after when running mach build foo, but I'm not sure how useful that would be.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 7

4 years ago
Also note that thanks to dumbmake, mach build foo doesn't even have a reliable behavior. it may or may not rebuild libxul, depending on what foo is.
(Reporter)

Comment 8

4 years ago
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #4)
> > (In reply to Mike Hommey [:glandium] from comment #3)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #2)
> > > > (In reply to Mike Hommey [:glandium] from comment #1)
> > > > > See bug 1052516 comment 2
> > > > 
> > > > such as when the build config has changed (which throws off build binaries)
> > > 
> > > how so?
> > 
> > By editing a moz.build file for example.
> 
> How does that make it work less than mach build foo?

It works if the moz.build is somewhere under the subdir.

> > > > Is this an intentional breakage?
> > > 
> > > Yes.
> > 
> > Well, was this announced anywhere?  Why don't we just remove the ./mach
> > build <subdirectory> syntax completely?
> 
> Because it's still useful for all the cases where recursion is not required,
> and the cases that don't involve compilation.

Well, it is useful for the cases where it does the right thing.  What if in another case it ends up doing the wrong thing by producing the incorrect build?

> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > (Also note that we're talking about breaking something which has worked for
> > years, and people *do* rely on it.)
> 
> For some value of "worked". And people were relying on it for years for
> compilation because mach build binaries didn't exist.
> 
> Although I guess we could reinstate it for incremental builds, but i'm not
> thrilled by keeping footguns.
> 
> Arguably, we could have a rule to just build the .o files instead of also
> linking, which I guess is what some people are after when running mach build
> foo, but I'm not sure how useful that would be.

But isn't what we have right now for ./mach build subdir a footgun as well, but in a different way?  Why is one class of footguns better than the other.
(Assignee)

Comment 9

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #4)
> > > (In reply to Mike Hommey [:glandium] from comment #3)
> > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > comment #2)
> > > > > (In reply to Mike Hommey [:glandium] from comment #1)
> > > > > > See bug 1052516 comment 2
> > > > > 
> > > > > such as when the build config has changed (which throws off build binaries)
> > > > 
> > > > how so?
> > > 
> > > By editing a moz.build file for example.
> > 
> > How does that make it work less than mach build foo?
> 
> It works if the moz.build is somewhere under the subdir.

Are you saying that mach build binaries doesn't take changes to moz.build? if so, then file a bug.

> But isn't what we have right now for ./mach build subdir a footgun as well,
> but in a different way?  Why is one class of footguns better than the other.

Current mach build subdir is a smaller footgun than what it used to be. It *will* be removed in due time.
(Reporter)

Comment 10

4 years ago
(In reply to Mike Hommey [:glandium] from comment #9)
> > But isn't what we have right now for ./mach build subdir a footgun as well,
> > but in a different way?  Why is one class of footguns better than the other.
> 
> Current mach build subdir is a smaller footgun than what it used to be. It
> *will* be removed in due time.

Can you at least announce this on dev.platform so that other people won't waste their time figuring out why they are getting incorrect builds?
Flags: needinfo?(mh+mozilla)
Duplicate of this bug: 1057027
I just ran into this as well during a './mach build toolkit/mozapps/update'. https://hg.mozilla.org/mozilla-central/rev/4e2a92a90646 seems to be the cset that introduced this to m-c.

My particular use case was to update test files for updates (all .js files), so running './mach build toolkit/mozapps/update' literally took 2-3 seconds. Running a full './mach build' takes roughly 1.5-2 minutes now. Is there another (faster) alternative to just './mach build'?

I'm all for avoiding footguns, but increasing compile times from seconds to minutes seems like a heavy cost to pay...
I hadn't tried running ./mach build binaries, since I assumed (due to the name) that this would only work for changes to .c, .cpp, .mm and similar files. Now that I tried it, it does seem to work for .js files as well with a run time of about 6 seconds. Not as good as the 2 seconds with ./mach build toolkit/mozapps/update, but I'm fine with that.
Two points:

* I believe we all agree we should retire dumbmake as soon as possible.

* Android devs use |mach build mobile/android| extensively.  Removing building subdirectories without providing a tangibly better, equally fast, alternative is a huge no-go for the Fennec team.

Comment 15

4 years ago
(In reply to Nick Alexander :nalexander from comment #14)
> Two points:
> 
> * I believe we all agree we should retire dumbmake as soon as possible.
> 
> * Android devs use |mach build mobile/android| extensively.  Removing
> building subdirectories without providing a tangibly better, equally fast,
> alternative is a huge no-go for the Fennec team.

Ditto for the Firefox frontend team for |mach build browser/{base,components,themes,locales,app,modules}|, and likewise for toolkit directories - most of which is just repacking jars and/or updating flat file system stuff by copy + preprocessing.
(Reporter)

Comment 16

4 years ago
OK, I'm writing to dev.platform to at least let people know about this breakage.
(Assignee)

Comment 17

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16)
> OK, I'm writing to dev.platform to at least let people know about this
> breakage.

Don't, I'm going to "fix" it.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 18

4 years ago
(In reply to Nick Alexander :nalexander from comment #14)
> * Android devs use |mach build mobile/android| extensively.  Removing
> building subdirectories without providing a tangibly better, equally fast,
> alternative is a huge no-go for the Fennec team.

Note that using mach build foo for non c/c++ still works.
(Assignee)

Comment 19

4 years ago
(In reply to Stephen Pohl [:spohl] from comment #13)
> I hadn't tried running ./mach build binaries, since I assumed (due to the
> name) that this would only work for changes to .c, .cpp, .mm and similar
> files. Now that I tried it, it does seem to work for .js files as well

Not, it doesn't.
(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Stephen Pohl [:spohl] from comment #13)
> > I hadn't tried running ./mach build binaries, since I assumed (due to the
> > name) that this would only work for changes to .c, .cpp, .mm and similar
> > files. Now that I tried it, it does seem to work for .js files as well
> 
> Not, it doesn't.

Sorry, you're right. Chose a poor example of a .js file to test this out...

Comment 21

4 years ago
Using a mercurial repo, you have to pop all your patches before pulling from the repo.  Then you reapply everything.  If you know only one directory was actually changed ./mach build that_directory works faster than ./mach build binaries, because ./mach build binaries will rebuild every directory that was touched, even though the content hasn't actually changed.

Please don't remove the ./mach build directory functionality.

Thanks!
(Assignee)

Updated

4 years ago
Summary: ./mach build <subdirectory> is busted → ./mach build <subdirectory> doesn't recurse for C/C++ compilations
(Assignee)

Comment 22

4 years ago
(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #8)
> > (In reply to Mike Hommey [:glandium] from comment #6)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #4)
> > > > (In reply to Mike Hommey [:glandium] from comment #3)
> > > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > > comment #2)
> > > > > > (In reply to Mike Hommey [:glandium] from comment #1)
> > > > > > > See bug 1052516 comment 2
> > > > > > 
> > > > > > such as when the build config has changed (which throws off build binaries)
> > > > > 
> > > > > how so?
> > > > 
> > > > By editing a moz.build file for example.
> > > 
> > > How does that make it work less than mach build foo?
> > 
> > It works if the moz.build is somewhere under the subdir.
> 
> Are you saying that mach build binaries doesn't take changes to moz.build?
> if so, then file a bug.

Ehsan, you didn't answer this or afaik, filed a bug. What's the status?
Flags: needinfo?(ehsan)

Comment 23

4 years ago
Given the updated summary, I don't think bug 1057027 is a dupe.
(Reporter)

Comment 24

4 years ago
(In reply to Mike Hommey [:glandium] from comment #22)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #8)
> > > (In reply to Mike Hommey [:glandium] from comment #6)
> > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > comment #4)
> > > > > (In reply to Mike Hommey [:glandium] from comment #3)
> > > > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > > > comment #2)
> > > > > > > (In reply to Mike Hommey [:glandium] from comment #1)
> > > > > > > > See bug 1052516 comment 2
> > > > > > > 
> > > > > > > such as when the build config has changed (which throws off build binaries)
> > > > > > 
> > > > > > how so?
> > > > > 
> > > > > By editing a moz.build file for example.
> > > > 
> > > > How does that make it work less than mach build foo?
> > > 
> > > It works if the moz.build is somewhere under the subdir.
> > 
> > Are you saying that mach build binaries doesn't take changes to moz.build?
> > if so, then file a bug.
> 
> Ehsan, you didn't answer this or afaik, filed a bug. What's the status?

Oh, sorry, I tested it with a small change and it seemed to work fine.  I think this might have not worked in the past and perhaps I just incorrectly assumed that it doesn't work.  At any rate, it seems to work fine right now.  I'll file a bug if I see any issue with this.
Flags: needinfo?(ehsan)
(In reply to Tanvi Vyas [:tanvi] from comment #21)
> Using a mercurial repo, you have to pop all your patches before pulling from
> the repo.  Then you reapply everything.  If you know only one directory was
> actually changed ./mach build that_directory works faster than ./mach build
> binaries, because ./mach build binaries will rebuild every directory that
> was touched, even though the content hasn't actually changed.
> 
> Please don't remove the ./mach build directory functionality.
> 
> Thanks!

Either this or guarantee that 'make -C <dir>' continues to work. I want to
do this all the time.

Updated

4 years ago
Duplicate of this bug: 1057690
I'm agnostic about restoring the functionality here (other than I'm used to using "mach build subdir", so hey,don't move my cheese :), but if we ever change it, having "mach build sudirectory" fail with some message about "mach build binaries" is the new whizz-bang would have been nice.  Posting to dev.platform isn't as thorough a way of getting the news out.

Also, "mach help build" mentions nothing about the "binaries" target.  Neither does the main 'mach' info page:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/mach

I had to google to find Mike's blog post about it.
(Assignee)

Comment 28

3 years ago
Created attachment 8478665 [details] [diff] [review]
Build C/C++ code when recursing from non-toplevel builds
Attachment #8478665 - Flags: review?(gps)
(Assignee)

Updated

3 years ago
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Comment on attachment 8478665 [details] [diff] [review]
Build C/C++ code when recursing from non-toplevel builds

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

I think this will placate most of the concerns I've seen in this bug and on the dev.platform thread. I worry about dependency foo. Since the binary dependencies are computed explicitly from moz.build data, we have no guarantees that child or parent directories build in a sane order. This is very much a "use at your own risk" feature.

I wonder if we should offer people a way of building static libraries easier. e.g. |mach build binaries:dom_| would match all binaries with names dom_* and build them with proper dependencies. Requires a bit of effort in the build system to facilitate thought. Let's see what this gets us first.
Attachment #8478665 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/e1174da6b707
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1056491
You need to log in before you can comment on or make changes to this bug.