Allow build script to detect more recent Windows 10 SDK versions #7217
Conversation
PetSerAl
requested review from
TravisEz13,
adityapatwardhan,
anmenaga and
daxian-dbw
as code owners
| # It is only present if the SDK is installed. | ||
| return (Test-Path "${env:ProgramFiles(x86)}\Windows Kits\10\bin\x64\mc.exe") | ||
| return (Test-Path "${env:ProgramFiles(x86)}\Windows Kits\10\bin\x64\mc.exe") -or | ||
| (Test-Path "${env:ProgramFiles(x86)}\Windows Kits\10\bin\10.0.*.0\x64\mc.exe") |
There was a problem hiding this comment.
I see 5 folder on my computer and I don't know path default...
Why we test full path here and use env path search in line 236?
Maybe test Get-Command -Application mc.exe?
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry the line I meant
https://github.com/PetSerAl/PowerShell/blob/8bee005699178008ca78efa306b4d196c53c0a54/build.psm1#L266
On my computer the exe is in PATH
where.exe mc.exe
C:\Program Files (x86)\Microsoft SDKs\Windows\v7.1A\Bin\MC.Exe
Perhaps new installer doesn't add this in PATH.
There was a problem hiding this comment.
For me it is not in PATH, unless you call vcvarsall.bat in initialize developer environment:
C:\>where.exe mc.exe
INFO: Could not find files for the given pattern(s).
C:\>"\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Auxiliary\Build\vcvarsall.bat" x64
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.0
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'
C:\>where.exe mc.exe
C:\Program Files (x86)\Windows Kits\10\bin\10.0.17134.0\x64\mc.exeThere was a problem hiding this comment.
Therefore, we can not test the existence of mc.exe before launching vcvarsall.bat, is it?
There was a problem hiding this comment.
As far as I understand, yes. But I am not the one who implement original check, so I can not state exact reasons, why it was implemented this way.
|
This is only needed for native code? @adityapatwardhan should this be moved to the PowerShell-Native repo? |
|
Yes the changes should be made in the https://github.com/PowerShell/PowerShell-native. Please open a PR over there. Thanks! |
|
@adityapatwardhan I already merged #7218. Just want you to know in case any changes to native build script should happen in the other repo. |
|
@adityapatwardhan I think it's probably better to keep the native-build script up-to-date in the powershell repo until we completely remove it. |
|
What is the cut-off date? |
|
@iSazonov I don't know a hard cut-off day, but I guess we want to get it done before GA. |
|
I will submit a PR in the powershell-native repo for this change and merge and then merge this PR, so that both side are in sync regarding this change. |
|
@PetSerAl Thank you for submitting this PR. The current code is PowerShell/PowerShell-Native repo already has this fix. Closing this PR. |
5 participants
PR Summary
PowerShell native windows binaries can be compiled using recent Window 10 SDK versions (for example 10.0.17134.0), but build script
build.psm1does not detect them installed. This PR allow build script to detect them.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests