Bug 197249

Summary: [Win] Add flag to enable version information stamping and disable by default.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: PlatformAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, Basuke.Suzuki, bfulgham, commit-queue, darin, don.olmstead, Hironori.Fujii, jhoneycutt, pvollan, ross.kirsling, stephan.szabo, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
PATCH
none
PATCH
ross.kirsling: review+
PATCH
none
PATCH
none
PATCH
none
PATCH none

Description Basuke Suzuki 2019-04-24 15:07:41 PDT
There's no need to add version information while developing.
Comment 1 Basuke Suzuki 2019-04-24 15:11:12 PDT
Created attachment 368186 [details]
PATCH
Comment 2 Brent Fulgham 2019-04-24 15:16:52 PDT
What constitutes DEVELOPER_MODE? I don't think it's used in Apple ports.
Comment 3 Basuke Suzuki 2019-04-24 15:35:30 PDT
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.
Comment 4 Ross Kirsling 2019-04-24 16:47:45 PDT
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?
Comment 5 Stephan Szabo 2019-04-24 17:09:36 PDT
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.
Comment 6 Basuke Suzuki 2019-04-25 11:21:48 PDT
How about defining USE_VERSION_STAMPER and default to YES on AppleWin and NO on other ports?
Comment 7 Ross Kirsling 2019-04-25 11:25:19 PDT
(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
Comment 8 Basuke Suzuki 2019-04-25 12:06:30 PDT
Created attachment 368256 [details]
PATCH

Define new flag `USE_VERSION_STAMPER`.
Comment 9 Ross Kirsling 2019-04-25 12:50:00 PDT
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?
Comment 10 Basuke Suzuki 2019-04-25 13:00:41 PDT
Make sense. I'll try another patch.
Comment 11 Basuke Suzuki 2019-04-25 15:29:38 PDT
Created attachment 368278 [details]
PATCH
Comment 12 WebKit Commit Bot 2019-04-25 16:08:14 PDT
Comment on attachment 368278 [details]
PATCH

Clearing flags on attachment: 368278

Committed r244669: <https://trac.webkit.org/changeset/244669>
Comment 13 WebKit Commit Bot 2019-04-25 16:08:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-04-25 16:20:07 PDT
<rdar://problem/50224412>
Comment 15 Basuke Suzuki 2019-04-26 16:49:01 PDT
It was reverted. https://trac.webkit.org/changeset/244676/webkit
Comment 16 Basuke Suzuki 2019-04-26 16:49:09 PDT
https://trac.webkit.org/changeset/244676/webkit
Comment 17 Basuke Suzuki 2019-04-26 16:49:28 PDT
Created attachment 368363 [details]
PATCH
Comment 18 Basuke Suzuki 2019-04-26 16:50:28 PDT
Created attachment 368364 [details]
PATCH
Comment 19 Ross Kirsling 2019-04-28 18:55:59 PDT
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.
Comment 20 Basuke Suzuki 2019-04-29 10:32:36 PDT
Created attachment 368466 [details]
PATCH
Comment 21 Basuke Suzuki 2019-04-29 10:32:56 PDT
(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.
Comment 22 Basuke Suzuki 2019-04-29 10:47:09 PDT
(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 23 WebKit Commit Bot 2019-04-29 11:18:59 PDT
Comment on attachment 368466 [details]
PATCH

Clearing flags on attachment: 368466

Committed r244741: <https://trac.webkit.org/changeset/244741>
Comment 24 WebKit Commit Bot 2019-04-29 11:19:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Konstantin Tokarev 2019-04-29 11:37:42 PDT
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.