GeezerVison

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

GeezerVison is showing github.com — Revert refactoring changes that broke remoting to Windows PowerShell 5.1 by SteveL-MSFT · Pull Request #5321 · PowerShell/PowerShell

Conversation

Steve Lee (SteveL-MSFT) commented Nov 3, 2017

Copy link

Copy Markdown

Member

Call backs depend on private members to have the same name across versions of PowerShell. Specifically fixes prompts and clear.

Paul Higinbotham (@PaulHigin) are there other callbacks I need to fix? I noticed that ctrl+c doesn't work from PSCore6 to Windows PowerShell 5.1.

Fix #4615

Sorry, something went wrong.

Uh oh!

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

All reactions


          revert refactoring changes so that it doesn't break remoting to Windo…

09bb265

…ws PowerShell 5.1

Steve Lee (SteveL-MSFT) assigned Travis Plunk (TravisEz13)

Nov 3, 2017

Steve Lee (SteveL-MSFT) requested a review from Paul Higinbotham (PaulHigin)

November 3, 2017 03:32

Steve Lee (SteveL-MSFT) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw) and Jason Shirk (lzybkr) as code owners

November 3, 2017 03:32

Jason Shirk (lzybkr) approved these changes

Nov 3, 2017

View reviewed changes

Jason Shirk (lzybkr) left a comment

Copy link

Copy Markdown

Contributor

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.

Sigh - it's silly that the protocol requires the names of the private fields. I suppose it's just an expedient implementation detail - just use reflection, but it's so brittle.

Sorry, something went wrong.

Uh oh!

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

All reactions

Steve Lee (SteveL-MSFT) commented Nov 3, 2017

Copy link

Copy Markdown

Member Author

Jason Shirk (@lzybkr) I agree now that I've learned how this aspect of the protocol works

PaulHigin 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.

Paul Higinbotham (PaulHigin) suggested changes

Nov 3, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/engine/hostifaces/ChoiceDescription.cs Outdated

		public sealed
		class ChoiceDescription
		{
		// DO NOT REMOVE OR RENAME THESE FIELDS - it will break remoting

Paul Higinbotham (PaulHigin) Nov 3, 2017

Copy link

Copy Markdown

Contributor

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 think the warning messages should say "it will break remoting compatibility with Windows PowerShell".

Sorry, something went wrong.

Uh oh!

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

All reactions

Paul Higinbotham (PaulHigin) Nov 3, 2017

Copy link

Copy Markdown

Contributor

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 incomplete. To fully fix this incompatibility we need to revert field names for all encoded class/structs. Here is the supported list:
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHostEncoder.cs#L41

Sorry, something went wrong.

Uh oh!

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

All reactions

Steve Lee (SteveL-MSFT) Nov 3, 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.

I reused an existing comment. I can update all occurrences of that comment to be more specific. Thanks for the pointer to the list of types, I'll fix those as well.

Sorry, something went wrong.

Uh oh!

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

All reactions

Steve Lee (SteveL-MSFT) requested review from Dan Travison (dantraMSFT) and Mike Richmond (mirichmo) as code owners

November 3, 2017 23:17

Travis Plunk (TravisEz13) commented Nov 4, 2017

Copy link

Copy Markdown

Member

Steve Lee (@SteveL-MSFT) One of the commits is not linked to your github account.

All reactions

Sorry, something went wrong.

Uh oh!

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


          fixed the rest of the supported types for remoting

0c14e56

Steve Lee (SteveL-MSFT) force-pushed the remoting-callback branch from 930e4ab to 0c14e56 Compare

November 6, 2017 03:35

Steve Lee (SteveL-MSFT) commented Nov 6, 2017

Copy link

Copy Markdown

Member Author

Travis Plunk (@TravisEz13) fixed

All reactions

Sorry, something went wrong.

Uh oh!

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

Mike Richmond (mirichmo) reviewed

Nov 6, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/engine/hostifaces/ChoiceDescription.cs Outdated

		public sealed
		class ChoiceDescription
		{
		// DO NOT REMOVE OR RENAME THESE FIELDS - it will break remoting compatibility with Windows PowerShell compatibility with Windows PowerShell

Mike Richmond (mirichmo) Nov 6, 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.

What about putting these fields within a region to make the grouping more explicit? The "Do Not Remove or Rename" region?

Sorry, something went wrong.

Uh oh!

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

All reactions

Steve Lee (SteveL-MSFT) Nov 6, 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.

Good idea. Will make an update.

Sorry, something went wrong.

Uh oh!

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

All reactions


          put remoting sensitive private members in descriptive region

5a2068e

Paul Higinbotham (PaulHigin) approved these changes

Nov 6, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) added this to the 6.0.0-RC milestone

Nov 7, 2017

Steve Lee (SteveL-MSFT) commented Nov 7, 2017

Copy link

Copy Markdown

Member Author

Travis Plunk (@TravisEz13) can you merge?

All reactions

Sorry, something went wrong.

Uh oh!

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

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

Nov 9, 2017

Steve Lee (SteveL-MSFT) deleted the remoting-callback branch

November 12, 2017 05:17

Steve Lee (SteveL-MSFT) mentioned this pull request

Nov 14, 2017

Cannot delete a mail contact from Office 365 in v6.0.0-beta.5 #5381

Closed

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

Aug 6, 2026


          Revert refactoring changes that broke remoting to Windows PowerShell …

744291f

…5.1 (PowerShell#5321)

- Revert refactoring changes so that it doesn't break remoting to Windows PowerShell 5.1
- Fix the rest of the supported types for remoting
- Put remoting sensitive private members in a descriptive region

Reviewers

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

daxian-dbw is a code owner

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

3 more reviewers

Mike Richmond (mirichmo)

mirichmo left review comments

Jason Shirk (lzybkr)

lzybkr approved these changes

Paul Higinbotham (PaulHigin)

PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

6.0.0-RC

Development

Successfully merging this pull request may close these issues.

Remoting host callbacks incompatible between PowerShell 5.1 and 6.0

Uh oh!

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

6 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.

GeezerVison

Conversation

Steve Lee (SteveL-MSFT) commented Nov 3, 2017

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 3, 2017

Uh oh!

Paul Higinbotham (PaulHigin) Nov 3, 2017

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) Nov 3, 2017

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Nov 3, 2017

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented Nov 4, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 6, 2017

Uh oh!

Mike Richmond (mirichmo) Nov 6, 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!

Steve Lee (SteveL-MSFT) Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 7, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mike Richmond (mirichmo) Nov 6, 2017 •

edited

Loading

Uh oh!

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