Bug 175722 – Update googletest

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sam Weinig]

The version of googletest we use for TestWebKitAPI is version 1.5.0, which is pretty old at this point. We should update to the latest (1.7.0 as of this filing) and has some features that would be useful, including the ability to use EXPECT_EQ on types that do not have an operator<< specified.

[Yusuke Suzuki]

Created attachment 320105 [details]
Patch

[Sam Weinig]

Thanks Yusuke!

[Yusuke Suzuki]

Created attachment 320249 [details]
Patch

[Yusuke Suzuki]

Created attachment 320284 [details]
Patch

[Darin Adler]

Comment on attachment 320284 [details]
Patch

When importing a whole directory, I don’t think the ChangeLog should list the individual files as if we were doing the programming as part of this project. Just one big one like this should do:

    gtest: Replaced with a new version.

If changes to our code for compatibility with the new gtest were needed, for those we should use the normal change log style.

[Alexey Proskuryakov]

Comment on attachment 320284 [details]
Patch

Marking r- since the patch breaks the build.

[Tim Horton]

I wonder if this will mean we can remove the GTEST_HAS_TR1_TUPLE=0 hack that is in a few places in our project.

[Don Olmstead]

It sounds like there might be a 1.8.1 release in the near future so perhaps we should revisit this work.

https://github.com/google/googletest/issues/1079#issuecomment-412532869

After that the 1.8.1 branch goes into maintenance and a 1.9 release will drop pre C++11 support.

https://github.com/google/googletest/issues/1366

[Ross Kirsling]

Created attachment 348556 [details]
Patch

[Ross Kirsling]

Comment on attachment 348556 [details]
Patch

This updates gtest to HEAD and ensures that we don't stick any WebKit stuff into gtest itself this time.

In doing so, there was just one missing include in WebCore that blocked compilation -- otherwise it was as simple as adding wtf/Assertions.h (and FastMalloc.h and DisallowCType.h for good measure) to TestWebKitAPI/config.h.

[Ross Kirsling]

Created attachment 348561 [details]
Patch

[Ross Kirsling]

Confirmed locally that Mac and WinCairo API tests all pass locally with this patch. :D

[Don Olmstead]

This WPE EWS bot is currently borked. Its giving the same errors across different patches.

Michael could you maybe make sure GTK and WPE run API tests successfully?

[Ross Kirsling]

(In reply to Tim Horton from comment #7)
> I wonder if this will mean we can remove the GTEST_HAS_TR1_TUPLE=0 hack that
> is in a few places in our project.

Regarding this comment, the only place I see this occurring is here:
https://github.com/WebKit/webkit/blob/master/Tools/TestWebKitAPI/Configurations/Base.xcconfig#L38

But I believe we can remove GTEST_HAS_TR1_TUPLE=0 as well as GTEST_HAS_RTTI=0 there, as these were also removed from gtest/xcode/Config/General.xcconfig.

[Ross Kirsling]

Created attachment 348573 [details]
Patch

[Michael Catanzaro]

(In reply to Don Olmstead from comment #13)
> This WPE EWS bot is currently borked. Its giving the same errors across
> different patches.
> 
> Michael could you maybe make sure GTK and WPE run API tests successfully?

The buildbot (separate from EWS) is broken since r235531 "[CMake] Replace AVFoundationSupport.py using CMake" and is showing the same error. So I think the EWS bot is fine, and the build is just broken.

I have no clue why it broke, maybe one of the changes in WebKitFeatures.cmake?

[Don Olmstead]

(In reply to Michael Catanzaro from comment #16)
> (In reply to Don Olmstead from comment #13)
> > This WPE EWS bot is currently borked. Its giving the same errors across
> > different patches.
> > 
> > Michael could you maybe make sure GTK and WPE run API tests successfully?
> 
> The buildbot (separate from EWS) is broken since r235531 "[CMake] Replace
> AVFoundationSupport.py using CMake" and is showing the same error. So I
> think the EWS bot is fine, and the build is just broken.
> 
> I have no clue why it broke, maybe one of the changes in
> WebKitFeatures.cmake?

Weird I swore previous patches for that one passed.

Is the bundling of the unified build deterministic? There’s always a chance that if the sets change that there can be a build breakage.

[Michael Catanzaro]

It is deterministic, but yes if files are added or removed that will change the sets.

[Ross Kirsling]

Created attachment 348597 [details]
Patch

[Ross Kirsling]

(In reply to Ross Kirsling from comment #19)
> Created attachment 348597 [details]
> Patch

Resubmitted now that WPE is stable again.

[Don Olmstead]

I believe this is ready for a review. We can follow up with whatever changes happen between this point in gtest master and 1.8.1.

There hasn't been a release to gtest in over 2 years and they keep teasing a release soon but who realistically knows. I would assume there will be a 1.8.1 release right after we land this of course because that's how the world works.

[Brent Fulgham]

Comment on attachment 348597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348597&action=review

Overall this patch looks fine, but I think the Project file changes in ThirdParty/gtest are wrong. It seems to lose our Production build target, and does not understand that we might build with our Internal SDK versus the external developer's SDK. I can't approve the patch in this form.

> Source/ThirdParty/gtest/xcode/Config/DebugProject.xcconfig:-39
> -SDKROOT_YES = macosx.internal;

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/Config/FrameworkTarget.xcconfig:-19
> -INSTALL_PATH = $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks;

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/Config/General.xcconfig:-19
> -ARCHS = $(ARCHS_STANDARD_32_64_BIT);

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/Config/General.xcconfig:20
> +WARNING_CFLAGS = -Wall -Werror -Wendif-labels -Wnewline-eof -Wno-sign-compare -Wshadow

I really don't think we want these changes for the macOS/iOS port.

> Source/ThirdParty/gtest/xcode/Config/ProductionProject.xcconfig:-34
> -WK_QUOTED_OVERRIDE_FRAMEWORKS_DIR_YES = "$(WK_OVERRIDE_FRAMEWORKS_DIR)";

Why is this file going away?

> Source/ThirdParty/gtest/xcode/Config/ReleaseProject.xcconfig:-41
> -SDKROOT_YES = macosx.internal;

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/Config/StaticLibraryTarget.xcconfig:-20
> -OTHER_LDFLAGS = ;

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/Makefile:-2
> -include ../../../../Makefile.shared

I don't think we want this change.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-294
> -		};

Why is this going away?

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-319
> -				1A0A4C4614B7A3BC00895135 /* Frameworks */,

Why is this going away?

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-347
> -		};

Ditto.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:841
> +				GCC_VERSION = com.apple.compilers.llvm.clang.1_0;

GCC_VERSION should be controlled by our xcconfig files. I don't want it set individually!

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:967
> +				SDKROOT = macosx;

We don't want this set in the project file. It should be controlled by the xcconfig file.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-1026
> -		};

Why are you taking away our Production build target?

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:1096
> +			defaultConfigurationName = Release;

Are all of these changes from Production to Release expected? I am a little surprised to see this.

[Ross Kirsling]

Created attachment 348700 [details]
Patch

[Ross Kirsling]

Created attachment 348701 [details]
Patch

[Ross Kirsling]

(In reply to Brent Fulgham from comment #22)
> Comment on attachment 348597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348597&action=review
> 
> Overall this patch looks fine, but I think the Project file changes in
> ThirdParty/gtest are wrong. It seems to lose our Production build target,
> and does not understand that we might build with our Internal SDK versus the
> external developer's SDK. I can't approve the patch in this form.

Sorry, I didn't know the files in that directory were custom. I've now gone through them all manually and believe existing functionality should be preserved.

[Don Olmstead]

There's been a legit release of gtest.

https://github.com/google/googletest/releases/tag/release-1.8.1

[Ross Kirsling]

Created attachment 348740 [details]
Patch

[Ross Kirsling]

Created attachment 348741 [details]
Patch

[Ross Kirsling]

Created attachment 348745 [details]
Patch

[Ross Kirsling]

Man, it sure doesn't like it when I include the runtests.sh mode updates...
Oh well, we don't need them anyway.

[Ross Kirsling]

Note that the WinCairo EWS failure here is a sporadically-occurring fluke (bug 187725).

Stepping forward from the earlier hash to the 1.8.1 release was a relatively trivial update to the patch here.

[Brent Fulgham]

Comment on attachment 348745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348745&action=review

This patch looks like it has correct the project file issues I was concerned about. I think this looks fine.

> Source/ThirdParty/gtest/README.WebKit:-16
> -      gtest-md.vcproj upgraded to VS 2005 (8.0) format to match the rest of WebKit

Were these changes rolled into gtest proper?

[Brent Fulgham]

Comment on attachment 348745 [details]
Patch

rs=me under the assumption that the Sony folks will fix the WinCairo build failure.

[Ross Kirsling]

Thanks Brent!

[WebKit Commit Bot]

Comment on attachment 348745 [details]
Patch

Clearing flags on attachment: 348745

Committed r235613: <https://trac.webkit.org/changeset/235613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44100989>