Closed
Bug 883339
Opened 12 years ago
Closed 10 years ago
Get Gtests running on Windows
Categories
(Testing :: General, defect)
Tracking
(firefox31 fixed, firefox32 fixed, firefox33 fixed)
RESOLVED
FIXED
mozilla33
People
(Reporter: BenWa, Assigned: glandium)
References
Details
Attachments
(2 files, 12 obsolete files)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
GTest fails to link on windows because of the metro library 'runtimeobject'.
Reporter | ||
Comment 1•11 years ago
|
||
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Reporter | ||
Updated•11 years ago
|
Attachment #767023 -
Attachment description: Disable GTest → Disable GTest on windows
Reporter | ||
Comment 2•11 years ago
|
||
Comment on attachment 767023 [details] [diff] [review]
Disable GTest on windows
wrong bug
Attachment #767023 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Assignee: bgirard → nobody
Updated•11 years ago
|
OS: Mac OS X → Windows 7
Comment 3•11 years ago
|
||
FWIW, I tried to get this going the other day. I didn't run into any problems with linking, but I did run into a problem with mach and paths in working with the gtest/xul.dll library that gets created.
Updated•11 years ago
|
Status: ASSIGNED → NEW
Summary: GTest fails to link Metro library → Get Gtests running on Windows
Comment 4•11 years ago
|
||
here's the error from running mach gtest:
1:40.50 xul.dll
1:40.59 Executing: link -NOLOGO -DLL -OUT:gtest/xul.dll -PDB:xul.pdb -SUBSYSTEM:WINDOWS -MACHINE:X86 @t:\Mozilla\MC-REL\toolkit\library\winvccorlib\tmpiakhyi.list module.res -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF ..\..\..\dist\lib\mozglue.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib
1:40.59 t:\Mozilla\MC-REL\toolkit\library\winvccorlib\tmpiakhyi.list:
1:40.59 dummyvccorlib.obj
1:40.59
1:40.59 LINK : fatal error LNK1104: cannot open file 'gtest/xul.dll'
1:40.59
1:40.59 t:/Mozilla/mc/config/rules.mk:910: recipe for target 'gtest/xul.dll' failed
1:40.59 mozmake.EXE[3]: *** [gtest/xul.dll] Error 1104
1:40.59 mozmake.EXE[3]: Leaving directory 't:/Mozilla/MC-REL/toolkit/library/winvccorlib'
1:40.59 t:/Mozilla/mc/config/recurse.mk:189: recipe for target 'libs' failed
1:40.59 mozmake.EXE[2]: *** [libs] Error 2
1:40.59 mozmake.EXE[2]: Leaving directory 't:/Mozilla/MC-REL/toolkit/library'
1:40.59 Makefile:435: recipe for target 'gtest/xul.dll' failed
1:40.59 mozmake.EXE[1]: *** [gtest/xul.dll] Error 2
1:40.59 mozmake.EXE[1]: *** Deleting file 'gtest/xul.dll'
1:40.61 mozmake.EXE[1]: Leaving directory 't:/Mozilla/MC-REL/toolkit/library'
1:40.61 Makefile:34: recipe for target 'gtest' failed
1:40.61 mozmake.EXE: *** [gtest] Error 2
1:40.61 mozmake.EXE: Leaving directory 't:/Mozilla/MC-REL/testing/gtest'
Error running mach:
['gtest']
The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You should consider filing a bug for this issue.
If filing a bug, please include the full output of mach, including this error
message.
The details of the failure are as follows:
Exception: Process executed with non-0 exit code: [u'T:/mozilla-build/msys/bin/sh.exe', u'-c', u'T:/mozilla-build/msys/bin/mozmake.EXE -C testing/gtest -j4 -s -w gtest']
File "t:\Mozilla\mc\python/mozbuild/mozbuild/mach_commands.py", line 596, in gtest
self._run_make(directory="testing/gtest", target='gtest', ensure_exit_code=True)
File "t:\Mozilla\mc\python/mozbuild\mozbuild\base.py", line 465, in _run_make
return fn(**params)
File "t:\Mozilla\mc\python/mozbuild\mozbuild\base.py", line 496, in _run_command_in_objdir
return self.run_process(cwd=self.topobjdir, **args)
File "t:\Mozilla\mc\python/mach\mach\mixin\process.py", line 137, in run_process
raise Exception('Process executed with non-0 exit code: %s' % args)
Comment 5•11 years ago
|
||
I actually got these to run after disabling building dummyvccorlib, which technically we no longer need.
Comment 6•11 years ago
|
||
Assignee: nobody → jmathies
Comment 7•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=349596fa734a
We do pretty well, except one gtest in the debug build is crashing:
GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: SendAsyncScrollDOMEvent(false, @0024EFF4 16-byte object <00-00 70-41 00-00 20-41 00-00 00-40 00-00 00-40>, @0024F014 8-byte object <00-00 48-42 00-00 48-42>)
Stack trace:
GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: SendAsyncScrollDOMEvent(false, @0024EFF4 16-byte object <00-00 70-41 00-00 20-41 00-00 00-40 00-00 00-40>, @0024F014 8-byte object <00-00 48-42 00-00 48-42>)
Stack trace:
PROCESS-CRASH | gtest | application crashed [@ xul.dll + 0x22e0a]
Crash dump filename: c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\testing\gtest\971dab95-5c37-41ee-99e6-f9f24b347dc2.dmp
Operating system: Windows NT
6.1.7601 Service Pack 1
CPU: x86
GenuineIntel family 6 model 30 stepping 5
4 CPUs
Crash reason: EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x38
Comment 8•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=c79fe297bbc3
It's not the last test that has the problem, these tests crash on shutdown. Worse, symbols for the modified xul.dll aren't available, so the crash stacks in the logs are worthless.
Reporter | ||
Comment 9•11 years ago
|
||
I landed a patch in bug 904227 to disable the gmock warning.
It would be nice to enable GTEST on window and disable the test that are busted ATM. The sooner we get gtest on window the sooner everyone will be forced to land cross platform compatible tests.
Comment 10•11 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #9)
> I landed a patch in bug 904227 to disable the gmock warning.
>
> It would be nice to enable GTEST on window and disable the test that are
> busted ATM. The sooner we get gtest on window the sooner everyone will be
> forced to land cross platform compatible tests.
I'm still poking at this, I need to get the tests slowed down so I can attach a debugger and hopefully identify the cause of the shutdown crash.
Comment 12•11 years ago
|
||
I'm not going to be able to get back to this for a few weeks or so, so if someone wants to pick this up feel free. The two remaining issues are (1) these tests always crash on shutdown, and (2) no symbols for the modified xul.dll make diagnosing crashes in automated tests painful.
Assignee: jmathies → nobody
Comment 13•11 years ago
|
||
I attached via gflags. There is a null DrawTarget here:
xul!mozilla::layers::BufferTextureClient::UpdateSurface
xul!TestTextureClientSurface
xul!Layers_TextureSerialization_Test::TestBody
xul!testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void>
xul!testing::Test::Run
This is because the backend is D2D and Factory::CreateDrawTargetForData doesn't support that backend. (Something similar came up in bug 946501)
Reporter | ||
Comment 14•11 years ago
|
||
Just don't compile the TextureSerialization test file on windows. It's better to get these test enabled now then wait for this stuff to be fixed.
Reporter | ||
Comment 15•11 years ago
|
||
Assignee: nobody → bgirard
Reporter | ||
Comment 16•11 years ago
|
||
Comment on attachment 8342334 [details] [diff] [review]
patch
Not sure who is a good reviewer for this? Jimm?
This is passing tests now.
Attachment #8342334 -
Flags: review?(jmathies)
Comment 17•11 years ago
|
||
Comment on attachment 8342334 [details] [diff] [review]
patch
Nice, let the bug sit long enough and it fixes itself. :)
Attachment #8342334 -
Flags: review?(jmathies) → review+
Reporter | ||
Comment 18•11 years ago
|
||
Forgot to try on 64-bit:
https://tbpl.mozilla.org/?tree=Try&rev=c8f5f898a069
Reporter | ||
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
Comment 21•11 years ago
|
||
Interestingly, the backout was green, but so was a PGO run triggered on the original landing. Clobber issue perhaps?
Reporter | ||
Comment 22•11 years ago
|
||
Ehsan are PGO linking much slower then normal linking? Any idea what the timeout for PGO links are? This patch causes us to link gtestxul from make check where I assume the timeout is insufficient for a PGO build.
Flags: needinfo?(ehsan)
Comment 23•11 years ago
|
||
Yes, PGO links take over an hour. The linker has to do all the actual codegen + the optimization passes take a long time. I'd suggest we just don't do gtests on PGO builds (at least on Windows) because of the link time expenditure.
Reporter | ||
Comment 25•11 years ago
|
||
This patch disables GTest for all PGO builds. We could maybe disable it just for windows.
Attachment #8342334 -
Attachment is obsolete: true
Attachment #8375545 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 26•11 years ago
|
||
PGO try run:
https://tbpl.mozilla.org/?tree=Try&rev=bbfc1bf19048
Without:
https://tbpl.mozilla.org/?tree=Try&rev=2dd745fffab0
Attachment #8375545 -
Attachment is obsolete: true
Attachment #8375545 -
Flags: review?(mh+mozilla)
Attachment #8375770 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 8375770 [details] [diff] [review]
Disable GTest on PGO builds
Review of attachment 8375770 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/gtest/Makefile.in
@@ +18,2 @@
> ifdef COMPILE_ENVIRONMENT
> +ifndef MOZ_PGO
Just disable when MOZ_PROFILE_GENERATE and MOZ_PROFILE_USE are defined, instead of AC_SUBSTing MOZ_PGO.
Attachment #8375770 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 28•11 years ago
|
||
Comment on attachment 8375770 [details] [diff] [review]
Disable GTest on PGO builds
Review of attachment 8375770 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/gtest/Makefile.in
@@ +18,2 @@
> ifdef COMPILE_ENVIRONMENT
> +ifndef MOZ_PGO
... which doesn't work during make check. sigh.
Attachment #8375770 -
Flags: review- → review+
Comment 29•11 years ago
|
||
I have issue when I run "mach gtest" on windows and get error "Couldn't load XPCOM"
And I want to give you my thoughts about it.
InitXPCOMGlue("..\\firefox")
{
...
XPCOMGlueStartup("..\\xul.dll")
...
}
XPCOMGlueStartup("..\\xul.dll")
{
...
if (getenv("MOZ_RUN_GTEST")) {
strcat(xpcomDir, ".gtest"); // (*)
}
...
flist = TS_tfopen(xpcomDir, READ_TEXTMODE);
if (!flist) {
return nullptr;
}
...
}
Before (*) we have xpcomDir = "..\\dependentlibs.list"
If we have MOZ_RUN_GTEST, after (*) we will have "..\\dependentlibs.list.gtest"
If we have no file "dependentlibs.list.gtest" we can get message "Couldn't load XPCOM"
I hope that my information will be useful and helpful for You.
Reporter | ||
Comment 30•11 years ago
|
||
Thanks I'll make sure to check that. Last I checked the file should of been there.
Comment 31•11 years ago
|
||
Attachment #8375770 -
Attachment is obsolete: true
Attachment #8391427 -
Flags: review+
Comment 32•11 years ago
|
||
Attachment #8391428 -
Flags: review?(bgirard)
Comment 33•11 years ago
|
||
BenWa's patch hasn't been checked in yet because apparently it doesn't actually disable GTest on Windows PGO builds:
<briansmith> BenWa: Is it OK to check in the patch in bug 883339? It would be very helpful to me.
<BenWa> briansmith: My patch to disable GTEST on PGO is wrong
<briansmith> Oh, I thought glandium had said it was wrong and then changed his mind
<briansmith> r- -> r+
<BenWa> comment 26 pushed this patch to try and it still tried to run gest on PGO
<BenWa> So something is wrong somewhere
Reporter | ||
Comment 34•11 years ago
|
||
Comment on attachment 8391428 [details] [diff] [review]
Disable test Layers.TextureSerialization on Windows to work around bug 983786
Review of attachment 8391428 [details] [diff] [review]:
-----------------------------------------------------------------
Anything to get unit test coverage running on windows.
Attachment #8391428 -
Flags: review?(bgirard) → review+
Comment 35•11 years ago
|
||
Looks like now gtest can work on windows
(at least from folder gfx/tests/gtest we can get logs from all tests from this folder)
https://tbpl.mozilla.org/?tree=Try&rev=e248e3d1d160
Comment 36•11 years ago
|
||
Comment on attachment 8391428 [details] [diff] [review]
Disable test Layers.TextureSerialization on Windows to work around bug 983786
This test passes now so this patch isn't needed.
Attachment #8391428 -
Attachment is obsolete: true
Comment 37•11 years ago
|
||
I found that this bug doesn't really depend on bug 1018402 after all; see bug 1018402 for more details.
gps, Could you please find somebody that understand the build system enough to help with this patch? The problem is that we're trying to make it so that GTests link and run in all configurations EXCEPT PGO builds because relinking libxul causes the build to timeout. Benoit has written a patch that seems like it should be doing that, but somehow the linking is still timing out. I would really appreciate the help here because we've written, and are writing, a lot of tests for mozilla::pkix (the new certificate verification code) as GTests, and it is important for us that those tests run on Windows too. Thanks!
No longer depends on: 1018402
Flags: needinfo?(gps)
Comment 38•11 years ago
|
||
glandium and mshal know linking the best and can be your point of contact.
I don't have permission to access bug 1018402. Can you please ensure we are CCd?
I agree we should get gtests running on Windows and start to write gtests over vanilla C++ testing programs - gtest really is a great C++ testing framework. I just don't know how busy everyone is. Personally, my only priority ATM is bug 928173.
Flags: needinfo?(gps)
Comment 39•11 years ago
|
||
I now know why Benoit's patch doesn't work:
When you use mk_add_options in a .mozconfig, that option isn't propagated anywhere EXCEPT TO client.mk. See the various mozconfig bits in client.mk and notice the comment in client.mk that calls out special handling for MOZ_PGO! That means that if we want to use mk_add_options to set MOZ_PGO that can be seen during the linking and running of GTests, then you need to execute the linking and running through client.mk, e.g. by moving the contents of testing/gtest/Makefile.in to testing/testsuite-targets and then twiddle client.mk a little bit. I have a patch that does this that I am currently testing.
Note that this is still sub-optimal because on PGO builds where GTest is disabled we'll still end up *compiling* the GTests that will never get linked, which is a waste. I think there are two better long-term fixes: (1) Enable GTests automatically with --enable-tests, and create a --disable-gtest configure option, then make Windows PGO builds use --disable-gtest, so that we can skip all the GTest-related work on Windows PGO builds, or (2) Move the enabling/disabling of PGO to a configure option and change configure.in to automatically disable GTests on Windows PGO builds.
Assignee: bgirard → brian
Status: NEW → ASSIGNED
Assignee | ||
Comment 40•11 years ago
|
||
mk_add_options "export MOZ_PGO=1"
Comment 41•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #40)
> mk_add_options "export MOZ_PGO=1"
I'm not sure what you mean here. None of the existing mozconfigs in the tree that enable PGO use "export" and the documentation for enabling PGO in local and/or try builds doesn't use "export" either. Are you suggesting I should change all the mozconfigs to use "export" instead of what I'm doing (adding "gtest" and "check" targets to client.mk), or something else?
Assignee | ||
Comment 42•11 years ago
|
||
I'm saying there's a way to pass down MOZ_PGO to the build system. Whether you want to use that or not is up to you.
Assignee | ||
Comment 43•11 years ago
|
||
Also note that the way PGO builds are done on mozilla-central, MOZ_PGO *is* passed down to the build system (it's given on the make command line, and inherited as such)
Comment 44•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9d8bacbdcee0
I would not be surprised to hear I am doing something dumb here and any/all guidance you can give me to correct any misunderstandings I have is appreciated, as I know nothing about our build system. And, since I'm not getting paid to work on this, extraordinary hand-holding is especially appreciated.
Attachment #8391427 -
Attachment is obsolete: true
Attachment #8436642 -
Flags: review?(mh+mozilla)
Comment 45•11 years ago
|
||
I'm pretty sure this isn't the route we want to take. (Not least because we want to get rid of client.mk eventually.) I think the right approach would be to just AC_SUBST(MOZ_PGO) in configure and then use it where needed.
Comment 46•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #45)
> I'm pretty sure this isn't the route we want to take. (Not least because we
> want to get rid of client.mk eventually.) I think the right approach would
> be to just AC_SUBST(MOZ_PGO) in configure and then use it where needed.
Fair enough. My patch didn't work. But, with my patch, when we run the make check, make prints out the environment. It appears make check is being run without MOZ_PGO=1 on the command line and it also appears that make check cannot find the MOZCONFIG either.
Notice how other invocations of make result in this output:
Adding client.mk options from c:/builds/moz2_slave/try-w32-0000000000000000000000/build/.mozconfig:
AUTOCLOBBER=1
export LIB=c:\\Program Files (x86)\\Windows Kits\\8.0\\Lib\\win8\\um\\x86;c:\\PROGRA~2\\MICROS~2.0\\vc\\lib;c:\\PROGRA~2\\MICROS~2.0\\vc\\atlmfc\\lib;c:\\tools\\sdks\\dx10\\lib
export LIBPATH=c:\\Program Files (x86)\\Windows Kits\\8.0\\Lib\\win8\\um\\x86;c:\\PROGRA~2\\MICROS~2.0\\vc\\lib;c:\\PROGRA~2\\MICROS~2.0\\vc\\atlmfc\\lib;c:\\tools\\sdks\\dx10\\lib
export PATH=c:\\Program Files (x86)\\Windows Kits\\8.0\\bin\\x86;c:\\PROGRA~2\\MICROS~2.0\\Common7\\IDE;c:\\PROGRA~2\\MICROS~2.0\\VC\\BIN;c:\\PROGRA~2\\MICROS~2.0\\Common7\\Tools;c:\\PROGRA~2\\MICROS~2.0\\VC\\VCPackages;c:\\mozilla-build\\moztools;c:\\Program Files (x86)\\Windows Kits\\8.0\\bin\\x86;c:\\PROGRA~2\\MICROS~2.0\\Common7\\IDE;c:\\PROGRA~2\\MICROS~2.0\\VC\\BIN;c:\\PROGRA~2\\MICROS~2.0\\Common7\\Tools;c:\\PROGRA~2\\MICROS~2.0\\VC\\VCPackages;c:\\mozilla-build\\moztools;c:\\mozilla-build\\python27;c:\\mozilla-build\\buildbotve\\scripts;C:\\mozilla-build\\msys\\local\\bin;c:\\mozilla-build\\wget;c:\\mozilla-build\\7zip;c:\\mozilla-build\\blat261\\full;c:\\mozilla-build\\python;c:\\mozilla-build\\svn-win32-1.6.3\\bin;c:\\mozilla-build\\upx203w;c:\\mozilla-build\\emacs-22.3\\bin;c:\\mozilla-build\\info-zip;c:\\mozilla-build\\nsis-2.22;c:\\mozilla-build\\nsis-2.33u;c:\\mozilla-build\\nsis-2.46u;c:\\mozilla-build\\wix-351728;c:\\mozilla-build\\hg;c:\\mozilla-build\\python\\Scripts;c:\\mozilla-build\\kdiff3;c:\\mozilla-build\\yasm;.;C:\\mozilla-build\\msys\\local\\bin;C:\\mozilla-build\\msys\\mingw\\bin;C:\\mozilla-build\\msys\\bin;c:\\windows\\system32;c:\\windows;c:\\windows\\System32\\Wbem;c:\\windows\\System32\\WindowsPowerShell\\v1.0\\;c:\\mozilla-build;c:\\mozilla-build\\python27;c:\\mozilla-build\\python27\\Scripts;C:\\mozilla-build\\msys\\bin;c:\\mozilla-build\\vim\\vim72;c:\\mozilla-build\\wget;c:\\mozilla-build\\info-zip;c:\\CoreUtils\\bin;c:\\mozilla-build\\buildbotve\\scripts;c:\\Program Files (x86)\\Microsoft SQL Server\\100\\Tools\\Binn\\;c:\\Program Files\\Microsoft SQL Server\\100\\Tools\\Binn\\;c:\\Program Files\\Microsoft SQL Server\\100\\DTS\\Binn\\;c:\\Program Files (x86)\\Windows Kits\\8.0\\Windows Performance Toolkit\\;c:\\mozilla-build\\moztools-x64\\bin;c:\\mozilla-build\\vim\\vim72
export INCLUDE=c:\\Program Files (x86)\\Windows Kits\\8.0\\include\\shared;c:\\Program Files (x86)\\Windows Kits\\8.0\\include\\um;c:\\Program Files (x86)\\Windows Kits\\8.0\\include\\winrt;c:\\Program Files (x86)\\Windows Kits\\8.0\\include\\winrt\\wrl;c:\\Program Files (x86)\\Windows Kits\\8.0\\include\\winrt\\wrl\\wrappers;c:\\PROGRA~2\\MICROS~2.0\\vc\\include;c:\\PROGRA~2\\MICROS~2.0\\vc\\atlmfc\\include;c:\\tools\\sdks\\dx10\\include
export WIN32_REDIST_DIR=c:/PROGRA~2/MICROS~2.0/VC/redist/x86/Microsoft.VC100.CRT
export MOZ_TOOLS=C:/mozilla-build/moztools
export SCCACHE_NO_HTTPS=1
export SCCACHE_BUCKET=mozilla-releng-ceph-cache-scl3-try
MOZ_PREFLIGHT_ALL+=build/sccache.mk
MOZ_POSTFLIGHT_ALL+=build/sccache.mk
export CC_WRAPPER=
export CXX_WRAPPER=
export COMPILE_PDB_FLAG=
export HOST_PDB_FLAG=
export MOZ_DEBUG_FLAGS=-Z7
MOZ_PGO=1
FOUND_MOZCONFIG := c:/builds/moz2_slave/try-w32-0000000000000000000000/build/.mozconfig
But, there is no such output for the make check step.
It seems like the build/test infrastructure is using some kind of custom script to execute "make check" that isn't in the Gecko source tree. Does anybody know where those scripts are located and how to change them?
Flags: needinfo?(ted)
Comment 47•11 years ago
|
||
It's kind of inconsequential. If you add an AC_SUBST like I suggested above, then when configure is run you can save the value of the MOZ_PGO variable and have it available in all future make invocations, which will solve the problem. It's not already that way because nobody had a need for it until this point.
That debug spew is from client.mk itself:
http://mxr.mozilla.org/mozilla-central/source/client.mk#219
`make check` on TBPL doesn't spit it out because the builders simply execute `make check` directly in the objdir.
Flags: needinfo?(ted)
Updated•11 years ago
|
Attachment #8436642 -
Flags: review?(mh+mozilla)
Comment 48•11 years ago
|
||
Similar (but slightly different) to what I said in my previous message, the root cause of the failure of Benoit's patch is that MOZ_PGO didn't get propagated to the configure step. This patch propagates MOZ_PGO to configure from client.mk. glandium's review comment on Benoit's patch indicated that he didn't want MOZ_PGO to be propogated down to other Makefiles so instead I AC_SUBST a specific variable controlling whether GTest should be disabled during "make check".
Note that my previous comments about adding --disable-gtest and similar to configure were based on an out-of-date understanding. It turns out that for the most part things already work like I was wanting.
https://tbpl.mozilla.org/?tree=Try&rev=78d324269892
Attachment #8436642 -
Attachment is obsolete: true
Attachment #8436991 -
Attachment is obsolete: true
Comment 49•11 years ago
|
||
Comment on attachment 8437161 [details] [diff] [review]
enable-gtest-on-windows.patch
The try run passed. The output of the GTest "make check" step in the log was:
echo GTest skipped during make check
GTest skipped during make check
Attachment #8437161 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 50•11 years ago
|
||
Comment on attachment 8437161 [details] [diff] [review]
enable-gtest-on-windows.patch
Review of attachment 8437161 [details] [diff] [review]:
-----------------------------------------------------------------
::: client.mk
@@ +329,5 @@
> $(NULL)
>
> CONFIGURE_ENV_ARGS += \
> MAKE='$(MAKE)' \
> + MOZ_PGO=$(MOZ_PGO) \
I'd rather "export MOZ_PGO" somewhere around export AUTOCLOBBER, and AC_SUBST(MOZ_PGO) in configure.in. That should replace both changes in client.mk.
::: configure.in
@@ +6439,5 @@
> AC_DEFINE_UNQUOTED(GTEST_HAS_RTTI, 0)
> AC_SUBST(GTEST_HAS_RTTI)
> if test -n "$_WIN32_MSVC"; then
> + AC_DEFINE_UNQUOTED(_VARIADIC_MAX, 10)
> +
trailing whitespaces
@@ +6445,5 @@
> + dnl so disable GTests on Windows PGO builds.
> + if test -n "$MOZ_PGO"; then
> + SKIP_GTEST_DURING_MAKE_CHECK=1
> + AC_SUBST(SKIP_GTEST_DURING_MAKE_CHECK)
> + fi
The check should be kept in Makefile.in
::: testing/gtest/Makefile.in
@@ +22,5 @@
> endif
> $(PYTHON) $(topsrcdir)/testing/gtest/rungtests.py --xre-path=$(DIST)/bin --symbols-path=$(DIST)/crashreporter-symbols $(DIST)/bin/$(MOZ_APP_NAME)$(BIN_SUFFIX)
> +else
> +check::
> + $(ECHO) GTest skipped during make check
@$(ECHO)
Note $(ECHO) displays nothing when running make -s. If that what you intend?
Attachment #8437161 -
Flags: review?(mh+mozilla) → feedback+
Comment 51•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #50)
> I'd rather "export MOZ_PGO" somewhere around export AUTOCLOBBER, and
> AC_SUBST(MOZ_PGO) in configure.in. That should replace both changes in
> client.mk.
Mike, I think there's one advantage to my current approach that isn't obvious. With my approach, one can run this to run the GTests to run during `make check` locally:
mozmake SKIP_GTEST_DURING_MAKE_CHECK= -C ../ff-pgo check
With your suggested approach, this would work:
mozmake MOZ_PGO= -C ../ff-pgo check
But, that depends on there not being any other side-effects from setting MOZ_PGO=. I think it is better to have the specific setting to control this one aspect than to abuse the MOZ_PGO setting like this. What do you think?
> The check should be kept in Makefile.in
I'd like to suggest another alternative: How about, in the mozconfigs that are used on tbpl, we set SKIP_GTEST_DURING_MAKE_CHECK=, and then AC_SUBST(SKIP_GTEST_DURING_MAKE_CHECK) in configure.in. That way, GTests will be enabled by default locally and only disabled for TBPL runs. This seems ideal to me. Then I could the check from configure.in.
> > +check::
> > + $(ECHO) GTest skipped during make check
>
> @$(ECHO)
>
> Note $(ECHO) displays nothing when running make -s. If that what you intend?
I don't know. I'm not sure it matters too much. Do you have a recommendation of something else to use instead?
Thanks for your help on this.
Flags: needinfo?(mh+mozilla)
Comment 52•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a2fd8d1cd161
https://tbpl.mozilla.org/?tree=Try&rev=fc46afe8e966
This patch implements the approach from my previous comment.
Attachment #8437161 -
Attachment is obsolete: true
Comment 53•11 years ago
|
||
Fix MOZCONFIG whitelist:
https://tbpl.mozilla.org/?tree=Try&rev=02de3d623c36
https://tbpl.mozilla.org/?tree=Try&rev=cb16419dfb5c
Attachment #8438628 -
Attachment is obsolete: true
Comment 54•11 years ago
|
||
Comment on attachment 8438666 [details] [diff] [review]
enable-gtest-on-windows.patch
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #53)
> https://tbpl.mozilla.org/?tree=Try&rev=02de3d623c36
This shows that non-PGO builds, including Windows XP Opt and Debug, are running the GTests (search the build log for "GTest unit test starting").
> https://tbpl.mozilla.org/?tree=Try&rev=cb16419dfb5c
This shows that the build and make check passes when MOZ_PGO=1 and SKIP_GTEST_DURING_MAKE_CHECK=1 on all platforms, except for platforms where MOZ_PGO=1 breaks the build all on its own (Android).
Also, I verified locally that "make check" in PGO and non-PGO builds will run the GTests. That is, GTests are always enabled locally.
Attachment #8438666 -
Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 55•11 years ago
|
||
Comment on attachment 8438666 [details] [diff] [review]
enable-gtest-on-windows.patch
Review of attachment 8438666 [details] [diff] [review]:
-----------------------------------------------------------------
This leaves out gtest on periodic pgo builds, and "manual" pgo builds on try. Really, building gtest with pgo ought to be made opt-in, not opt-out. I'm even tempted to just ignore gtest+pgo until someone actually complains he needs to do such test.
Attachment #8438666 -
Flags: review?(mh+mozilla) → review-
Updated•10 years ago
|
Comment 56•10 years ago
|
||
Thanks for the review. I have modified my patch to do things the way you requested.
Non-PGO try run:
https://tbpl.mozilla.org/?tree=Try&pusher=brian@briansmith.org
"TEST-INFO | GTest unit test starting" on Windows XP.
PGO try run:
https://tbpl.mozilla.org/?tree=Try&rev=1ff821afbd33
"GTest skipped during make check" on Windows XP.
Updated•10 years ago
|
Attachment #8442263 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 57•10 years ago
|
||
Comment on attachment 8442263 [details] [diff] [review]
enable-gtest-on-windows.patch
Review of attachment 8442263 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/gtest/Makefile.in
@@ +7,5 @@
>
> include $(topsrcdir)/config/rules.mk
>
> +# Bug 1027228: Linking xul-gtest.dll times out on TBPL builds, so we disable
> +# GTest on Windows PGO builds.
The real reason is that it doubles the build times, that are already very long. Timeouts are an unfortunate bug that's going to disappear. I don't think we should enable gtests blindly when it does.
@@ +28,5 @@
> endif
> $(PYTHON) $(topsrcdir)/testing/gtest/rungtests.py --xre-path=$(DIST)/bin --symbols-path=$(DIST)/crashreporter-symbols $(DIST)/bin/$(MOZ_APP_NAME)$(BIN_SUFFIX)
> +else
> +check::
> + $(ECHO) GTest skipped during make check
Just use echo.
Attachment #8442263 -
Flags: review?(mh+mozilla) → review+
Comment 58•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5789d0e7e0
Thanks for the review!
Comment 59•10 years ago
|
||
sorry had to backout since this might have caused bug 1029469
Comment 60•10 years ago
|
||
Mike, let's check in this variant of the patch, which lets "mach gtest" work locally. It sucks having this patch in my patch queue because it modifies configure.in which causes my builds to be slow after rebasing.
Attachment #8438666 -
Attachment is obsolete: true
Attachment #8442263 -
Attachment is obsolete: true
Attachment #8445241 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 61•10 years ago
|
||
Comment on attachment 8445241 [details] [diff] [review]
Enable "mach gtest" on Windows but leave GTest disabled for "make check"
Review of attachment 8445241 [details] [diff] [review]:
-----------------------------------------------------------------
r+, but you'll need a [leave open] in the whiteboard, since this doesn't actually enable gtests on windows.
Attachment #8445241 -
Flags: review?(mh+mozilla) → review+
Comment 62•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2dce0704b69
Thanks for the quick review!
Keywords: leave-open
Comment 63•10 years ago
|
||
I don't have more time to spend on this. Thanks for all the help that resulted in the minor improvement I was able to make.
Currently, "mach gtest" works on Windows but the GTests don't run on Windows TBPL builds or during "make check." Bug 1029469 comment 4 explains what needs to be done to enable GTest during "make check" on non-PGO TBPL builds.
Assignee: brian → nobody
Assignee | ||
Comment 64•10 years ago
|
||
The testing/gtest/Makefile.in part comes from Brian's first landing and was already reviewed by myself. The rest is to work around bug 1029469, which, at this point, I don't have good ideas how to fix.
Attachment #8445749 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Comment 65•10 years ago
|
||
Comment on attachment 8445749 [details] [diff] [review]
Enable gtest on windows TBPL non-PGO builds
Review of attachment 8445749 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/gtest/Makefile.in
@@ +8,5 @@
> include $(topsrcdir)/config/rules.mk
>
> # Bug 1028035: Linking xul-gtest.dll takes too long, so we disable GTest on
> # Windows PGO builds.
> +ifeq (1WINNT,$(MOZ_PGO)$(OS_ARCH))
I think we usually use a _ in between conditionals like this to make it a little easier to read.
Attachment #8445749 -
Flags: review?(ted) → review+
Comment 66•10 years ago
|
||
Assignee | ||
Comment 67•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 68•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla33
Comment 69•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e32c33cfb90
https://hg.mozilla.org/releases/mozilla-aurora/rev/22a274158290
https://hg.mozilla.org/releases/mozilla-beta/rev/019bb4a42fb7
https://hg.mozilla.org/releases/mozilla-beta/rev/4dcb1ccdd2e1
Uplifted as part of the uplifting for bug 1031542, since the tests are GTests.
status-firefox31:
--- → fixed
status-firefox32:
--- → fixed
Updated•10 years ago
|
status-firefox33:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•