GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Separate Windows PSRP binary build out of Start-PSBuild by daxian-dbw · Pull Request #4335 · PowerShell/PowerShell

Conversation

Dongbo Wang (daxian-dbw) commented Jul 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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 at src\powershell-win-core like before.
The NuGet package 'psrp.windows' is added to powershell-core feed, and we reference it in powershell-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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lzybkr reacted with thumbs up emoji

All reactions

👍 1 reaction

Dongbo Wang (daxian-dbw) added 2 commits

July 24, 2017 15:31


          Separate Windows PSRP build out of Start-PSBuild

d9cbc97


          Minor change to build.psm1

620ea80

Dongbo Wang (daxian-dbw) self-assigned this

Jul 24, 2017

Dongbo Wang (daxian-dbw) requested review from Steve Lee (SteveL-MSFT) and Travis Plunk (TravisEz13)

July 24, 2017 22:45

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Jul 24, 2017

Steve Lee (SteveL-MSFT) requested changes

Jul 24, 2017

View reviewed changes

Comment thread

build.psm1 Outdated

		[string]$Arch = 'x64'
		)

		if (-not $Environment.IsWindows) { return }

Steve Lee (SteveL-MSFT) Jul 24, 2017

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Jul 24, 2017

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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'

Steve Lee (SteveL-MSFT) Jul 24, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Jul 24, 2017

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jul 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Jul 25, 2017

Copy link

Copy Markdown

Member Author

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 Travis Plunk (@TravisEz13). Appx is for NanoServer, and we never published one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) Jul 25, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add a warning

3bc790d

Travis Plunk (TravisEz13) reviewed

Jul 25, 2017

View reviewed changes

Comment thread

build.psm1

		# Update googletest submodule for linux native cmake
		if ($Environment.IsLinux -or $Environment.IsOSX) {
		try {
		# Update googletest submodule for linux native cmake

Travis Plunk (TravisEz13) Jul 25, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Jul 25, 2017

Copy link

Copy Markdown

Member Author

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.

Travis Plunk (@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 running Start-PSPackage -Package on Windows.

Steve Lee (@SteveL-MSFT) can you please confirm if we can remove New-AppxPackage? If so, I will take care of (1) in this PR and Travis Plunk (@TravisEz13) will take care of (2) in his packaging changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) reviewed

Jul 25, 2017

View reviewed changes

Comment thread

build.psm1

		if ($Environment.IsWindows -and $Package) {
		$machinePath = [Environment]::GetEnvironmentVariable('Path', 'MACHINE')
		$newMachineEnvironmentPath = $machinePath

Travis Plunk (TravisEz13) Jul 25, 2017

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) Jul 25, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jul 25, 2017

Copy link

Copy Markdown

Member

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.

Joey Aiello (@joeyaiello) are you ok if we remove Appx support? Seems like this is something we can bring back later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) approved these changes

Jul 25, 2017

View reviewed changes


          Add parameter '-BuildWindowsNative' to 'Start-PSBootstrap'

9df7909

Dongbo Wang (daxian-dbw) commented Jul 25, 2017

Copy link

Copy Markdown

Member Author

I think it should be OK to remove New-AppxPackage, so add -BuildWindowsNative to Start-PSBootstrap to indicate installing dependencies for building PSRP. It's more clear to review from CodeFlow.

TravisEz13 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) approved these changes

Jul 25, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) commented Jul 25, 2017

Copy link

Copy Markdown

Member Author

Steve Lee (@SteveL-MSFT) could you please take another look?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) approved these changes

Jul 25, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) merged commit b0efc7c into PowerShell:master

Jul 25, 2017

Dongbo Wang (daxian-dbw) deleted the build branch

July 25, 2017 23:50

Dongbo Wang (daxian-dbw) mentioned this pull request

Jul 27, 2017

Porting Enable-PSRemoting and its Tests #2671

Merged

Dongbo Wang (daxian-dbw) mentioned this pull request

Aug 3, 2017

Add beta.5 changelog #4484

Closed

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Separate Windows PSRP binary build out of Start-PSBuild (PowerShell#4335

99ea816

)

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 at `src\powershell-win-core` like before.
- The NuGet package `'psrp.windows'` is added to `powershell-core` feed, and we reference it in `powershell-win-core.csproj` to get the Windows PSRP related files. Files (.dll and .pdb) in the package are from the beta.4 release build.

Reviewers

Travis Plunk (TravisEz13)

TravisEz13 approved these changes

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Dongbo Wang (daxian-dbw) commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jul 25, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Jul 25, 2017

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dongbo Wang (daxian-dbw) commented Jul 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) Jul 25, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.