Closed Bug 844288 Opened 7 years ago Closed 7 years ago

Dual link libxul.so and libxul-unit.so and replace enable-gtest by enable-test

Categories

(Firefox Build System :: General, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 16 obsolete files)

Dual Linking v13
26.34 KB, patch
BenWa
: review+
Details | Diff | Splinter Review 
Currently gtest is compiled separately from enable-test because this is the configuration we ship and we don't want to ship our unit test part of libxul.so or run our other test on libxul.so with embedded unit tests. We need to generate libxul.so and then again generate libxul-unit.so that will additionally include our unit tests. Once this is done we can remove enable-gtest and simple work off enable-test.
Blocks: 844852
I don't know if you're the right person to ask GPS. I don't know how to implement this properly without making it a total hack of the build system. I want to generate libxul.so as-is and then add in all the GTEST_*SRCS and generate libxul-unit.so.
Flags: needinfo?(gps)
libxul linking is one of the parts of the build system I don't yet know that much about. All I know is it is triggered in /toolkit/library/Makefile. Mike, Ted, and Kyle all know more than me. Magic 8 ball says Mike!
Flags: needinfo?(gps) → needinfo?(mh+mozilla)
Component: General → Build Config
Product: Testing → Core
Something like this (untested)?

ifeq (,$(filter %-unit,$(SHARED_LIBRARY_NAME)))
TEST_LIB = $(DLL_PREFIX)$(SHARED_LIBRARY_NAME)-unit$(DLL_SUFFIX)
else
SHARED_LIBRARY_LIBS += ...
endif

libs:: $(TEST_LIB)

$(TEST_LIB): $(SHARED_LIBRARY)
        $(MAKE) $@ SHARED_LIBRARY_NAME=$(SHARED_LIBRARY_NAME)-unit
Flags: needinfo?(mh+mozilla)
Attached patch patch (WIP) (obsolete) — — Splinter Review 
This patch is almost done but has two remaining problems:
1) toolkit/library dual linking should follow the form suggested by glandium
2) We can't simply conditionally open libxul or libxul-unit. One possibility would be to create a separate dist directory for libxul-unit.
My WIP was made difficult because of libxpcom but with bug 852950 this is no longer an issue.
Depends on: 852950
I talked with ted and gps on IRC. I'm going to stop linking libxul-unit by default and run the make target from mach gtest.

I'll be delaying the work for getting this ready for automation in another bug.
Attached patch patch (obsolete) — — Splinter Review 
Only tested on mac:
https://tbpl.mozilla.org/?tree=Try&rev=2e5ac8a9832b
Assignee: nobody → bgirard
Attachment #723575 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #741043 - Flags: review?(gps)
Attached patch Part 2: Stage XUL-unit (obsolete) — — Splinter Review
Attachment #741077 - Flags: review?(gps)
Attached patch Part 1: Dual link libxul (obsolete) — — Splinter Review 
Replaced setenv with nspr version to fix windows:
https://tbpl.mozilla.org/?tree=Try&rev=f6d6b2c45825

Blocking on bug 846356 to finish the port.
Attachment #741043 - Attachment is obsolete: true
Attachment #741043 - Flags: review?(gps)
Attachment #741080 - Flags: review?(gps)
Depends on: 846356
Attached patch Part 2: Stage XUL-unit (obsolete) — — Splinter Review 
Forgot to build gtest before packaging.
Attachment #741077 - Attachment is obsolete: true
Attachment #741077 - Flags: review?(gps)
Attachment #741083 - Flags: review?(gps)
Parts of these patches are slightly outside my comfort zone.

ted: would you mind picking this up? I'm sure you have something you could trade to me :)
Attachment #741080 - Flags: review?(gps) → review?(ted)
Attachment #741083 - Flags: review?(gps) → review?(ted)
Run GTest on make check for now:
https://tbpl.mozilla.org/?tree=Try&rev=40ba847cc0ff
I think I need gtest to be a .PHONY target:
https://tbpl.mozilla.org/?tree=Try&rev=09e80ef3e6fb
Attached patch Part 3: Run GTest on make check (obsolete) — — Splinter Review 
Ideally later this will get it's own job but for now running this on make check is fine. The overhead of GTest is the additional link step. Running is only a few milliseconds total ATM.

We're hitting the link limiting with this patch. Looks like central is about to hit it soon as well.
Attachment #742547 - Flags: review?(ted)
bitrotted by android gtest support and xpcom unit tests.
Attachment #741080 - Attachment is obsolete: true
Attachment #741080 - Flags: review?(ted)
Attachment #743124 - Flags: review?(ted)
Comment on attachment 743124 [details] [diff] [review]
Part 1: Dual link libxul (rebased)

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

The build bits look fine here. I'd like bsmedberg to look at the XPCOM glue changes, though.

::: gfx/tests/gtest/Makefile.in
@@ +11,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +# Create a GTest library
> +MODULE_NAME    = gfxtest 

You have a few extraneous spaces in here.

::: toolkit/library/Makefile.in
@@ +702,5 @@
>  libs:: $(FINAL_TARGET)/dependentlibs.list
>  
> +TEST_LIB = $(DLL_PREFIX)$(SHARED_LIBRARY_NAME)-unit$(DLL_SUFFIX)
> +
> +$(TEST_LIB):: $(SHARED_LIBRARY) 

trailing space

@@ +713,5 @@
> +
> +COMPONENT_LIBS += \
> +  gtest \
> +  gfxtest \
> +	xpcom_glue_gtest \

Stray tab here.

::: toolkit/toolkit.mozbuild
@@ +149,5 @@
>      'xpfe/appshell'
>  ])
>  
> +if CONFIG['ENABLE_TESTS']:
> +    add_tier_dir('platform', 'gfx/tests/gtest')

Can these not be built from the gfx Makefile?

::: toolkit/xre/nsAppRunner.cpp
@@ +3216,5 @@
> +    if (mozilla::RunGTest) {
> +      result = mozilla::RunGTest();
> +    } else {
> +      result = 1;
> +      printf("TEST-UNEXPECTED-FAIL | Not compiled with enable-tests\n");

TEST-UNEXPECTED-FAIL | gtest | ...
Attachment #743124 - Flags: review?(ted)
Attachment #743124 - Flags: review?(benjamin)
Attachment #743124 - Flags: review+
Attachment #743124 - Flags: review?(benjamin) → review?(mh+mozilla)
Comment on attachment 743124 [details] [diff] [review]
Part 1: Dual link libxul (rebased)

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

::: browser/app/nsBrowserApp.cpp
@@ +544,5 @@
> +      printf("Run GTest\n");
> +      runGTest = true;
> +      break;
> +    }
> +  }

It's an annoyance that any app that wants gtests would need to do this. Can't we enable this with an environment variable instead?

::: testing/gtest/Makefile.in
@@ +43,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
> +check gtest::
> +	$(MAKE) -C $(DEPTH)/toolkit/library gtest
> +ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)) 

trailing whitespace

::: xpcom/glue/standalone/nsXPCOMGlue.cpp
@@ +416,5 @@
>          cursor = xpcomDir;
>      }
>  
> +    if (runGTest) {
> +        strcat(xpcomDir, ".gtest");

I'm pretty sure this blows up with binary components: they depend on libxul, and the dynamic linker will thus load both libs. On Linux, it's probably not a problem, because the dynamic linker is likely to resolve the symbols in the gtest libxul, but I'd be surprised if it's not a problem on mac and windows, where symbols are bound to the library by name. That is, symbols from binary components are going to be resolved to the non-gtest libxul.
As I said on IRC when we discussed this a while ago, the -unit thing can't work. You need a library with the same name, even if it's in a different directory.
Attachment #743124 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #19)
> I'd be surprised if it's not a problem on mac and
> windows, where symbols are bound to the library by name. That is, symbols
> from binary components are going to be resolved to the non-gtest libxul.

It works fine on mac and windows but I haven't tried with custom binary components.
(In reply to Benoit Girard (:BenWa) from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > I'd be surprised if it's not a problem on mac and
> > windows, where symbols are bound to the library by name. That is, symbols
> > from binary components are going to be resolved to the non-gtest libxul.
> 
> It works fine on mac and windows but I haven't tried with custom binary
> components.

Oh, it probably works "fine", depending how/what binary components are calling back into libxul, but you should double check the loaded libraries, I'm pretty sure you'll see both libxuls, which means random crashes awaiting to happen. If not, then I'd be interested to know why not, because that's very much unexpected.
(In reply to Mike Hommey [:glandium] from comment #21)
> (In reply to Benoit Girard (:BenWa) from comment #20)
> > (In reply to Mike Hommey [:glandium] from comment #19)
> > > I'd be surprised if it's not a problem on mac and
> > > windows, where symbols are bound to the library by name. That is, symbols
> > > from binary components are going to be resolved to the non-gtest libxul.
> > 
> > It works fine on mac and windows but I haven't tried with custom binary
> > components.
> 
> Oh, it probably works "fine", depending how/what binary components are
> calling back into libxul, but you should double check the loaded libraries,
> I'm pretty sure you'll see both libxuls, which means random crashes awaiting
> to happen. If not, then I'd be interested to know why not, because that's
> very much unexpected.

I  did check on gdb with 'info shared' and only XUL-unit was loaded but that's fine, I'm working on adding the extra directory ATM.
(In reply to Benoit Girard (:BenWa) from comment #22)
> I  did check on gdb with 'info shared' and only XUL-unit was loaded but
> that's fine, I'm working on adding the extra directory ATM.

IIRC there's no binary component on mac, that would explain.
Attached patch Part 1: Dual link libxul v2 (obsolete) — — Splinter Review 
toolkit/library/Makefile.in is heavily updated
Attachment #743124 - Attachment is obsolete: true
Comment on attachment 741083 [details] [diff] [review]
Part 2: Stage XUL-unit

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

::: testing/gtest/Makefile.in
@@ +94,5 @@
> +GTEST_LIBRARY = xul-unit$(DLL_SUFFIX)
> +ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
> +# Matching logic in toolkit/library/Makefile.in
> +GTEST_LIBRARY = XUL-unit
> +endif

If it makes it simpler I'd be fine with moving the stage-package rule here to toolkit/library. You could call it stage-gtest or something, stage-package is just a convention.

@@ +99,5 @@
> +
> +PKG_STAGE = $(DIST)/test-package-stage
> +TEST_HARNESS_BINS = \
> +  $(GTEST_LIBRARY) \
> +	dependentlibs.list.gtest \

nit: extraneous tab
Attachment #741083 - Flags: review?(ted) → review+
Attachment #742547 - Flags: review?(ted) → review+
Attached patch Part 1: Dual link libxul v5 (obsolete) — — Splinter Review
Attachment #744808 - Attachment is obsolete: true
Try to fix a bug with the Makefile causing fat binary to fail:
https://tbpl.mozilla.org/?tree=Try&rev=e4a6e60e444c
Attachment #745887 - Flags: review?(mh+mozilla)
Comment on attachment 745887 [details] [diff] [review]
Part 1: Dual link libxul v5

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

::: gfx/tests/gtest/Makefile.in
@@ +34,5 @@
> +GTEST_CPPSRCS += \
> +  TestMoz2D.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestBase.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestPoint.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestScaling.cpp \

The topsrcdir notation is wrong and also breaks mac universal binaries. Working a fix for that.
Comment on attachment 745887 [details] [diff] [review]
Part 1: Dual link libxul v5

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

::: gfx/tests/gtest/Makefile.in
@@ +34,5 @@
> +GTEST_CPPSRCS += \
> +  TestMoz2D.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestBase.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestPoint.cpp \
> +  $(topsrcdir)/gfx/2d/unittest/TestScaling.cpp \

use VPATH

::: testing/gtest/Makefile.in
@@ +41,5 @@
>    $(NULL)
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +check gtest::

I don't think it's a good idea to (re)build things when doing make check.

It's also sad that there's no rule to run gtest without mach.

@@ +44,5 @@
>  
> +check gtest::
> +	$(MAKE) -C $(DEPTH)/toolkit/library gtestxul
> +ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> +	$(MAKE) -C $(DEPTH)/browser/app repackage

Why do you need a repackage here?

::: toolkit/library/Makefile.in
@@ +720,5 @@
> +  gfxtest \
> +  $(NULL)
> +endif
> +
> +dependentlibs.list.gtest.in $(FINAL_TARGET)/dependentlibs.list: dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))))

You don't need dependentlibs.list.gtest.in, just use $(FINAL_TARGET)/dependentlibs.list
Attachment #745887 - Flags: review?(mh+mozilla) → review-
Ah, I forgot to mention the most important: you need to change MKSHLIB/MKCSHLIB to use $(notdir $@) instead of $@ for things like "-Wl,-h", "-h" or "-Wl,-soname" (and I think the one for netbsd needs to change from what it is to $(notdir $@))
(In reply to Mike Hommey [:glandium] from comment #30)
> ::: testing/gtest/Makefile.in
> @@ +41,5 @@
> >    $(NULL)
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +check gtest::
> 
> I don't think it's a good idea to (re)build things when doing make check.

Well we don't want to link gtestxul from build. When testing I often found that I forgot building because now I had to do a top level build to update my tests, link gtest and then run gtest. This building target is fast because we have the right dependency so it just takes a ~5 seconds if you haven't changed anything.

If you'd like I can break this down into a seperate build target.

> 
> It's also sad that there's no rule to run gtest without mach.

That's fixed with part 3.

> 
> @@ +44,5 @@
> >  
> > +check gtest::
> > +	$(MAKE) -C $(DEPTH)/toolkit/library gtestxul
> > +ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> > +	$(MAKE) -C $(DEPTH)/browser/app repackage
> 
> Why do you need a repackage here?

gtestxul has been generated BEFORE we package then gtest/xul is generated properly in the mac .app dir. But right now the typical work flow is browser/app THEN gtestxul so without this gtest/xul doesn't get placed in the .app dir and it fails in mac until you do another top level build to repackage.
Attached patch Part 1: Dual link libxul v5 (obsolete) — — Splinter Review
Attachment #745887 - Attachment is obsolete: true
Attachment #746993 - Flags: review?(mh+mozilla)
Apparently runtimeobject is no longer needed. Trying to remove it:
https://tbpl.mozilla.org/?tree=Try&rev=5274db12c759
Comment on attachment 746993 [details] [diff] [review]
Part 1: Dual link libxul v5

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

::: configure.in
@@ +1434,5 @@
>      fi
>      _DEFINES_CFLAGS='$(ACDEFINES) -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT'
>  else
> +    MKSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> +    MKCSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'

You need to do the same on http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2319 and http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2426

::: gfx/2d/Makefile.in
@@ -37,5 @@
> -        GTestMain.cpp \
> -        TestBase.cpp \
> -        TestPoint.cpp \
> -        TestScaling.cpp \
> -        TestCairo.cpp \

You just remove those?

::: gfx/layers/Makefile.in
@@ -73,5 @@
>          TexturePoolOGL.cpp \
>          $(NULL)
>  
> -GTEST_CPPSRCS = \
> -        TestTiledLayerBuffer.cpp \

and this?

::: toolkit/library/Makefile.in
@@ +20,5 @@
>  
>  ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
>  # This is going to be a framework named "XUL", not an ordinary library named
>  # "libxul.dylib"
> +ifeq (,$(filter %-unit,$(LIBRARY_NAME)))

You don't need this anymore.

@@ +709,5 @@
> +EFFECTIVE_LIB_PREFIX=$(LIB_PREFIX)
> +endif
> +
> +$(FINAL_TARGET)/dependentlibs.list.gtest gtestxul: $(FINAL_TARGET)/dependentlibs.list.gtest.in
> +	$(MKDIR) -p $(EFFECTIVE_LIB_PREFIX)gtest $(FINAL_TARGET)/gtest $(IMPORT_LIB_DEST)/gtest

Theoretically, you don't need the last two (make libs should do it)

@@ +710,5 @@
> +endif
> +
> +$(FINAL_TARGET)/dependentlibs.list.gtest gtestxul: $(FINAL_TARGET)/dependentlibs.list.gtest.in
> +	$(MKDIR) -p $(EFFECTIVE_LIB_PREFIX)gtest $(FINAL_TARGET)/gtest $(IMPORT_LIB_DEST)/gtest
> +	$(MAKE) libs SHARED_LIBRARY_NAME=gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME) FINAL_TARGET=$(FINAL_TARGET)/gtest SDK_LIBRARY= IMPORT_LIB_DEST=$(IMPORT_LIB_DEST)/gtest LINK_GTEST=true

This should be made so that mach build toolkit/library/gtest/XUL works. (that is, with a $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME): target). It would also be better to use the rule to build the lib directly, instead of make libs.

That being said, on further reflexion, I think it would be better if the directory was simply "gtest" on all platforms, which you should be achievable by adding DLL_PREFIX= to that command.

So something like (untested):
GTEST_LIB := gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME)$(DLL_SUFFIX)
$(GTEST_LIB): $(LIBRARY_NAME)
$(GTEST_LIB) $(FINAL_TARGET)/$(GTEST_LIB):
        $(MAKE) $@ SHARED_LIBRARY_NAME=gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME) FINAL_TARGET=$(FINAL_TARGET)/gtest SDK_LIBRARY= IMPORT_LIB_DEST=$(IMPORT_LIB_DEST)/gtest LINK_GTEST=true DLL_PREFIX=

$(FINAL_TARGET)/dependentlibs.list.gtest: $(FINAL_TARGET)/dependentlibs.list
        sed -e "s|$(SHARED_LIBRARY)|gtest/$(SHARED_LIBRARY)|" $< > $@

libs:: $(FINAL_TARGET)/dependentlibs.list.gtest $(GTEST_LIB) $(FINAL_TARGET)/$(GTEST_LIB)

@@ +720,5 @@
> +  gfxtest \
> +  $(NULL)
> +endif
> +
> +$(FINAL_TARGET)/dependentlibs.list.gtest.in $(FINAL_TARGET)/dependentlibs.list: dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))))

Please don't create this intermediate .gtest.in file. Just reuse $(FINAL_TARGET)/dependentlibs.list.
Attachment #746993 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #37)
> Comment on attachment 746993 [details] [diff] [review]
> Part 1: Dual link libxul v5
> 
> Review of attachment 746993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +1434,5 @@
> >      fi
> >      _DEFINES_CFLAGS='$(ACDEFINES) -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT'
> >  else
> > +    MKSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> > +    MKCSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> 
> You need to do the same on
> http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2319

If I do it on -o here then it will output in toolkit/library and replace the main xul.

> and
> http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2426
> 
> ::: gfx/2d/Makefile.in
> @@ -37,5 @@
> > -        GTestMain.cpp \
> > -        TestBase.cpp \
> > -        TestPoint.cpp \
> > -        TestScaling.cpp \
> > -        TestCairo.cpp \
> 
> You just remove those?

Yes. It's moved to gfx/tests/gtest but since this code lives in gkmedia on windows it wont work with xul gtests. For now I'm disabling them since adding them to gtest was never a priority for the GFX team. These tests are maintained part of the standalone azure repo.

> 
> ::: gfx/layers/Makefile.in
> @@ -73,5 @@
> >          TexturePoolOGL.cpp \
> >          $(NULL)
> >  
> > -GTEST_CPPSRCS = \
> > -        TestTiledLayerBuffer.cpp \
> 
> and this?

moved to gfx/tests/gtest.
(In reply to Benoit Girard (:BenWa) from comment #38)
> (In reply to Mike Hommey [:glandium] from comment #37)
> > Comment on attachment 746993 [details] [diff] [review]
> > Part 1: Dual link libxul v5
> > 
> > Review of attachment 746993 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: configure.in
> > @@ +1434,5 @@
> > >      fi
> > >      _DEFINES_CFLAGS='$(ACDEFINES) -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT'
> > >  else
> > > +    MKSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> > > +    MKCSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> > 
> > You need to do the same on
> > http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2319
> 
> If I do it on -o here then it will output in toolkit/library and replace the
> main xul.

Not -o, -soname, see comment 31.

> > and
> > http://hg.mozilla.org/mozilla-central/file/7130e5134a6e/configure.in#l2426
> > 
> > ::: gfx/2d/Makefile.in
> > @@ -37,5 @@
> > > -        GTestMain.cpp \
> > > -        TestBase.cpp \
> > > -        TestPoint.cpp \
> > > -        TestScaling.cpp \
> > > -        TestCairo.cpp \
> > 
> > You just remove those?
> 
> Yes. It's moved to gfx/tests/gtest but since this code lives in gkmedia on
> windows it wont work with xul gtests. For now I'm disabling them since
> adding them to gtest was never a priority for the GFX team. These tests are
> maintained part of the standalone azure repo.

You could file a followup to do a gtest version of gkmedia.
(In reply to Mike Hommey [:glandium] from comment #37)
> @@ +710,5 @@
> > +endif
> > +
> > +$(FINAL_TARGET)/dependentlibs.list.gtest gtestxul: $(FINAL_TARGET)/dependentlibs.list.gtest.in
> > +	$(MKDIR) -p $(EFFECTIVE_LIB_PREFIX)gtest $(FINAL_TARGET)/gtest $(IMPORT_LIB_DEST)/gtest
> > +	$(MAKE) libs SHARED_LIBRARY_NAME=gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME) FINAL_TARGET=$(FINAL_TARGET)/gtest SDK_LIBRARY= IMPORT_LIB_DEST=$(IMPORT_LIB_DEST)/gtest LINK_GTEST=true
> 
> This should be made so that mach build toolkit/library/gtest/XUL works.
> (that is, with a
> $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME):
> target). It would also be better to use the rule to build the lib directly,
> instead of make libs.
> 
> That being said, on further reflexion, I think it would be better if the
> directory was simply "gtest" on all platforms, which you should be
> achievable by adding DLL_PREFIX= to that command.
> 
> So something like (untested):
> GTEST_LIB := gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME)$(DLL_SUFFIX)
> $(GTEST_LIB): $(LIBRARY_NAME)
> $(GTEST_LIB) $(FINAL_TARGET)/$(GTEST_LIB):
>         $(MAKE) $@
> SHARED_LIBRARY_NAME=gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME)
> FINAL_TARGET=$(FINAL_TARGET)/gtest SDK_LIBRARY=
> IMPORT_LIB_DEST=$(IMPORT_LIB_DEST)/gtest LINK_GTEST=true DLL_PREFIX=
> 
> $(FINAL_TARGET)/dependentlibs.list.gtest: $(FINAL_TARGET)/dependentlibs.list
>         sed -e "s|$(SHARED_LIBRARY)|gtest/$(SHARED_LIBRARY)|" $< > $@
> 
> libs:: $(FINAL_TARGET)/dependentlibs.list.gtest $(GTEST_LIB)
> $(FINAL_TARGET)/$(GTEST_LIB)
> 

This doesn't work because it breaks this used by NSPR_LIBS and MOZALLOC_LIB: http://mxr.mozilla.org/mozilla-central/source/config/config.mk#797

Likewise you suggested on IRC using SHARED_LIBRARY='$$(LIBRARY_NAME)$$(DLL_SUFFIX)' which just overrides the original libxul.so.
Here's a patch with the review feedback applied plus some ifdef in winvccorlib to fix the metro linking errors.

I haven't been able to address getting the libgtest/gtest to be consistent. When I get something to work on a platform is breaks the other. I would really appreciate some help for that modification. I don't think it's worth the effort to fix TBH. We should rather spend some energy to fixing the build system to handle this use case properly and clean this mess up later.
Attachment #746993 - Attachment is obsolete: true
Attachment #750015 - Flags: review?(mh+mozilla)
Blocks: 871801
Comment on attachment 750015 [details] [diff] [review]
Part 1: Dual link libxul v8 - Fixed metro problem

You still have windows build failures.
Attachment #750015 - Flags: review?(mh+mozilla)
I'd like to get a solution for libgtest folder naming? Are we pushing on solving it and how should we approach it? Perhaps you can take a look at solving this issue. I did spend a considerable amount of time on it but couldn't get it to work on all platforms.

The windows failure are easy. I need to disable gtest for the windows b2g build and for the metro library we can likely remove it which I will finish when I get back to Toronto.
Attached patch Part 1: Dual link libxul v9 (obsolete) — — Splinter Review
Attachment #750015 - Attachment is obsolete: true
Attached patch Part 1: Dual link libxul v10 (obsolete) — — Splinter Review
Attachment #754978 - Attachment is obsolete: true
Attachment #755473 - Flags: review?(mh+mozilla)
Comment on attachment 755473 [details] [diff] [review]
Part 1: Dual link libxul v10

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

r+ with those fixed.

::: configure.in
@@ +2299,5 @@
>      if test "$LIBRUNPATH"; then
>  	DSO_LDOPTS="-Wl,-R$LIBRUNPATH $DSO_LDOPTS"
>      fi
> +    MKSHLIB='$(CXX) $(CXXFLAGS) $(DSO_PIC_CFLAGS) $(DSO_LDOPTS) -Wl,-soname,$(notdir lib$(LIBRARY_NAME)$(DLL_SUFFIX)) -o $@'
> +    MKCSHLIB='$(CC) $(CFLAGS) $(DSO_PIC_CFLAGS) $(DSO_LDOPTS) -Wl,-soname,$(notdir lib$(LIBRARY_NAME)$(DLL_SUFFIX)) -o $@'

notdir $@

::: js/src/configure.in
@@ +1225,5 @@
>      fi
>      _DEFINES_CFLAGS='$(ACDEFINES) -D_JS_CONFDEFS_H_ -DMOZILLA_CLIENT'
>  else
> +    MKSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'
> +    MKCSHLIB='$(LD) $(DSO_LDOPTS) -h $(notdir $@) -o $@'

There are a few more occurrences to change in this file. (same as main configure.in, essentially)

::: toolkit/library/Makefile.in
@@ +697,5 @@
> +
> +.PHONY: gtestxul
> +
> +$(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME): gtestxul
> +	echo $(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME)

the MKDIR and MAKE commands should be here, and gtestxul depend on $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME) instead of the other way around. You probably don't need this echo either.

::: toolkit/library/winvccorlib/Makefile.in
@@ +6,5 @@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
>  
> +# When we're linking GTest we recurse into this directory but don't need                                                               

trailing whitespaces

@@ +7,5 @@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
>  
> +# When we're linking GTest we recurse into this directory but don't need                                                               
> +# to generate this library                                                                                                             

trailing whitespaces

@@ +8,5 @@
>  VPATH		= @srcdir@
>  
> +# When we're linking GTest we recurse into this directory but don't need                                                               
> +# to generate this library                                                                                                             
> +ifndef LINK_GTEST  

trailing whitespaces
Attachment #755473 - Flags: review?(mh+mozilla) → review+
Looks like it's not a good idea to define a custom target that's the library name:

Makefile:701: warning: overriding commands for target `gtest/XUL'
/builds/slave/try-osx64-00000000000000000000/build/config/rules.mk:1024: warning: ignoring old commands for target `gtest/XUL'
make[3]: Circular ../../dist/bin/gtest/dependentlibs.list <- gtest/XUL dependency dropped.
/builds/slave/try-osx64-00000000000000000000/build/obj-firefox/i386/_virtualenv/bin/python /builds/slave/try-osx64-00000000000000000000/build/toolkit/library/dependentlibs.py gtest/XUL -L ../../dist/bin/gtest  > ../../dist/bin/gtest/dependentlibs.list
/bin/sh: ../../dist/bin/gtest/dependentlibs.list: No such file or directory
make[3]: *** [../../dist/bin/gtest/dependentlibs.list] Error 1
Comment on attachment 755473 [details] [diff] [review]
Part 1: Dual link libxul v10

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

::: toolkit/library/Makefile.in
@@ +11,5 @@
>  
>  include $(topsrcdir)/rdf/util/src/objs.mk
>  include $(topsrcdir)/intl/unicharutil/util/objs.mk
>  
> +ifndef LIBRARY_NAME

btw, that's not needed.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f940af8df8 (along with its push companion bug 869114, separately in https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8df1f7497b).

Awkward situation: on your push, OS X opt, the universal build where we do a 32-bit build and then a 64-bit build and then package them together mostly using the bits from just one, insisting that they be identical between the two builds to be willing to package, built just fine. Then on the next push, it failed in the postflight stick-together, so because we clobber when a mosquito flies by our ears, RyanVM clobbered, and the retrigger did just fine, and things built fine for a couple of hours. Then there were enough slaves around which had clobbered on a build after your push, and could thus again do a dep build, and every build started failing in postflight, like https://tbpl.mozilla.org/php/getParsedLog.php?id=23574691&tree=Mozilla-Inbound.

I backed the two out separately hoping that maybe we could somehow manage to get a dep build on each of them and that way tell which it was, though retriggering a lot of builds is sort of inconvenient. Not sure how else to tell, unless you can repro locally, since try always does clobbers so there's no way it can help you at all.
Oh, and because the world has to be as awkward as possible, a slave which is in the failing state, having once done a build with your push, apparently has to be clobbered again to get back to a state where it's possible to tell what it can or can't do in the way of a dep build, so I have to clobber again and I don't know when or how we'll tell whether we're back to being able to do dep builds.
Doing universal builds locally isn't that hard, you can mostly just use http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig

(In reply to Phil Ringnalda (:philor) from comment #55)
> again do a dep build, and every build started failing in postflight, like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23574691&tree=Mozilla-

I don't understand how this is related to this push at all, this looks like a mozbase problem, I would suspect bug 790765.
Can I reland this patch then if we think it was caused by bug 790765? I don't see how I would impact the postflight since I believe these are package manifest based which I don't modify.
I'm afraid this broke my openbsd builds, which now seem to go by default under gtest (which wasnt build previously, i think) :

gtest-all.cc                                                                                                        [1848/9534]
clang++ -o gtest-all.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /src/mozilla-central/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/src/mozilla-central/testing/gtest/gtest -I/src/mozilla-central/testing/gtest/gtest/include -I/src/mozilla-central/testing/gtest/gmock -I/src/mozilla-central/testing/gtest/gmock/include  -I/src/mozilla-central/testing/gtest -I. -I../../dist/include  -I/usr/obj/m-c/dist/include/nspr -I/usr/obj/m-c/dist/include/nss      -fPIC -Qunused-arguments   -I/usr/X11R6/include -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer  -Qunused-arguments   -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/gtest-all.o.pp  /src/mozilla-central/testing/gtest/gtest/src/gtest-all.cc
GTestRunner.cpp
In file included from /src/mozilla-central/testing/gtest/gmock/src/gmock-all.cc:40:
In file included from /src/mozilla-central/testing/gtest/gmock/include/gmock/gmock.h:58:
In file included from /src/mozilla-central/testing/gtest/gmock/include/gmock/gmock-actions.h:46:
In file included from /src/mozilla-central/testing/gtest/gmock/include/gmock/internal/gmock-internal-utils.h:45:
In file included from /src/mozilla-central/testing/gtest/gmock/include/gmock/internal/gmock-generated-internal-utils.h:42:
In file included from /src/mozilla-central/testing/gtest/gmock/include/gmock/internal/gmock-port.h:45:
In file included from /src/mozilla-central/testing/gtest/gtest/include/gtest/internal/gtest-linked_ptr.h:74:
In file included from /src/mozilla-central/testing/gtest/gtest/include/gtest/internal/gtest-port.h:499:
In file included from /usr/include/g++/tr1/tuple:159:
In file included from ../../dist/system_wrappers/tr1/functional:2:
/usr/include/g++/tr1/functional:906:51: error: cannot use typeid with -fno-rtti
          __dest._M_access<const type_info*>() = &typeid(_Functor);
(In reply to Landry Breuil (:gaston) from comment #60)
> which now seem to go by default
> under gtest (which wasnt build previously, i think)

That's correct

> 
> gtest-all.cc                                                                

We're white listing the platform incorrectly in configure.in (search for GTEST_HAS_RTTI). Try removing that, if it fixes it we will land a follow up.
Comment on attachment 741083 [details] [diff] [review]
Part 2: Stage XUL-unit

I've decided to roll this into bug 844852. I'll apply the review comment there.
Attachment #741083 - Attachment is obsolete: true
Comment on attachment 742547 [details] [diff] [review]
Part 3: Run GTest on make check

I've decided to roll this into bug 844852. I'll apply the review comment there.
Attachment #742547 - Attachment is obsolete: true
Whiteboard: [keep open]
Blocks: 877809
https://hg.mozilla.org/mozilla-central/rev/dc76402b8471
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Seems this has broken Windows builds of mc with VC 2012. Most devs have upgraded, so we might want to back this out if there isn't an easy fix.
Depends on: 878018
Backed out again in https://hg.mozilla.org/mozilla-central/rev/4755d50e2402 on suspicion of being the reason dep builds aren't relinking libxul.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla24 → ---
No longer blocks: 871801
Depends on: 871801
Attached patch Dual link libxul v11 (obsolete) — — Splinter Review
Attachment #755473 - Attachment is obsolete: true
Attached patch diff between v10, v11 (obsolete) — — Splinter Review 
I'm not sure if glandium is around. Otherwise I'm deferring to ted. I only need a review from one.
Attachment #756729 - Flags: review?(ted)
Attachment #756729 - Flags: review?(mh+mozilla)
https://wiki.mozilla.org/ReviewRobot

Automated patch review result for: attachment 756724 [details] [diff] [review]
Dual link libxul v11

Patch failed to apply on tip (Might be part of patch queue or against another branch/repo)
https://wiki.mozilla.org/ReviewRobot

Automated patch review result for: attachment 756729 [details] [diff] [review]
diff between v10, v11

Patch failed to apply on tip (Might be part of patch queue or against another branch/repo)
https://wiki.mozilla.org/ReviewRobot

Automated patch review result for: attachment 756724 [details] [diff] [review]
Dual link libxul v11

::: 'toolkit/xre/nsAppRunner.cpp'


::: 'gfx/tests/gtest/TestTiledLayerBuffer.cpp'


::: 'testing/gtest/mozilla/GTestRunner.h'
@@ 8 @@
In a header, code inside a namespace should be indented. [whitespace/indent] [4]


::: 'testing/gtest/mozilla/GTestRunner.cpp'


::: 'xpcom/glue/standalone/nsXPCOMGlue.cpp'
@@ 420 @@
Almost always, snprintf is better than strcat [runtime/printf] [4]


::: 'gfx/tests/gtest/TestMoz2D.cpp'


Total errors found: 2
Comment on attachment 756729 [details] [diff] [review]
diff between v10, v11

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

::: config/config.mk
@@ +746,5 @@
>  
>  # MDDEPDIR is the subdirectory where dependency files are stored
>  MDDEPDIR := .deps
>  
> +EXPAND_LIBS_EXEC = $(PYTHON) $(topsrcdir)/config/expandlibs_exec.py $(if $@,--depend $(MDDEPDIR)/$(dir $@)/$(@F).pp --target $@)

These .pp files are not going to be included in rules.mk.

(Also note $(dir $@) is $(@D) )

::: toolkit/library/Makefile.in
@@ +698,4 @@
>  $(FINAL_TARGET)/dependentlibs.list.gtest: $(FINAL_TARGET)/dependentlibs.list
>   sed -e "s|$(SHARED_LIBRARY)|gtest/$(SHARED_LIBRARY)|" $< > $@
>  
> +# EXTRA_MDDEPEND_FILES = $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME).pp

I'd rather you made this work, and in all likeliness, the only reason it doesn't is that you placed this line after the rules.mk include.
Attachment #756729 - Flags: review?(ted)
Attachment #756729 - Flags: review?(mh+mozilla)
Attachment #756729 - Flags: review-
Attached patch Dual linking v12 (obsolete) — — Splinter Review 
- Fixed the ordering and enabled the line that was disabled.
- Inspected the .pp file locally
- Tested a change to a GTest only test file and checked that XUL is not relinked but gtest/XUL is.
- Tested a change to a gecko file and checked that both XUL + gtest/XUL is relinked.
Attachment #756724 - Attachment is obsolete: true
Attachment #756729 - Attachment is obsolete: true
Attachment #758761 - Flags: review?(mh+mozilla)
Comment on attachment 758761 [details] [diff] [review]
Dual linking v12

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

::: toolkit/library/Makefile.in
@@ +646,5 @@
> +else
> +EFFECTIVE_LIB_PREFIX=$(LIB_PREFIX)
> +endif
> +
> +EXTRA_MDDEPEND_FILES = $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME).pp

set GTEST_LIB = $(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME).$(DLL_SUFFIX) and use it everywhere it makes sense (so, here, and below, for the gtestxul dependency and the targets).
(Note the DLL_SUFFIX, you forgot it ; which makes me think EFFECTIVE_LIB_PREFIX should probably be DLL_PREFIX instead of LIB_PREFIX)

@@ +710,5 @@
> +endif
> +
> +ifdef LINK_GTEST
> +
> +$(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME):

Didn't you want this to be always rebuilt?
Attachment #758761 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #75)
> 
> @@ +710,5 @@
> > +endif
> > +
> > +ifdef LINK_GTEST
> > +
> > +$(EFFECTIVE_LIB_PREFIX)gtest/$(EFFECTIVE_LIB_PREFIX)$(LIBRARY_NAME):
> 
> Didn't you want this to be always rebuilt?

Only because I couldn't get dependencies to work but your suggestion in Comment 73 got things working with correct dependencies so I see no point now in always building.
(In reply to Benoit Girard (:BenWa) from comment #76)
> Only because I couldn't get dependencies to work but your suggestion in
> Comment 73 got things working with correct dependencies so I see no point
> now in always building.

Ah ok, then you don't need the empty rule if you use DLL_SUFFIX :)
https://hg.mozilla.org/mozilla-central/rev/1c257409f5a6
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 882937
Depends on: 885624
Depends on: 886079
Depends on: 886656
Depends on: 946732
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.