Closed Bug 790115 Opened 7 years ago Closed 7 years ago

[elm] .purgecaches file not working automatically in elm builds

Categories

(Firefox Build System :: General, defect)

Other Branch
x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: mbrubeck, Assigned: glandium)

References

Details

(Whiteboard: [completed-elm])

Attachments

(2 files, 3 obsolete files)

When I update my local build of the "elm" branch, and then run either the desktop or Metro browser, the browser fails to find or remove the .purgecaches file.  I have to use -purgecaches or MOZ_PURGE_CACHES instead.

.purgecaches is created in $OBJDIR/dist/bin:
http://hg.mozilla.org/projects/elm/file/6ccd07cc1346/config/rules.mk#l1669

...but we look for it in the appData directory:
http://hg.mozilla.org/projects/elm/file/6ccd07cc1346/toolkit/xre/nsAppRunner.cpp#l3513

On elm, this directory is either dist/bin/metro or dist/bin/browser:
http://hg.mozilla.org/projects/elm/file/6ccd07cc1346/browser/app/nsBrowserApp.cpp#l216
http://hg.mozilla.org/projects/elm/file/6ccd07cc1346/browser/app/nsBrowserApp.cpp#l233
OS: Windows 7 → Windows 8 Metro
Summary: [elm] .purgecaches file not working automatically in elm builds on Windows 8 Metro → [elm] .purgecaches file not working automatically in elm builds
Attached patch patch (obsolete) — Splinter Review
This is the simplest fix I could see, though it seems a bit inelegant.  (Would it be better to make nsAppRunner always look for .purgecaches in the same place?)
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #660178 - Flags: review?(mh+mozilla)
Comment on attachment 660178 [details] [diff] [review]
patch

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

Note you need to change js/src/config/rules.mk when you change config/rules.mk.

::: config/rules.mk
@@ +1667,5 @@
>  
> +APP_DATA_DIRS = \
> +  $(DIST)/bin \
> +  $(DIST)/bin/browser \
> +  $(DIST)/bin/metro \

That's kind of awful :(
An idea that would work is, provided that DIST_SUBDIR is defined for metro (it is for browser on elm), to use $(DIST)/bin/$(DIST_SUBDIR), and only that. And that would work for more than browser or metro.

@@ +1674,4 @@
>  default all::
> +	for dir in $(APP_DATA_DIRS); do \
> +	  if test -d $$dir ; then touch $$dir/.purgecaches ; fi ; \
> +	done

It would be better to use make dependencies.
Attachment #660178 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #2)
> That's kind of awful :(

Agreed, but I'm not sure how to make a prettier version that works. :(

> An idea that would work is, provided that DIST_SUBDIR is defined for metro
> (it is for browser on elm), to use $(DIST)/bin/$(DIST_SUBDIR), and only
> that. And that would work for more than browser or metro.

An elm build with --enable-metro produces both "browser" and "metro", so when building changes that affect shared components (like toolkit) we really want to touch both .purgecaches files and not just one of them.  DIST_SUBDIR is set to "metro" only when building the /browser/metro directory, so this wouldn't remove the cache properly if you built only in /toolkit or /services.
Attached patch patch v2 (obsolete) — Splinter Review
This feel less like an abomination, but still not very elegant...
Attachment #660178 - Attachment is obsolete: true
Attachment #660247 - Flags: feedback?(mh+mozilla)
Attached patch patch v2 (obsolete) — Splinter Review
forgot to qref
Attachment #660247 - Attachment is obsolete: true
Attachment #660247 - Flags: feedback?(mh+mozilla)
Attachment #660249 - Flags: review?(mh+mozilla)
Attachment #660249 - Attachment is patch: true
Comment on attachment 660249 [details] [diff] [review]
patch v2

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

::: config/rules.mk
@@ +1665,5 @@
>  libs export::
>  	$(CHECK_FROZEN_VARIABLES)
>  
> +ifdef DIST_SUBDIR
> +PURGECACHES_FILES = $(DIST)/bin/$(DIST_SUBDIR)/.purgecaches

DIST_SUBDIR is only set to browser when building under browser/, so the problem you raise in comment 3 would still be there :( so you'd pretty much need to hardcode browser in there, probably depending on the MOZ_BUILD_APP.

@@ +1678,2 @@
>  default all::
> +	if test -d $$dir ; then touch $(PURGECACHES_FILES) ; fi ; \

Please use make dependencies, like:
default all: $(PURGECACHE_FILES)

$(PURGECACHE_FILES):
        test -d ${@D} && touch $@

And don't forget to update js/src/config/rules.mk as well :)
Attachment #660249 - Flags: review?(mh+mozilla) → review-
Attached patch patch v3Splinter Review
Addresses review comments.

If we want to avoid browser-specific code in rules.mk, we could move it to /browser/app-rules.mk instead.
Attachment #660249 - Attachment is obsolete: true
Attachment #660498 - Flags: review?(mh+mozilla)
Attachment #660498 - Flags: review?(mh+mozilla) → review+
Should this be tracked as a dependent of bug 755724 so it makes it into mc?
Pushed a trivial followup to fix a build error in a clobber build:
https://hg.mozilla.org/projects/elm/rev/fca9249a16fa
Bug 755724 will require that PURGECACHES_DIRS is set to $(DIST)/bin/browser for browser/.

This is derived from mbrubeck's patch that landed on elm.
Attachment #712360 - Flags: review?(ted)
Assignee: mbrubeck → mh+mozilla
Attachment #712360 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/2d9c75b462f8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
OS: Windows 8 Metro → Windows 8.1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.