There's no need to add version information while developing.
Created attachment 368186 [details] PATCH
What constitutes DEVELOPER_MODE? I don't think it's used in Apple ports.
Oh, that's interesting. I thought this is the standard option for every platform. It is defined in here. https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitdirs.pm#L2203 As you said, AppleWin doesn't use that. What is the best way for our case? We don't have VersionStamper tool. We are basically working on Git repository and not in SVN repository. There's no sense we use this feature.
Given that DEVELOPER_MODE is always set for WinCairo, it appears that this patch is only really differentiating between the Windows ports. If so, then what we really want to check is WTF_PLATFORM_WIN_CAIRO, no?
Well, theoretically, that flag could be turned on for a particular build of AppleWin port if someone put it into cmake flags, but it'd probably cause other problems. It probably makes more sense to do it explicitly based on port if it's desirable for AppleWin builds to fail if they didn't have the stamper tool.
How about defining USE_VERSION_STAMPER and default to YES on AppleWin and NO on other ports?
(In reply to Basuke Suzuki from comment #6) > How about defining USE_VERSION_STAMPER and default to YES on AppleWin and NO > on other ports? SGTM
Created attachment 368256 [details] PATCH Define new flag `USE_VERSION_STAMPER`.
Comment on attachment 368256 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=368256&action=review r=me with one concern. > Source/cmake/WebKitFeatures.cmake:224 > + WEBKIT_OPTION_DEFINE(USE_VERSION_STAMPER "Toggle stamping version information during build" PRIVATE OFF) If this is Windows-specific then I'm not certain whether we should add it here. Note that OptionsGTK.cmake defines a bunch of port-specific options near the top: https://github.com/WebKit/webkit/blob/master/Source/cmake/OptionsGTK.cmake#L72-L87 Perhaps we should do similarly for OptionsWin?
Make sense. I'll try another patch.
Created attachment 368278 [details] PATCH
Comment on attachment 368278 [details] PATCH Clearing flags on attachment: 368278 Committed r244669: <https://trac.webkit.org/changeset/244669>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50224412>
It was reverted. https://trac.webkit.org/changeset/244676/webkit
https://trac.webkit.org/changeset/244676/webkit
Created attachment 368363 [details] PATCH
Created attachment 368364 [details] PATCH
Comment on attachment 368364 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=368364&action=review I'm a bit confused -- I thought the behavioral effect of this patch was simply to remove warning text from the WinCairo build. Do we understand why the clean build started failing for both Windows platforms? > WebKitLibraries/win/tools/scripts/auto-version.pl:86 > + my $SVN_REVISION; Seems that you could initialize to '' here and avoid the else branch below.
Created attachment 368466 [details] PATCH
(In reply to Ross Kirsling from comment #19) > Comment on attachment 368364 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368364&action=review > > I'm a bit confused -- I thought the behavioral effect of this patch was > simply to remove warning text from the WinCairo build. Do we understand why > the clean build started failing for both Windows platforms? > > > WebKitLibraries/win/tools/scripts/auto-version.pl:86 > > + my $SVN_REVISION; > > Seems that you could initialize to '' here and avoid the else branch below. That would be better. Updated.
(In reply to Ross Kirsling from comment #19) > Comment on attachment 368364 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368364&action=review > > I'm a bit confused -- I thought the behavioral effect of this patch was > simply to remove warning text from the WinCairo build. Do we understand why > the clean build started failing for both Windows platforms? There're three version related location in this patch. 1. WTF/wtf/CMakeLists.txt 2. JavaScriptCore/CMakeLists.txt 3. WebKitLegacy/CMakeLists.txt In 1 and 2, there are two custom command invoked for MSVC: a: auto-version.pl to generate version information text file. b: version-stamp.pl to stamp version information to binary. On the other hand, third case, it generate a version information file using auto-version.pl and the file is used in the resource file to be compiled. This caused the previous build error. In the updated patch, I've remove the patch for WebKitLegacy and modify auto-version.pl not to invoke SVN if the directory isn't managed by SVN to avoid warning.
Comment on attachment 368466 [details] PATCH Clearing flags on attachment: 368466 Committed r244741: <https://trac.webkit.org/changeset/244741>
If this feature makes no sense for any port other than AppleWin, it would be better if we moved it into PlatformAppleWin.cmake instead of introducing global option.