Bug 191713 – [CMake][AppleWin] Use DEBUG_SUFFIX only where needed

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CMake

[Don Olmstead]

MSVC builds executable code into the same directory structure regardless of configuration. For full debug builds of internal AppleWin builds DEBUG_SUFFIX is set to separate release and debug build artifacts. CMake files were modified to support this but it appears that ${DEBUG_SUFFIX} is used in places where it isn't needed.

# Correct usage we're just setting the output name
set(WTF_OUTPUT_NAME WTF${DEBUG_SUFFIX})
# Correct usage this library is built outside of the WebKit tree
target_link_libraries(WebKitQuartzCoreAdditions CoreFoundation${DEBUG_SUFFIX})

# Incorrect usage since CMake builds the library and will know the output name
set(test_wtf_LIBRARIES WTF${DEBUG_SUFFIX})

All incorrect usages should be removed assuming CMake is functioning as I suspect it will.

[Don Olmstead]

Created attachment 354987 [details]
WIP Patch

This fixes all incorrect usages of ${DEBUG_SUFFIX} within CMake files.

To test the build I ran the following command to pretend I had an internal AppleWin build.

perl Tools\Scripts\build-webkit --wincairo --64-bit --debug --cmakeargs="-DDEBUG_SUFFIX=_debug"

I had to do some build fixes so I suspect that the internal Apple Windows build might be having issues but I can't verify since I don't have access.

Also the adding of ${DEBUG_SUFFIX} is not consistent. There are a number of places where produced executables and libraries do not have a debug suffix. All the WEBKIT_FRAMEWORKS have _debug.lib, e.g. WTF_debug.lib/dll. Some executables have them, e.g. jsc_debug.exe, but not others, MiniBrowser.exe.

Anyways this builds now so we can go from here in terms of fixing it all.

[EWS Watchlist]

Attachment 354987 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/PlatformWin.cmake:450:  Alphabetical sorting problem. "PRIVATE WebKitLegacyGUID" should be before "PRIVATE Winmm".  [list/order] [5]
ERROR: Source/WebKitLegacy/CMakeLists.txt:33:  Alphabetical sorting problem. "PRIVATE PAL" should be before "PRIVATE WebCore".  [list/order] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 354987 [details]
WIP Patch

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

Made some comments so the changes make sense. ${DEBUG_SUFFIX} is just removed from anything that will end up in a linked library.

Just turning on the review flag to see what might be required next.

> Source/ThirdParty/ANGLE/PlatformWin.cmake:38
> +if (DEBUG_SUFFIX)
> +    list(APPEND ANGLEGLESv2_LIBRARIES dxguid)
> +endif ()

If there's no DEBUG_SUFFIX _DEBUG is removed from the compiler flags. Compiling with DEBUG_SUFFIX revealed that dxguid is needed when compiling ANGLE due to missing symbols that are used when _DEBUG is present. This is why I suspect that the build might not actually be working.

> Source/WebKitLegacy/PlatformWin.cmake:450
> -    PRIVATE WebKitGUID${DEBUG_SUFFIX}
> +    PRIVATE WebKitLegacyGUID

The original code might actually be a bug. I know we've had issues with IDL not regenerating without a clean build.

> Source/WebKitLegacy/PlatformWin.cmake:458
> +        libANGLE
> +        libEGL
> +        libGLESv2

ANGLE stuff does not actually contain a suffix. Its including a .exp file which doesn't match the library name if I set things.

[Brent Fulgham]

I'm not sure if we have this totally right. I believe we need to support the following states:

(1) Production build, full optimizations, LTO, stripping, etc.
(2) Local release build, not linked against any debug stuff.
(3) Local debug build, built with debug symbols and -0 optimization, but linked against release libraries (e.g., CRT, CoreGraphics, etc.)
(4) Debug_Suffix debug build, build with debug symbols and linked against debug libraries.

We only append the "_debug" suffix to WebKit binaries that are built for (4).

I'm not sure the current patch is totally right, because I think it will give us "debug suffix" builds without the suffix on the WebKit bits.

[Don Olmstead]

(In reply to Brent Fulgham from comment #4)
> I'm not sure if we have this totally right. I believe we need to support the
> following states:
> 
> (1) Production build, full optimizations, LTO, stripping, etc.
> (2) Local release build, not linked against any debug stuff.
> (3) Local debug build, built with debug symbols and -0 optimization, but
> linked against release libraries (e.g., CRT, CoreGraphics, etc.)
> (4) Debug_Suffix debug build, build with debug symbols and linked against
> debug libraries.
> 
> We only append the "_debug" suffix to WebKit binaries that are built for (4).
> 
> I'm not sure the current patch is totally right, because I think it will
> give us "debug suffix" builds without the suffix on the WebKit bits.

$ find WebKitBuild/Debug/lib64 -name *.lib
WebKitBuild/Debug/lib64/DumpRenderTreeLib.lib
WebKitBuild/Debug/lib64/gtest.lib
WebKitBuild/Debug/lib64/ImageDiffLib.lib
WebKitBuild/Debug/lib64/JavaScriptCore_debug.lib
WebKitBuild/Debug/lib64/jscLib.lib
WebKitBuild/Debug/lib64/libANGLE.lib
WebKitBuild/Debug/lib64/libEGL.lib
WebKitBuild/Debug/lib64/libGLESv2.lib
WebKitBuild/Debug/lib64/MiniBrowserLib.lib
WebKitBuild/Debug/lib64/PAL_debug.lib
WebKitBuild/Debug/lib64/testapiLib.lib
WebKitBuild/Debug/lib64/testmasmLib.lib
WebKitBuild/Debug/lib64/testRegExpLib.lib
WebKitBuild/Debug/lib64/TestRunnerInjectedBundle.lib
WebKitBuild/Debug/lib64/TestWebCoreLib.lib
WebKitBuild/Debug/lib64/TestWebKitAPIBase.lib
WebKitBuild/Debug/lib64/TestWebKitAPIInjectedBundle.lib
WebKitBuild/Debug/lib64/TestWebKitLegacyLib.lib
WebKitBuild/Debug/lib64/TestWebKitLib.lib
WebKitBuild/Debug/lib64/TestWTFLib.lib
WebKitBuild/Debug/lib64/WebCoreTestSupport.lib
WebKitBuild/Debug/lib64/WebCore_debug.lib
WebKitBuild/Debug/lib64/WebKit2.lib
WebKitBuild/Debug/lib64/WebKitGUID_debug.lib
WebKitBuild/Debug/lib64/WebKitTestRunnerLib.lib
WebKitBuild/Debug/lib64/WebKit_debug.lib
WebKitBuild/Debug/lib64/WTF_debug.lib

As you can see WTF, JavaScriptCore, WebCore, PAL, WebKit (WebKitLegacy technically) all have the _debug suffix in there.

Its just the things around it like ANGLE and the testing libraries don't have a suffix.

[Alex Christensen]

I don't think we can just remove these like you have without messing up our internal build.  What is the motivation for wanting to do this?

[Don Olmstead]

(In reply to Alex Christensen from comment #6)
> I don't think we can just remove these like you have without messing up our
> internal build.  What is the motivation for wanting to do this?

It was brought up in my patch for https://bugs.webkit.org/show_bug.cgi?id=191662 and I wanted to test my theory on this.

I tried it with setting the DEBUG_SUFFIX manually on WinCairo so I think this is correct. AppleWin build is currently red in trunk so that's why its not coming back.

Could someone in Apple give this patch a go on a build? I'm pretty confident that it will work for the reasons stated above.

[Brent Fulgham]

Comment on attachment 354987 [details]
WIP Patch

I actually think this looks good, but we need to confirm that our weird DebugSuffix build still does what we expect. We'll run a test today and confirm.

[Per Arne Vollan]

I am seeing some link errors when applying this patch. It seems JavaScript_Debug is trying to link with WTF.lib, not WTF_debug.lib.

[Don Olmstead]

CMake is invoked in each individual .vcxproj so this won't work.

It makes the case for using CMake's external project but that's significantly more involved.

[Alex Christensen]

Comment on attachment 354987 [details]
WIP Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.