Bug 190797 – [Win] Allow passing extra vswhere arguments to build-webkit.

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Basuke Suzuki]

With this patch, it is possible to pass extra arguments to vswhere which is used internally to find the location of MS VisualStudio.

[Basuke Suzuki]

Created attachment 352897 [details]
PATCH

[Basuke Suzuki]

Created attachment 352898 [details]
PATCH

Example:
# to use version greater or equeal than 15.7 but lesser than 15.8
> perl build-webkit --wincairo --vswhereargs='-version [15.7,15.8)'

# to use VisualStudio Community over VisualStudio Professional
> perl build-webkit --wincairo --vswhereargs='-products Microsoft.VisualStudio.Product.Community'

[Basuke Suzuki]

Created attachment 352901 [details]
PATCH

[Ross Kirsling]

Comment on attachment 352901 [details]
PATCH

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

Seems fine once builds pass. :)

> Tools/ChangeLog:12
> +        # to use version greater or equeal than 15.7 but lesser than 15.8

Nit: "greater than or equal to 15.7 but less than 15.8"

[Basuke Suzuki]

> > Tools/ChangeLog:12
> > +        # to use version greater or equeal than 15.7 but lesser than 15.8
> 
> Nit: "greater than or equal to 15.7 but less than 15.8"

Ouch. Yes, that's right. GTE. Real programmer doesn't mistake this English. Shame on me. I'll fix it after seeing the result of build.

[Don Olmstead]

Is there a reason why this is needed? https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I don't think we should be figuring out semantic versioning because VC had problems. We should always be able to build with VC and I think the VC++ now has us in their test suite so hopefully this shouldn't happen again.

[Basuke Suzuki]

(In reply to Don Olmstead from comment #6)
> Is there a reason why this is needed?
> https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I
> don't think we should be figuring out semantic versioning because VC had
> problems. We should always be able to build with VC and I think the VC++ now
> has us in their test suite so hopefully this shouldn't happen again.

Well this shouldn't but "History repeats itself". VC changes its implementation on 15.8 and that causes https://bugs.webkit.org/show_bug.cgi?id=189987 .

Also it's handy for us to have a way to test preview installation. I don't see any downside of this patch.

[Basuke Suzuki]

Created attachment 352905 [details]
FIX example sentence in ChangeLog

[Don Olmstead]

(In reply to Basuke Suzuki from comment #7)
> (In reply to Don Olmstead from comment #6)
> > Is there a reason why this is needed?
> > https://bugs.webkit.org/show_bug.cgi?id=189987 should be resolved now. I
> > don't think we should be figuring out semantic versioning because VC had
> > problems. We should always be able to build with VC and I think the VC++ now
> > has us in their test suite so hopefully this shouldn't happen again.
> 
> Well this shouldn't but "History repeats itself". VC changes its
> implementation on 15.8 and that causes
> https://bugs.webkit.org/show_bug.cgi?id=189987 .
> 
> Also it's handy for us to have a way to test preview installation. I don't
> see any downside of this patch.

My only complaint is that vswhere is used internally so exposing things doesn't seem to be something people would do on a regular basis. I can see the utility of this but it seems like a pretty small use case.

I don't see any problems with the contents of the patch.

[Basuke Suzuki]

Don, is wincairo bot fails actually because of this patch?

[Ross Kirsling]

(In reply to Basuke Suzuki from comment #10)
> Don, is wincairo bot fails actually because of this patch?

No, it's just the MIDL issue again (bug 187725).

[Fujii Hironori]

AppleWin port doesn't use vswhere.

  Bug 189412 – download-github-release.py hits errors on Windows bots

WinCairo BuildBot and EWS bots explicitly set env vars by using Select-VSEnvironment outside of build-webkit.

  https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/ews/windowsservercore/WebKit-EWS/Run-EWS.ps1#L33
  https://github.com/WebKitForWindows/docker-webkit-dev/blob/master/buildbot/windowsservercore/WebKit-BuildWorker/Run-BuildbotWorker.ps1#L54

build-webkit prefers VSINSTALLDIR env var to vswhere.

   https://github.com/WebKit/webkit/blob/19cf5c15f58dcbe9843f2ffa2ede30a96bc97bbd/Tools/Scripts/webkitdirs.pm#L696

I think vswhere should be removed from build-webkit.

[Fujii Hironori]

FYI,

https://github.com/Microsoft/vswhere

> vswhere is included with the installer as of Visual Studio 2017 version 15.2 and later, and can
> be found at the following location: %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe.

So, I'm using it like following:

> for /F "delims=" %%I in ('"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" -latest -property installationPath') do set VSPATH=%%I
> call "%VSPATH%\VC\Auxiliary\Build\vcvars64.bat"

[Konstantin Tokarev]

Comment on attachment 352905 [details]
FIX example sentence in ChangeLog

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

> Tools/Scripts/webkitdirs.pm:671
> +    undef $@;

If we want to use eval{}, it may be a good idea to import Try::Tiny module into the tree and use it instead, its usage is much more clear and less error prone

> Tools/Scripts/webkitdirs.pm:673
> +    return undef unless (scalar @$installations);

I think following code would be easier to read:

unless (@$installations) {
    return;
}

Though you may want to omit it completely, because sorting empty array requires no work

> Tools/Scripts/webkitdirs.pm:683
> +    return $vsWhereFoundInstallation;

return $sorted[0]

What you really want is to return first element of array, not to modify that array for possible future use (what pop does)

> Tools/Scripts/webkitdirs.pm:686
> +sub visualStudioProductIdPriority($)

See below about prototypes

> Tools/Scripts/webkitdirs.pm:1972
> +sub buildVisualStudioProject($;$;$)

In modern perl community usage of prototypes is considered harmful, unless it is done to imitate psecific behavior of builtin functions, or to implement embedded DSL. See e.g. http://www.modernperlbooks.com/mt/2009/08/the-problem-with-prototypes.html

Also, correct spelling of this one is ($;$$)

[Basuke Suzuki]

Comment on attachment 352905 [details]
FIX example sentence in ChangeLog

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

Thanks for review!

>> Tools/Scripts/webkitdirs.pm:671
>> +    undef $@;
> 
> If we want to use eval{}, it may be a good idea to import Try::Tiny module into the tree and use it instead, its usage is much more clear and less error prone

Okay, I'll try. How can I import perl module into tree?

>> Tools/Scripts/webkitdirs.pm:673
>> +    return undef unless (scalar @$installations);
> 
> I think following code would be easier to read:
> 
> unless (@$installations) {
>     return;
> }
> 
> Though you may want to omit it completely, because sorting empty array requires no work

Okay to remove this at this. And I learned accessing out of indexed value is undef. Fancy.

>> Tools/Scripts/webkitdirs.pm:683
>> +    return $vsWhereFoundInstallation;
> 
> return $sorted[0]
> 
> What you really want is to return first element of array, not to modify that array for possible future use (what pop does)

Okay. Right.

>> Tools/Scripts/webkitdirs.pm:1972
>> +sub buildVisualStudioProject($;$;$)
> 
> In modern perl community usage of prototypes is considered harmful, unless it is done to imitate psecific behavior of builtin functions, or to implement embedded DSL. See e.g. http://www.modernperlbooks.com/mt/2009/08/the-problem-with-prototypes.html
> 
> Also, correct spelling of this one is ($;$$)

Got it. I will remove that.

[Fujii Hironori]

Respond to my comment. Why do you need this change?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #16)
> Respond to my comment. Why do you need this change?

See my previous comment to Don. WinCairo uses vswhere so that it is better way for choosing instance of VisualStudio. If you want to delete that, that should be another discussion.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #17)
> See my previous comment to Don. WinCairo uses vswhere so that it is better
> way for choosing instance of VisualStudio.

This is not right.  WinCairo doesn't necessarily use vswhere. (Comment 12)
It use VSINSTALLDIR if VSINSTALLDIR is set. Otherwise, it uses vswhere.

You can simply use vcvars64.bat. What is the difference using command line switch and using vcvars64.bat?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #18)
> (In reply to Basuke Suzuki from comment #17)
> > See my previous comment to Don. WinCairo uses vswhere so that it is better
> > way for choosing instance of VisualStudio.
> 
> This is not right.  WinCairo doesn't necessarily use vswhere. (Comment 12)
> It use VSINSTALLDIR if VSINSTALLDIR is set. Otherwise, it uses vswhere.

So that WinCairo uses vswhere if env var is not set. And that means vswhere is the default choice because that var isn't defined by default.

But is this conversation meaningful? There's a feature which somebody uses and this patch improve the value of that usage.

If you think the feature is harmful, open a bug for that and let's discuss about that. This bug is not the right place for removing vswhere. I don't think I have to probe my way is better than your way on this bug.

[Fujii Hironori]

I think removing vswhere is the simplest solution for Bug 189412. OK, I will do it in Bug 189412.

[Fujii Hironori]

Did you test your patch? It doesn't work as you expect.

I installed two instances:

> installationVersion: 15.7.27703.2047
> productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\Common7\IDE\devenv.exe
> displayName: Visual Studio Professional 2017

> installationVersion: 15.8.28010.2048
> productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\devenv.exe
> displayName: Visual Studio Community 2017

vswhere ouputs as expected.

> PS C:\webkit\ga> vswhere -property installationPath -version "[15.7,15.8)"
> C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional

Here is the build log:

> PS C:\webkit\ga> perl Tools\Scripts\build-webkit --release --wincairo --generate-project-only --no-ninja --vswhereargs='-version [15.7,15.8)'
> Updating WinCairoRequirements.zip...
> Found existing release: v2018.10.09
> Seeking latest release from WebKitForWindows/WinCairoRequirements...
> Found release to download: v2018.10.09
> Already up-to-date!
> Updating vswhere.exe...
> Found existing release: 2.5.2
> Seeking latest release from Microsoft/vswhere...
> Found release to download: 2.5.2
> Already up-to-date!
> +  cmake -DPORT="WinCairo" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DDEVELOPER_MODE=ON "C:/webkit/ga"
> -- The C compiler identification is MSVC 19.15.26732.1
> -- The CXX compiler identification is MSVC 19.15.26732.1
> -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe
> -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe -- works

It picks the compiler of Community.

webkitdirs.pm is using vswhere to find msbuild, but it is used only for building generated solution file.
CMake looks for VS installation by its own logic.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #21)
> Did you test your patch? It doesn't work as you expect.
> 
> I installed two instances:
> 
> > installationVersion: 15.7.27703.2047
> > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\Common7\IDE\devenv.exe
> > displayName: Visual Studio Professional 2017
> 
> > installationVersion: 15.8.28010.2048
> > productPath: C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\devenv.exe
> > displayName: Visual Studio Community 2017
> 
> vswhere ouputs as expected.
> 
> > PS C:\webkit\ga> vswhere -property installationPath -version "[15.7,15.8)"
> > C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional
> 
> Here is the build log:
> 
> > PS C:\webkit\ga> perl Tools\Scripts\build-webkit --release --wincairo --generate-project-only --no-ninja --vswhereargs='-version [15.7,15.8)'
> > Updating WinCairoRequirements.zip...
> > Found existing release: v2018.10.09
> > Seeking latest release from WebKitForWindows/WinCairoRequirements...
> > Found release to download: v2018.10.09
> > Already up-to-date!
> > Updating vswhere.exe...
> > Found existing release: 2.5.2
> > Seeking latest release from Microsoft/vswhere...
> > Found release to download: 2.5.2
> > Already up-to-date!
> > +  cmake -DPORT="WinCairo" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DDEVELOPER_MODE=ON "C:/webkit/ga"
> > -- The C compiler identification is MSVC 19.15.26732.1
> > -- The CXX compiler identification is MSVC 19.15.26732.1
> > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe
> > -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.15.26726/bin/Hostx64/x64/cl.exe -- works
> 
> It picks the compiler of Community.
> 
> webkitdirs.pm is using vswhere to find msbuild, but it is used only for
> building generated solution file.
> CMake looks for VS installation by its own logic.

That is the expected behaviour. Vswhere is for choosing msbuild. That’s the result I wanted.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #22)
> (In reply to Fujii Hironori from comment #21)
> > webkitdirs.pm is using vswhere to find msbuild, but it is used only for
> > building generated solution file.
> > CMake looks for VS installation by its own logic.
> 
> That is the expected behaviour. Vswhere is for choosing msbuild. That’s the
> result I wanted.

I'm confusing. 
Why do you want to choose msbuild? You don't need to choose compiler? What problem does your patch solve?

[Fujii Hironori]

I got the answer to my question.
Different compilers are used in CMake configuration time (chosen by CMake) and building time (chosen by vswhere).

[Fujii Hironori]

Using vswhere causes the compiler version mismatch problem (Bug 190797 Comment 24).
CMake looks for VS by its own logic.

We should use vcvars64.bat to safely choose a VS installation.
If you have multiple VS installations, there are mulptile
"x64 Native Tools Command Prompt for Visual Studio 2017" menu items in Windows Start menu.
You can use which you want to choose VS.

[Alex Christensen]

Comment on attachment 352905 [details]
FIX example sentence in ChangeLog

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