Closed
Bug 844288
Opened 10 years ago
Closed 9 years ago
Dual link libxul.so and libxul-unit.so and replace enable-gtest by enable-test
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 16 obsolete files)
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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)
Updated•10 years ago
|
Component: General → Build Config
Product: Testing → Core
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
My WIP was made difficult because of libxpcom but with bug 852950 this is no longer an issue.
Depends on: 852950
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #741077 -
Flags: review?(gps)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Forgot to build gtest before packaging.
Attachment #741077 -
Attachment is obsolete: true
Attachment #741077 -
Flags: review?(gps)
Attachment #741083 -
Flags: review?(gps)
Comment 11•9 years ago
|
||
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 :)
Updated•9 years ago
|
Attachment #741080 -
Flags: review?(gps) → review?(ted)
Updated•9 years ago
|
Attachment #741083 -
Flags: review?(gps) → review?(ted)
Assignee | ||
Comment 12•9 years ago
|
||
Run GTest on make check for now: https://tbpl.mozilla.org/?tree=Try&rev=40ba847cc0ff
Assignee | ||
Comment 13•9 years ago
|
||
I think I need gtest to be a .PHONY target: https://tbpl.mozilla.org/?tree=Try&rev=09e80ef3e6fb
Assignee | ||
Comment 14•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f0944704e0ee
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=def17f2c7a7e
Comment 18•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #743124 -
Flags: review?(benjamin) → review?(mh+mozilla)
Comment 19•9 years ago
|
||
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-
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Comment 23•9 years ago
|
||
(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.
Assignee | ||
Comment 24•9 years ago
|
||
toolkit/library/Makefile.in is heavily updated
Attachment #743124 -
Attachment is obsolete: true
Comment 25•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #742547 -
Flags: review?(ted) → review+
Assignee | ||
Comment 26•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2dba88c632dc
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #744808 -
Attachment is obsolete: true
Assignee | ||
Comment 28•9 years ago
|
||
Try to fix a bug with the Makefile causing fat binary to fail: https://tbpl.mozilla.org/?tree=Try&rev=e4a6e60e444c
Assignee | ||
Updated•9 years ago
|
Attachment #745887 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 29•9 years ago
|
||
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 30•9 years ago
|
||
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-
Comment 31•9 years ago
|
||
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 $@))
Assignee | ||
Comment 32•9 years ago
|
||
(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.
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #745887 -
Attachment is obsolete: true
Attachment #746993 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 34•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1353d81fc661
Assignee | ||
Comment 35•9 years ago
|
||
Apparently runtimeobject is no longer needed. Trying to remove it: https://tbpl.mozilla.org/?tree=Try&rev=5274db12c759
Assignee | ||
Comment 36•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a30f6618f7fd
Comment 37•9 years ago
|
||
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-
Assignee | ||
Comment 38•9 years ago
|
||
(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.
Comment 39•9 years ago
|
||
(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.
Assignee | ||
Comment 40•9 years ago
|
||
(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.
Assignee | ||
Comment 41•9 years ago
|
||
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)
Assignee | ||
Comment 42•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0705062ef7ad
Assignee | ||
Comment 43•9 years ago
|
||
Fix the OSX b2g build: https://tbpl.mozilla.org/?tree=Try&rev=ed4f1eaf6cfa
Comment 44•9 years ago
|
||
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)
Assignee | ||
Comment 45•9 years ago
|
||
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.
Assignee | ||
Comment 46•9 years ago
|
||
Attachment #750015 -
Attachment is obsolete: true
Assignee | ||
Comment 47•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=50d579bb439d
Assignee | ||
Comment 48•9 years ago
|
||
Attachment #754978 -
Attachment is obsolete: true
Attachment #755473 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 49•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ea4349e627db
Comment 50•9 years ago
|
||
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+
Assignee | ||
Comment 51•9 years ago
|
||
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 52•9 years ago
|
||
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.
Assignee | ||
Comment 53•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fe7d11f20033
Assignee | ||
Comment 54•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/816311e43409
Whiteboard: [keep open]
Comment 55•9 years ago
|
||
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.
Comment 56•9 years ago
|
||
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.
Comment 57•9 years ago
|
||
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.
Assignee | ||
Comment 58•9 years ago
|
||
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.
Assignee | ||
Comment 59•9 years ago
|
||
Relanded per IRC discussion: http://hg.mozilla.org/integration/mozilla-inbound/rev/dc76402b8471
Comment 60•9 years ago
|
||
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);
Assignee | ||
Comment 61•9 years ago
|
||
(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.
Assignee | ||
Comment 62•9 years ago
|
||
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
Assignee | ||
Comment 63•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Whiteboard: [keep open]
Comment 64•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dc76402b8471
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
![]() |
||
Comment 65•9 years ago
|
||
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.
Comment 66•9 years ago
|
||
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 → ---
![]() |
||
Updated•9 years ago
|
Assignee | ||
Comment 67•9 years ago
|
||
Attachment #755473 -
Attachment is obsolete: true
Assignee | ||
Comment 68•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=24c2566c6ee3
Assignee | ||
Comment 69•9 years ago
|
||
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)
Comment 70•9 years ago
|
||
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)
Comment 71•9 years ago
|
||
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)
Comment 72•9 years ago
|
||
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 73•9 years ago
|
||
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-
Assignee | ||
Comment 74•9 years ago
|
||
- 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 75•9 years ago
|
||
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+
Assignee | ||
Comment 76•9 years ago
|
||
(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.
Comment 77•9 years ago
|
||
(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 :)
Assignee | ||
Comment 78•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ad08d20e7113
Attachment #758761 -
Attachment is obsolete: true
Attachment #761552 -
Flags: review+
Assignee | ||
Comment 79•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c257409f5a6
Comment 80•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1c257409f5a6
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•