[webkit-reviews] review denied: [Bug 175722] Update googletest : [Attachment 348597] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 175722: Update googletest
https://bugs.webkit.org/show_bug.cgi?id=175722

Attachment 348597: Patch

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




--- Comment #22 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 348597
  --> https://bugs.webkit.org/attachment.cgi?id=348597
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.