1176557 - Automatic background builds triggered by filesystem watching

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Having to type "mach build" adds overhead. Having builds kick off automatically when files change enables people to see results sooner than if we had to wait for someone to initiate the build.

Facebook's "watchman" tool provides an easy-to-use yet powerful mechanism to harness filesystem watching.

We should enable a build system mode that performs automatic background builds when files change. This should enable developers to move faster, as they spend less time triggering and waiting on builds.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1176557 - Detect and register Watchman 3.1+ in configure; r?glandium

Bug 1176557 - Detect and register Watchman 3.1+ in configure

Future commits will integrate the Watchman filesystem watching tool into
mach and the build system. The first step is to detect the presence of a
suitable Watchman binary during configure.

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1176557 - Initial support for automatic builds in the background; r?glandium

Bug 1176557 - Introduce `mach autobuild` for performing automatic background builds

The `mach autobuild` family of commands is introduced.

`mach autobuild start` registers watchman triggers that will kick off
specific partial-tree builds when files change. Currently, support is
limited to C/C++ files and the "binaries" target.

`mach autobuild stop` unregisters watchman triggers.

There's still a lot of room for improvement of this feature. For
example, build output isn't reliably captured anywhere. Mach will save
output from the last command, but this feature is far from sufficient,
as several builds can be kicked off back to back and developers may want
to access logs from an earlier build. But you have to start somewhere.

Another problem is race conditions. We likely need to add a lock to the
build system to ensure only one build is active at a time. If we don't
do this, chaos will likely ensue.

Comment 3

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/11781/#review10255

The review flag on the 2nd commit is really for feedback. I'm not comfortable with the feature landing in the current state. At the very least, we need to establish a build lock to prevent concurrent building or the world can explode. I'd also like to implement a build rule for processing install manifests, as that will make give non-Gecko developers a benefit as well.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8625121 [details]
MozReview Request: Bug 1176557 - Detect and register Watchman 3.1+ in configure; r?glandium

Bug 1176557 - Detect and register Watchman 3.1+ in configure; r?glandium

Future commits will integrate the Watchman filesystem watching tool into
mach and the build system. The first step is to detect the presence of a
suitable Watchman binary during configure.

Comment 5

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1176557 - Refactor file locking to be a context manager; r?glandium

Bug 1176557 - Refactor file locking to be a context manager; r?glandium

An upcoming patch will introduce locking around `mach build`. We already
have a mechanism for performing file-based locking. However, the API is
a bit ancient, as the code was originally written for an old version of
Python and was copied into mozbuild in 4a2c4921ff7c / bug 924331.

We rewrite the file locking API to be more modern. Gone is the reliance
on __del__. Replacing it is a context manager and explicit APIs for
controlling the state of the lock.

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1176557 - Introduce a lock around `mach build`; r?glandium

Bug 1176557 - Introduce a lock around `mach build`; r?glandium

Performing multiple builds concurrently is a good way to produce bad
builds. Don't allow it.

As part of this, an unused import was discovered and removed.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8625122 [details]
MozReview Request: Bug 1176557 - Initial support for automatic builds in the background; r?glandium

Bug 1176557 - Initial support for automatic builds in the background; r?glandium

The `mach autobuild` family of commands is introduced.

`mach autobuild start` registers watchman triggers that will kick off
specific partial-tree builds when files change. Currently, support is
limited to C/C++ files and the "binaries" target.

`mach autobuild stop` unregisters watchman triggers.

`mach autobuild build` receives a list of changed files and initiates a
build. Currently, we only support auto building binaries.

There's still a lot of room for improvement of this feature. For
example, build output isn't easily discoverable. But, you need to start
somewhere. I'm not sure how this feature will be received. Let's roll it
out and see what people have to say.

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/11781/#review10261

I now think the series is in a good enough state to take seriously. Log management could use some improvement. But at this juncture, I'm more interested in whether developers feel this is a feature worth spending more time on. We only know that if we land something.

Comment 9

[Mike Hommey [:glandium]]

Without looking in the implementation details, the locking approach feels wrong. Think about the worst case scenario: edit a central header (jsapi.h, for example). Then you have what essentially will be a clobber build as far as binaries are concerned blocking any further code update triggering a new build. For 10 minutes. For such a feature to be useful, it needs to kill existing builds and start new ones.
May want to issue big warnings when running on battery, too (and not just when doing autobuild start, but on each trigger).

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8625121 [details]
MozReview Request: Bug 1176557 - Detect and register Watchman 3.1+ in configure; r?glandium

Let me unflag these for now.

Comment 11

[Gregory Szorc [:gps]]

nalexander has been playing around with watchman too. I think we should converge solutions :)