build: Enable building for win-arm and win-arm64 #5524
Conversation
|
Waiting on mmi.dll to be published for win-arm and win-arm64 |
|
Confirmed that basic PSCore6 works on win-arm, I think we can merge this. |
|
@adityapatwardhan hold off on merging this, looks like I need to build pwshplugin.dll for arm |
build.psm1
Outdated
There was a problem hiding this comment.
!$env:PROCESSOR_ARCHITECTURE -match "arm"
!$env:PROCESSOR_ARCHITECTURE will be evaluated first in this case.
|
Once the changes to building |
|
@SteveL-MSFT please update here when |
|
Note that PR #5617 would also require |
|
Have some paths hardcoded to validate build. Will update, but requires user to have VS2017 latest installed to build for win-arm. |
fix finding vs2017 dynamically
|
Verified pwrshplugin.dll works on both arm and arm64. @adityapatwardhan can you re-review this PR as I've made substantial changes for the native compilation? |
…er than reg.exe so that error doesn't show on success fix formatting issue in cmake.defs
There was a problem hiding this comment.
LGTM with minor comment.
|
|
||
| # Clean up | ||
| Remove-Item .\$fileName | ||
| New-Item $regKey -Force |
There was a problem hiding this comment.
New-Item $regKey -Force > $null
|
@daxian-dbw Can you have a look too? |
|
@adityapatwardhan Yes, will look tonight or first thing tomorrow morning. |
build.psm1
Outdated
There was a problem hiding this comment.
Question: @SteveL-MSFT Is it going to be required to use VS2017 to build the plugin dll?
There was a problem hiding this comment.
@daxian-dbw The build worker image we use in AppVeyor is Visual Studio 2015. Do we need to update that?
There was a problem hiding this comment.
Actually we are using Visual Studio 2017 through the AppVeyor.yml
There was a problem hiding this comment.
We don't build the native binaries in CI, so it shouldn't matter. Maybe we should switch to an image that doesn't have VS pre-installed.
There was a problem hiding this comment.
@daxian-dbw VS2017 is required to build arm64
There was a problem hiding this comment.
LGTM. Left a question comment.
build.psm1
Outdated
There was a problem hiding this comment.
x64_arm and x64_arm64 seems weird to me. Why are we combining two architectures?
There was a problem hiding this comment.
I'll add a comment. This is the form that the Visual Studio build environment expects for cross compilation. The first is the host and the second the target. VS supports x86_arm64, for example which I'm not supporting here.
address PR feedback
|
@TravisEz13 @daxian-dbw @adityapatwardhan please review the change to the pwrshplugin config reader to address #5286 Decided to do that as part of this PR since I'm publishing a new drop to myget might as well do it once rather than twice. |
There was a problem hiding this comment.
A few non-blocking comments.
| if (*slashIter != L'\\') | ||
| CloseHandle(dirHandle); | ||
| this->pathToPowerShellAssemblies = psHomeDir; | ||
| if (this->pathToPowerShellAssemblies.size() > 0) // Found a match |
There was a problem hiding this comment.
It seems the size() > 0 check should be moved to before the CreateFileW call.
Or maybe the size() > 0 is not needed, because if getValueFromLine returns an empty string, I think CreateFileW will fail.
There was a problem hiding this comment.
Wrote a small program to test this and an empty string does return an invalid handle. I can remove the size check.
| slashIter--; | ||
| if (*slashIter != L'\\') | ||
| CloseHandle(dirHandle); | ||
| this->coreClrDirectory = this->getValueFromLine(line, coreClrDirTag); |
There was a problem hiding this comment.
Why calling this->getValueFromLine(line, coreClrDirTag); again instead of use coreClrDir?
There was a problem hiding this comment.
Mistake, should have been coreClrDir. Will fix.
| if (*slashIter != L'\\') | ||
| CloseHandle(dirHandle); | ||
| this->coreClrDirectory = this->getValueFromLine(line, coreClrDirTag); | ||
| if (this->coreClrDirectory.size() > 0) // Found a match |
There was a problem hiding this comment.
Same comment about the size() check here.
|
@SteveL-MSFT Making those changes means we need to create a new |
…HOME/source address PR feedback
|
@daxian-dbw yeah, I realize that. After this is merged, I'll create one called RC2. |
|
@daxian-dbw @TravisEz13 Any ideas why WIP might be blocking this? |
|
it hasn't run, edit the title and it will run. |
* enable win-arm and win-arm64 builds * fix using arm64 tools for build fix finding vs2017 dynamically * change install-powershellremoting.ps1 script to use reg provider rather than reg.exe so that error doesn't show on success fix formatting issue in cmake.defs * add check that path being read from config file is valid address PR feedback * fix copying of PowerShell.Core.Instrumentation as VS2017 puts it in $HOME/source address PR feedback
* enable win-arm and win-arm64 builds * fix using arm64 tools for build fix finding vs2017 dynamically * change install-powershellremoting.ps1 script to use reg provider rather than reg.exe so that error doesn't show on success fix formatting issue in cmake.defs * add check that path being read from config file is valid address PR feedback * fix copying of PowerShell.Core.Instrumentation as VS2017 puts it in $HOME/source address PR feedback
Requires VS2017 latest preview to build for arm64. Validated win-arm build on Win10 IOT with remoting, but current install script doesn't work correctly on Win10 IOT, need to update.
Also fixed issue with pwrshplugin.dll not validating path before using them.
Fix #5286