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.
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.
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.
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.
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.
(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.
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?
(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.
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.
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.
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.
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.