Closed Bug 1386040 Opened 4 years ago Closed 3 years ago

./mach watch doesn't work on some files without passing --purgecaches to ./mach run

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1386040 - Write .purgecaches sentinels during |mach watch|.
59 bytes, text/x-review-board-request
gps
: review+
Details 
I'm seeing this when using the `./mach watch` command from Bug 1384241 (OSX, artifact build)

STR:
Terminal 1: `./mach watch`
At the top of browser/components/nsBrowserGlue.js, add `dump("file changed\n");`
Terminal 2: `./mach run`
No dump statement appears

If I quit the running instance do `./mach run --purgecaches` the dump statement appears as expected
A relatively easy fix would be to always pass --purgecaches to mach run in artifact builds. But that may be papering over a problem with the command. I see there are similar issues in certain cases with `./mach build faster` (like Bug 1368699), although I'm not seeing that issue myself. If I do this then the dump statement does appear as expected:

At the top of browser/components/nsBrowserGlue.js, add `dump("file changed\n");`
`./mach build faster`
`./mach run`
When I do the STR for comment 0 (not working, mach watch)

  > find objdir.noindex -name .purgecaches
  objdir.noindex/dist/bin/browser/.purgecaches

When I do the STR for comment 1 (is working, mach build faster)

  > find objdir.noindex -name .purgecaches
  objdir.noindex/dist/bin/browser/.purgecaches
  objdir.noindex/dist/Nightly.app/Contents/Resources/browser/.purgecaches
We may want `mach run` to automagically purge these caches. I don't know the implications well enough to say one way or another.
(In reply to Gregory Szorc [:gps] from comment #3)
> We may want `mach run` to automagically purge these caches. I don't know the
> implications well enough to say one way or another.

I also don't know the implications, but that would fix this problem and also things like Bug 1368699. Going to needinfo Mossop for feedback. It looks like there are 3 ways right now to purge caches (introduced in Bug 531886):

1) the .purgecaches file, which allows for automatic purging in local builds (the .purgecaches file is written by the build system)
2) a -purgecaches argument which causes all caches to be skipped
3) a MOZ_PURGE_CACHES environment variable which acts the same as the -purgecaches argument

IIUC the solution would be to either:
1) Track down and fix any situations where the build system isn't writing .purgecaches file properly (like here, Bug 1368699, Bug 1211957)
2) Don't rely on .purgecaches and instead always purge by default in local builds. I haven't done measurements but I guess the main reason for .purgecaches is performance (especially in debug builds).

A fairly simple workaround would be to make `./mach run` always purge caches in certain conditions (opt builds, for instance). But this would leave the broken behavior when opening a locally built binary directly without using mach.
Flags: needinfo?(dtownsend)
glandium: do you have an opinion here? (Also, did I ask you about purgecaches recently? I swear I asked someone about it in the past month or two and it's really bothering me that I can't recall specifics.)

My naive opinion is that if these caches are for startup perf, then it seems like not using them for local builds seems like a win given the track record of "broken" builds from their use. As long as there is an escape hatch so people can test the "with caches enabled" code paths, I think we could live without them by default.
Flags: needinfo?(mh+mozilla)
Yes the caches are for performance reasons. The difference should be pretty minimal in general, stuff is cached after first load. Obviously I'd prefer if we could keep the behaviour and fix the bugs around the build system but I'm assuming that is difficult so otherwise I don't think too many folks will miss it if we cleared the cache on startup for every local build.
Flags: needinfo?(dtownsend)
I'm not opposed to a hacky fix like "purge caches during every build system invocation." That feels less evil than purging caches for every `mach run`, as I think caches should only be purged if the build changed, not if you ran Firefox. I just don't know enough about these caches to know what is appropriate :/
(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not opposed to a hacky fix like "purge caches during every build system
> invocation." That feels less evil than purging caches for every `mach run`,

Good point, having it connected to the build would be a lot better than having it connected to the run
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > I'm not opposed to a hacky fix like "purge caches during every build system
> > invocation." That feels less evil than purging caches for every `mach run`,
> 
> Good point, having it connected to the build would be a lot better than
> having it connected to the run

It would be easy to have |mach watch| touch .purgecaches.  I'm more interested in why |mach build faster| doesn't.
(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not opposed to a hacky fix like "purge caches during every build system
> invocation." That feels less evil than purging caches for every `mach run`,
> as I think caches should only be purged if the build changed, not if you ran
> Firefox. I just don't know enough about these caches to know what is
> appropriate :/

How hard would it be to purge caches on every build system invocation? If we did that perhaps we could also close out Bug 1368699. I assumed that PURGECACHES_FILES was intending to do this already (https://dxr.mozilla.org/mozilla-central/source/config/rules.mk#1592-1599).
Flags: needinfo?(gps)
Blocks: 1389169
It is technically easy to have the build system purge caches. I'm just not sure what the history behind cache purging is.
Flags: needinfo?(gps)
Just wanted to note that I've been using the mach watch + the restart keyboard shortcut workflow for a couple of weeks and it's been wonderful. I've been holding off on announcing the workflow more broadly until we can figure out a fix for this bug since I think it'll cause people to not trust it (even knowing about this bug I sometimes accidentally don't purge caches and wonder if watch broke).

I wonder if we can have a more narrow fix here as opposed to changing how the cache purging works in general. If we could bring `./mach watch` into parity with `./mach build faster` it would fix this bug. That is, `./mach build faster` always works except in some rare / hard to reproduce cases like bug 1368699 - can we make `./mach watch` rely on the same thing so that it works in the same cases?
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Just wanted to note that I've been using the mach watch + the restart
> keyboard shortcut workflow for a couple of weeks and it's been wonderful.
> I've been holding off on announcing the workflow more broadly until we can
> figure out a fix for this bug since I think it'll cause people to not trust
> it (even knowing about this bug I sometimes accidentally don't purge caches
> and wonder if watch broke).
> 
> I wonder if we can have a more narrow fix here as opposed to changing how
> the cache purging works in general. If we could bring `./mach watch` into
> parity with `./mach build faster` it would fix this bug. That is, `./mach
> build faster` always works except in some rare / hard to reproduce cases
> like bug 1368699 - can we make `./mach watch` rely on the same thing so that
> it works in the same cases?

It's not easy to do exactly what |mach build faster| does, since |mach build faster| is mostly implemented in Make and ties into a bunch of much larger schemes.  But I think it would be easy to make |mach watch| clone whatever |mach build faster| does with buildid and .purgecaches.
Attachment #8944094 - Flags: review?(core-build-config-reviews) → review?(gps)
Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

https://reviewboard.mozilla.org/r/214410/#review220402

I could probably grant r+ on this. But I'd like to see the final version of the dependent patches before signing off.

::: python/mozbuild/mozbuild/faster_daemon.py:295
(Diff revision 1)
> +            if backend_cls:
> +                output = None
> +                jobs = 1
> +                verbose = False
> +                status = 0
> +                backend_cls.postbuild(self.config_environment, output, jobs, verbose, status)

This will need renamed to `post_build`. I'm also not a fan of the short-lived variables. Just pass the arguments inline.
Attachment #8944094 - Flags: review?(gps) → review-
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

https://reviewboard.mozilla.org/r/214410/#review222870
Attachment #8944094 - Flags: review?(gps) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f99f5cd4e544
Write .purgecaches sentinels during |mach watch|. r=gps
https://hg.mozilla.org/mozilla-central/rev/f99f5cd4e544
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Core → Firefox Build System
Flags: needinfo?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.