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)
Firefox Build System
General
Tracking
(firefox57 wontfix, firefox60 fixed)
RESOLVED
FIXED
mozilla60
People
(Reporter: bgrins, Assigned: nalexander)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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
Reporter | ||
Comment 1•4 years ago
|
||
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`
Reporter | ||
Comment 2•4 years ago
|
||
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
Comment 3•4 years ago
|
||
We may want `mach run` to automagically purge these caches. I don't know the implications well enough to say one way or another.
Reporter | ||
Comment 4•4 years ago
|
||
(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)
Comment 5•4 years ago
|
||
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)
Comment 6•4 years ago
|
||
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)
Comment 7•4 years ago
|
||
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 :/
Reporter | ||
Comment 8•4 years ago
|
||
(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
Assignee | ||
Comment 9•4 years ago
|
||
(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.
Reporter | ||
Comment 10•4 years ago
|
||
(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)
Comment 11•4 years ago
|
||
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)
Reporter | ||
Comment 12•4 years ago
|
||
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?
Assignee | ||
Comment 13•4 years ago
|
||
(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.
Updated•4 years ago
|
status-firefox57:
--- → wontfix
Comment hidden (mozreview-request) |
Updated•3 years ago
|
Attachment #8944094 -
Flags: review?(core-build-config-reviews) → review?(gps)
Comment 15•3 years ago
|
||
mozreview-review |
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 | ||
Updated•3 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•3 years ago
|
||
mozreview-review |
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+
Comment 19•3 years ago
|
||
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f99f5cd4e544 Write .purgecaches sentinels during |mach watch|. r=gps
Comment 20•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f99f5cd4e544
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•3 years ago
|
Product: Core → Firefox Build System
Updated•10 months ago
|
Flags: needinfo?(mh+mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•