370750 - Add MOCHITESTS_DIR magic to config/rules.mk

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jeff Walden [:Waldo]]

I think I have a penchant for masochism.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

I don't like forcing inclusion of relativesrcdir, but I assume there's no other way since that's how we did it before.  Also, I changed the name to MOCHITEST_DIRS, and it's a list of directories to accommodate splitting up tests into subdirectories of a single directory if desired.

Comment 2

[Robert Sayre]

I think you have the r? flags backwards

Comment 3

[Robert Sayre]

Comment on attachment 255486 [details] [diff] [review]
Patch

bsmedberg wants no wildcards here, so it is easy to ifdef for branching and platform

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> I think you have the r? flags backwards

No, I didn't -- bsmedberg for the build-fu, you for the concepts.  But anyway...

(In reply to comment #3)
> bsmedberg wants no wildcards here, so it is easy to ifdef for branching and
> platform

So basically you want the status quo of having to manually list every single file used in all the tests?  Why?  I don't see what you gain here -- you could use the same ifdefs in the Makefile.in to determine the value of MOCHITEST_DIRS and split functionality by branch/platform/etc. that way, and there's no hassle of having to edit the Makefile.in for the general case of adding a test to an already-existing set of tests.

Comment 5

[Robert Sayre]

(In reply to comment #4)
> (In reply to comment #2)
> > I think you have the r? flags backwards
> 
> No, I didn't -- bsmedberg for the build-fu, you for the concepts.  But
> anyway...

but I am not an SR, so the process weenies would have a fit. :) But anyway...


> (In reply to comment #3)
> > bsmedberg wants no wildcards here, so it is easy to ifdef for branching and
> > platform
> 
> So basically you want the status quo of having to manually list every single
> file used in all the tests?  Why? 

I don't care either way. The only thing Benjamin has objected to is wildcards, so when he is back I am cool with whatever he wants.

Comment 7

[(not currently active) Ted Mielczarek]

bug 395019 could make the relativesrcdir thing less yucky. We should fix this, having to write extra rules in every Makefile sucks.

Comment 8

[Dave Townsend [:mossop]]

Still working on this Jeff?

Comment 9

[Jeff Walden [:Waldo]]

Nope, I'm not likely to get to this any time soon, hasn't even entered my mind in years.

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is what I think we should do. It adds MOCHITEST_FILES, MOCHITEST_CHROME_FILES and MOCHITEST_BROWSER_FILES variables. Set them to a list of files and they get copied to the appropriate places. No need for relativesrcdir to be set since it calculates it in a similar way proposed by bug 395019. Apparently that is broken for srcdir builds, do we still care about those?

Want to get review on this part before I actually go through and convert lots of makefiles to use these.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 429865 [details] [diff] [review]
patch rev 1

I've been unsuccessful in killing srcdir builds yet, so we should make this not break them, certainly. Looking at my patch from that old bug again, and testing with a local srcdir configure, I think this might work:
relativesrcdir = $(subst $(abspath $(topsrcdir))/,,$(abspath $(srcdir)))

Why don't you try this patch with that in autoconf.mk.in or config.mk?

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
>@@ -191,16 +191,33 @@ check-one:
>           $(testxpcsrcdir)/runxpcshelltests.py \
>           --symbols-path=$(DIST)/crashreporter-symbols \
>           --test-path=$(SOLO_FILE) \
>           $(DIST)/bin/xpcshell \
>           $(foreach dir,$(XPCSHELL_TESTS),$(testxpcobjdir)/$(MODULE)/$(dir))
> 
> endif # XPCSHELL_TESTS
> 
>+mochirelativedir = $(subst $(topsrcdir),,$(srcdir))
>+
>+ifdef MOCHITEST_FILES
>+libs:: $(MOCHITEST_FILES)
>+	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests$(mochirelativedir)
>+endif

the "libs:: $(MOCHITEST_FILES)" doesn't really buy us anything here, just drop the dependency and use $(MOCHITEST_FILES) instead of $^.

r=me with those changes. Thanks, I've wanted this for a long time! (Oh, and remember that you need to copy your changes to js/src/config/rules.mk as well.)

Comment 13

[Gregory Szorc [:gps]]

Attachment: Remove bit rot

Removed some bit rot. Created rules in config/makefiles/mochitest.mk instead of rules.mk.

This doesn't work because ???. I suspect I'm not fully grokking this new makefile hierarchy we have going. Should be trivial for someone to make it work, however.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

One issue that will probably need addressing is cases like <http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/dom-level1-core/Makefile.in>, where we have _TEST_FILES_{A,...,G} because Windows.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: trivial changes to make it work

This patch copies the necessary files to js/src/, fixes some syntax errors, and adds VPATH back in where appropriate.  m-c builds with this patch applied from a fresh tree; I didn't check to see whether build directories with and without a patch contained equivalent files.

I'm willing to slog through Makefiles to make the necessary changes; anybody want to come with to lighten the load?

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 637166 [details] [diff] [review]
trivial changes to make it work

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

::: config/makefiles/mochitest.mk
@@ +11,5 @@
> +endif
> +
> +ifdef MOCHITEST_FILES
> +libs:: $(MOCHITEST_FILES)
> +	$(INSTALL) $(foreach f,$^,"$f") $(call mochitestdir,mochitest)

Unless I'm much mistaken, this should be $(call mochitestdir,test)

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Convert dom/

Excludes dom/imptests, because those are autogenerated (I'll fix them later) and dom/tests/mochitest/dom-level* as discussed. Next up, content/.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: And content/

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 637534 [details] [diff] [review]
And content/

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

\o/

::: content/xul/templates/tests/chrome/Makefile.in
@@ +4,5 @@
>  
> +DEPTH = ../../../../..
> +topsrcdir = @top_srcdir@
> +srcdir = @srcdir@
> +VPATH = @srcdir@

While you are semi-gratuitously touching these groups of lines, could you make them use := instead?  Here and elsewhere.

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: part 1 - rename makefile variables

This patch is part 1 of a 3-part series; for ease of review, we're just renaming all the variables to the form we want them to be in later.  (Seemed easier to me anyway; I know some people prefer to see all the changes at once.)

Comment 21

[Nathan Froyd [:froydnj]]

Attachment: part 2 - makefile fragments

Part 2 just adds the makefile fragments.  This is effectively an effectless change, but it should be easy to review the fragments themselves this way.

Comment 22

[Nathan Froyd [:froydnj]]

Attachment: part 3 - convert libs:: rules to use the new fragments

This patch is where all the magic happens.

Left unconverted are rules which install into subdirectories of mochitests; if anybody has any bright ideas for how to handle those, I'm all ears.

This patch obsoletes virtually all uses of relativesrcdir and friends.  I'll happily do that in a followup bug; this patch series is getting rather large in size.

A sanity-checking build on my linux box survives; I'll try pushing this to try.  I'll also note that the entire patch series is based on day-old-ish m-c; some updates will probably be required.  Assuming those updatess are not too wacky, I'll take r+ on these patches to imply r+ on whatever updates need doing to update to current trunk.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 638853 [details] [diff] [review]
part 1 - rename makefile variables

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

You may want to fix the indentation in some of those.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 638854 [details] [diff] [review]
part 2 - makefile fragments

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

::: config/makefiles/mochitest.mk
@@ +10,5 @@
> +  mochitestdir = $(DEPTH)/_tests/testing/mochitest/$1/$(subst $(topsrcdir),,$(srcdir))
> +endif
> +
> +define mochitest-libs-rule-template
> +libs:: $$($1)

Stylistically, we always use the $(1) form in makefiles. I think you can make that $($(1)), too (instead of $$($(1)))

@@ +19,5 @@
> +# installing them with a single $(INSTALL) invocation would overflow
> +# command-line length limits on some operating systems.
> +ifdef MOCHITEST_FILES_PARTS
> +ifdef MOCHITEST_FILES
> +$(error You must define only one of MOCHITEST_FILES_PARTS and MOCHITEST_FILES)

s/and/or/ ?

Comment 25

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #22)
> This patch obsoletes virtually all uses of relativesrcdir and friends.  I'll
> happily do that in a followup bug; this patch series is getting rather large
> in size.

Note there's a bug for replacing relativesrcdir.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #25)
> (In reply to Nathan Froyd (:froydnj) from comment #22)
> > This patch obsoletes virtually all uses of relativesrcdir and friends.  I'll
> > happily do that in a followup bug; this patch series is getting rather large
> > in size.
> 
> Note there's a bug for replacing relativesrcdir.

That's bug 395019. By the way, it would be better to use that for part 2.

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 638856 [details] [diff] [review]
part 3 - convert libs:: rules to use the new fragments

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

Might be better to land everything in one changeset.

::: content/html/document/test/Makefile.in
@@ +84,3 @@
>  
>  ifneq (mobile,$(MOZ_BUILD_APP))
> +include $(topsrcdir)/config/rules.mk

Remove the ifneq :)

::: layout/base/tests/Makefile.in
@@ +360,5 @@
>  	$(NULL)
>  
> +include $(topsrcdir)/config/rules.mk
> +
> +DEFINES += -D_IMPL_NS_LAYOUT

You can leave the DEFINES where it was.

::: layout/style/test/Makefile.in
@@ +215,5 @@
> +	$(RM) $@
> +	./host_ListCSSProperties$(HOST_BIN_SUFFIX) > $@
> +	cat $(srcdir)/css_properties_like_longhand.js >> $@
> +
> +GARBAGE += css_properties.js

Likewise, you don't have to move this stuff.

::: toolkit/mozapps/extensions/test/browser/Makefile.in
@@ +116,1 @@
>  	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)-window

It might be worth looking if this is required, because it would be nice if that could also go away.

Comment 28

[Nathan Froyd [:froydnj]]

Thanks for the quick review.

(In reply to Mike Hommey [:glandium] from comment #27)
> ::: layout/base/tests/Makefile.in
> @@ +360,5 @@
> >  	$(NULL)
> >  
> > +include $(topsrcdir)/config/rules.mk
> > +
> > +DEFINES += -D_IMPL_NS_LAYOUT
> 
> You can leave the DEFINES where it was.
> 
> ::: layout/style/test/Makefile.in
> @@ +215,5 @@
> > +	$(RM) $@
> > +	./host_ListCSSProperties$(HOST_BIN_SUFFIX) > $@
> > +	cat $(srcdir)/css_properties_like_longhand.js >> $@
> > +
> > +GARBAGE += css_properties.js
> 
> Likewise, you don't have to move this stuff.

DEFINES and GARBAGE might be able to stay, but at least some of the other rules need to be moved after including rules.mk, otherwise we run into trouble with OBJ_SUFFIX.  (It's a little weird that DEFINES and GARBAGE are only ever appended to, which I suppose makes it OK to place them wherever, but also makes them a bit fragile.)

Comment 29

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #28)
> Thanks for the quick review.
> 
> (In reply to Mike Hommey [:glandium] from comment #27)
> > ::: layout/base/tests/Makefile.in
> > @@ +360,5 @@
> > >  	$(NULL)
> > >  
> > > +include $(topsrcdir)/config/rules.mk
> > > +
> > > +DEFINES += -D_IMPL_NS_LAYOUT
> > 
> > You can leave the DEFINES where it was.
> > 
> > ::: layout/style/test/Makefile.in
> > @@ +215,5 @@
> > > +	$(RM) $@
> > > +	./host_ListCSSProperties$(HOST_BIN_SUFFIX) > $@
> > > +	cat $(srcdir)/css_properties_like_longhand.js >> $@
> > > +
> > > +GARBAGE += css_properties.js
> > 
> > Likewise, you don't have to move this stuff.
> 
> DEFINES and GARBAGE might be able to stay, but at least some of the other
> rules need to be moved after including rules.mk, otherwise we run into
> trouble with OBJ_SUFFIX.

That's true.

> (It's a little weird that DEFINES and GARBAGE are
> only ever appended to, which I suppose makes it OK to place them wherever,
> but also makes them a bit fragile.)

They are not used in target definitions, so they can be assigned/appended anywhere. Variables used for target definitions are a different story.

[ where target definition is a "target: dependencies" line ]

Comment 30

[Nathan Froyd [:froydnj]]

So a diff -ur --unidirectional-new-file on a pristine build directory versus a patched build directory reveals that a lot of files have been moved around.  The culprit seems to be that relativesrcdir was an approximate alias of srcdir.  For instance, see http://dxr.lanedo.com/mozilla-central/accessible/tests/mochitest/actions/Makefile.in.html#l10 or http://dxr.lanedo.com/mozilla-central/toolkit/components/places/tests/mochitest/bug_461710/Makefile.in.html#l10 .  The diffstat for the above diff command is:

 901 files changed, 53152 insertions(+), 4 deletions(-)

I haven't investigated the deletions yet (probably a minor bug), but does anybody else think the large amount of churn here is a concern?  Comparing tests before and after this change is applied is going to be beastly, at the very least.  Could a change like this also mess with orangefactor and tbpl starring?

An example of a moved test is:

old: _tests/testing/mochitest/a11y/accessible/actions/test_anchors.html 
new: _tests/testing/mochitest/a11y/accessible/tests/mochitest/actions/test_anchors.html

Comment 31

[Mike Hommey [:glandium]]

That would most definitely mess with tbpl starring.

Comment 32

[Mike Hommey [:glandium]]

Might be safer to just use relativesrcdir for the time being.

Comment 33

[(no longer active)]

(In reply to comment #31)
> That would most definitely mess with tbpl starring.

This is how the TBPL suggestions work: for each failure line in the log excerpt of the page, we parse out the path name, get the file name, and then issue a bugzilla query for bugs which have "orange" in their status whiteboard and the file name in their summary.  So, as long as this patch doesn't change the leaf name of files (which it doesn't as far as I can tell), TBPL will work fine with it.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: part 2 - makefile fragments

(In reply to Mike Hommey [:glandium] from comment #32)
> Might be safer to just use relativesrcdir for the time being.

Ah, I see what the .mk files were doing attempting to use relativetestdir.

Since relativesrcdir is already there, I've changed the fragments to use relativesrcdir instead.  This shuffles a few files around, but the number is much fewer than what we had before, and it's small enough that if there *is* anyway fallout, it will be minor.

Comment 35

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #34)
> This shuffles a few files around

To elaborate: gfx/tests is affected (3 tests), since the makefile installed the tests into $(MOCHITEST_DIR)/gfx instead of gfx/tests, and toolkit/components/perf is affected (1 test), since the makefile installed the tests into $(MOCHITEST_DIR)/$(MODULE), where $(MODULE) is jsperf.

Comment 36

[Nathan Froyd [:froydnj]]

Try looks pretty good:

https://tbpl.mozilla.org/?tree=Try&rev=2f601e611da7

Pushed as one big patch per glandium's suggestion:

https://hg.mozilla.org/integration/mozilla-inbound/rev/654677c62195

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/654677c62195