Bug 197249 – [Win] Add flag to enable version information stamping and disable by default.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

There's no need to add version information while developing.

[Basuke Suzuki]

Created attachment 368186 [details]
PATCH

[Brent Fulgham]

What constitutes DEVELOPER_MODE? I don't think it's used in Apple ports.

[Basuke Suzuki]

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.

[Ross Kirsling]

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?

[Stephan Szabo]

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.

[Basuke Suzuki]

How about defining USE_VERSION_STAMPER and default to YES on AppleWin and NO on other ports?

[Ross Kirsling]

(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

[Basuke Suzuki]

Created attachment 368256 [details]
PATCH

Define new flag `USE_VERSION_STAMPER`.

[Ross Kirsling]

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?

[Basuke Suzuki]

Make sense. I'll try another patch.

[Basuke Suzuki]

Created attachment 368278 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 368278 [details]
PATCH

Clearing flags on attachment: 368278

Committed r244669: <https://trac.webkit.org/changeset/244669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50224412>

[Basuke Suzuki]

It was reverted. https://trac.webkit.org/changeset/244676/webkit

[Basuke Suzuki]

https://trac.webkit.org/changeset/244676/webkit

[Basuke Suzuki]

Created attachment 368363 [details]
PATCH

[Basuke Suzuki]

Created attachment 368364 [details]
PATCH

[Ross Kirsling]

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.

[Basuke Suzuki]

Created attachment 368466 [details]
PATCH

[Basuke Suzuki]

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

[Basuke Suzuki]

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

[WebKit Commit Bot]

Comment on attachment 368466 [details]
PATCH

Clearing flags on attachment: 368466

Committed r244741: <https://trac.webkit.org/changeset/244741>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Konstantin Tokarev]

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.