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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

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

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

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?)

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

(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.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

This feel less like an abomination, but still not very elegant...

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

forgot to qref

Comment 6

[Mike Hommey [:glandium]]

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 :)

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Addresses review comments.

If we want to avoid browser-specific code in rules.mk, we could move it to /browser/app-rules.mk instead.

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/projects/elm/rev/9bad0d06ba0f

Comment 9

[Jim Mathies [:jimm]]

Should this be tracked as a dependent of bug 755724 so it makes it into mc?

Comment 10

[Matt Brubeck (:mbrubeck)]

Pushed a trivial followup to fix a build error in a clobber build:
https://hg.mozilla.org/projects/elm/rev/fca9249a16fa

Comment 11

[Mike Hommey [:glandium]]

Attachment: Create .purgecaches in $(DIST)/bin and $(DIST)/bin/webapprt, and allow to override $(DIST)/bin

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.

Comment 12

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/2d9c75b462f8