Closed Bug 883339 Opened 6 years ago Closed 5 years ago

Get Gtests running on Windows

Categories

(Testing :: General, defect)

x86
Windows 7
defect
Not set

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: BenWa, Assigned: glandium)

References

Details

Attachments

(2 files, 12 obsolete files)

3.28 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.11 KB, patch
ted
: review+
Details | Diff | Splinter Review
GTest fails to link on windows because of the metro library 'runtimeobject'.
Attached patch Disable GTest on windows (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #767023 - Attachment description: Disable GTest → Disable GTest on windows
Comment on attachment 767023 [details] [diff] [review]
Disable GTest on windows

wrong bug
Attachment #767023 - Attachment is obsolete: true
Assignee: bgirard → nobody
OS: Mac OS X → Windows 7
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.
Status: ASSIGNED → NEW
Summary: GTest fails to link Metro library → Get Gtests running on Windows
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)
I actually got these to run after disabling building dummyvccorlib, which technically we no longer need.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
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
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.
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.
(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.
Duplicate of this bug: 944678
Blocks: 328755
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
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)
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.
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 on attachment 8342334 [details] [diff] [review]
patch

Nice, let the bug sit long enough and it fixes itself. :)
Attachment #8342334 - Flags: review?(jmathies) → review+
Interestingly, the backout was green, but so was a PGO run triggered on the original landing. Clobber issue perhaps?
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)
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.
Yep, what Ted said!
Flags: needinfo?(ehsan)
Attached file Disable GTest on PGO builds (obsolete) —
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)
Attached patch Disable GTest on PGO builds (obsolete) — Splinter Review
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)
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-
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+
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.
Thanks I'll make sure to check that. Last I checked the file should of been there.
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
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+
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 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
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)
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)
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
mk_add_options "export MOZ_PGO=1"
(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?
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.
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)
Attached patch enable-gtest-on-windows.patch (obsolete) — Splinter Review
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)
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.
Attached file moz-pgo-log.txt (obsolete) —
(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)
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)
Attached patch enable-gtest-on-windows.patch (obsolete) — Splinter Review
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 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)
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+
(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 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)
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-
Attached patch enable-gtest-on-windows.patch (obsolete) — Splinter Review
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.
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+
sorry had to backout since this might have caused bug 1029469
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)
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+
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
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: nobody → mh+mozilla
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+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.