-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate Windows PSRP binary build out of Start-PSBuild #4335
Conversation
build.psm1
Outdated
[string]$Arch = 'x64' | ||
) | ||
|
||
if (-not $Environment.IsWindows) { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to return error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think a warning would be better.
build.psm1
Outdated
|
||
# cmake is needed to build powershell.exe | ||
if (-not (precheck 'cmake' $null)) { | ||
throw 'cmake not found. Run "Start-PSBootstrap -Package". You can also install it from https://chocolatey.org/packages/cmake' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not introduce a new -BuildNativeWindows
switch instead of reusing -Package
? That way I can privately build a package w/o needing the SDK or building pwshplugin locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support building Appx package for Windows platform (New-AppxPackage
) and it requires Win10 SDK, that's why I keep using -Package
here. For other package formats, the windows native dependencies are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, but it doesn't appear we've ever published an appx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just talked to @TravisEz13. Appx is for NanoServer, and we never published one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also I believe it never worked quite right.
build.psm1
Outdated
if ($Environment.IsLinux -or $Environment.IsOSX) { | ||
try { | ||
# Update googletest submodule for linux native cmake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a switch to bootstrap the components needed for native components so that we don't add them in systems where they are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TravisEz13 and I talked offline. The -package
parameter is required only for creating Appx package, and before this change -package
only takes effect on Unix platforms. If we don't care about Appx, for example in our release build, we can just run Start-PSBootstrap
for windows.
If we can remove the script about Appx package, then we need to make the following subsequent changes:
- Add switch parameter
-BuildWindowsNative
to install dependencies for building PSRP on Windows. - Install
Wix
when runningStart-PSPackage -Package
on Windows.
@SteveL-MSFT can you please confirm if we can remove New-AppxPackage
? If so, I will take care of (1) in this PR and @TravisEz13 will take care of (2) in his packaging changes.
build.psm1
Outdated
# Install for Windows | ||
if ($Environment.IsWindows) { | ||
# Install Windows dependencies for building native binaries or creating Appx package | ||
if ($Environment.IsWindows -and $Package) { | ||
$machinePath = [Environment]::GetEnvironmentVariable('Path', 'MACHINE') | ||
$newMachineEnvironmentPath = $machinePath | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example line 1221 checks for win 10 sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the biggest component that can reduce the docker image size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joeyaiello are you ok if we remove Appx support? Seems like this is something we can bring back later.
I think it should be OK to remove |
@SteveL-MSFT could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix #3014
There are following major changes:
Start-PSBootstrap -BuildWindowsNative
installs the native dependencies required for building PSRP binary. Without-BuildWindowsNative
, it only installs the dotnet-SDK on Windows platform.Start-PSBuild
doesn't build Windows PSRP binary. Instead,Start-BuildNativeWindowsBinaries
is added to build it. After the build, 3 files ('pwrshplugin.dll'
,'pwrshplugin.pdb'
,'Install-PowerShellRemoting.ps1'
) will be bin-placed atsrc\powershell-win-core
like before.'psrp.windows'
is added topowershell-core
feed, and we reference it inpowershell-win-core.csproj
to get the Windows PSRP related files. The NuGet package is here, and the content of the package is as follows: (.dll and .pdb files are from the beta.4 release build)