Skip compiling the non-supported cmdlets in System.Management.Automation.dll #6939

daxian-dbw · 2018-05-25T20:27:06Z

PR Summary

Fix #6886
Our configuration definition 'Linux' caused our Linux build to be unoptimized, and we didn't realize until recently. This was fixed in #6853, which makes our Linux build adopts the same configuration settings as on Windows -- debug and release.

Start-PSBuild -Clean on Linux starts to use the debug configuration by default, which reveals another issue on Linux build -- we use a cache in InitialSessionState to initialize cmdlets from S.M.A.dll in order to speed up powershell startup and items in cache drifts from actual cmdlet types available in S.M.A.dll in Unix platforms.

This is because there are some cmdlets not supported on Unix, which are if/def'ed out in the cache code, but they are still compiled in S.M.A.dll on Unix. We have some code that only runs in debug configuration to check if the cache is in sync with the real status of S.M.A.dll, but we never got to run in debug configuration because of the 'Linux' configuration we used before.

This PR skips compiling the non-supported cmdlets in S.M.A.dll, to keep the cache in InitialSessionState in sync with the actually available cmdlet types in System.Management.Automation.dll.

Change Details

Note that some of the cmdlet source files contain codes that are used in other places outside the cmdlet. Those codes are moved out to PSRemotingCmdlet.cs, a more general place where base classes and helper classes for remoting command are located. The following type/methods are moved:

Class QueryRunspaces and enum SessionFilterState from ConnectPSSession.cs
Class PSSessionOption and enum ProxyAccessType from NewPSSessionOptionCommand.cs
Methods CheckIfPowerShellVersionIsInstalled and CheckPSVersion from CustomShellCommands.cs to class RemotingCommandUtils in PSRemotingCmdlet.cs

PR Checklist

iSazonov

Leave a comment

iSazonov · 2018-05-26T15:02:36Z

src/System.Management.Automation/engine/remoting/commands/NewPSSessionConfigurationFile.cs

Maybe split the file by classes and exclude a new file from compile on Unix?

If we look CodeFactor issues it recommends to use one-class-per-file - if we move code in the PR maybe follow this rule?

I'm personally in favour of this approach too. I know we've already taken the other path in most places, but there are some good factors:

Keeps *nix and Windows totally separate

Means we can avoid #if altogether

.NET Core does it with X.Unix.cs and X.Windows.cs partial classes: https://github.com/dotnet/corefx/blob/aeda0b030144b478d82e245e39d2a4acb022c501/src/Common/src/System/Net/SocketProtocolSupportPal.Unix.cs

Makes the code a lot more maintainable

Still get things like monomorphism optimisations in the JIT

Once-class-per-file is ridiculous. We have modern editors with all kinds of features for managing large files. I'm much happier navigating within a few files than having to deal with hundreds of tabs. I vote we suppress this rule.

Agree with @BrucePay in the general case of having multiple classes per file. Sometimes it's overkill to make a class nested, but it makes a lot more sense contextually to have it in the same file as a larger class. Enums are a good motivating example I find. Also, it's nice to feel like I've moved on from this historical holdover of Java and MATLAB...

My question was only about the file. The rule was already disabled in PR #6930.

@iSazonov The reason I was using #if !UNIX in this file is because the class SessionConfigurationUtils in this file is heavily used and only used in those two cmdlets (New-PSSessionConfigurationFile and New-PSRoleCapabilityFile), so it makes sense to keep them close to each other in the same file.

iSazonov · 2018-05-26T19:44:03Z

Can we close #1998?

BrucePay

LGTM

rjmholt

Oh no I never submitted my review before!

All looks good to me.

rjmholt · 2018-05-29T19:29:44Z

src/System.Management.Automation/engine/remoting/commands/NewPSSessionConfigurationFile.cs

I'm personally in favour of this approach too. I know we've already taken the other path in most places, but there are some good factors:

Keeps *nix and Windows totally separate

Means we can avoid #if altogether

.NET Core does it with X.Unix.cs and X.Windows.cs partial classes: https://github.com/dotnet/corefx/blob/aeda0b030144b478d82e245e39d2a4acb022c501/src/Common/src/System/Net/SocketProtocolSupportPal.Unix.cs

Makes the code a lot more maintainable

Still get things like monomorphism optimisations in the JIT

iSazonov · 2018-06-02T09:58:58Z

@daxian-dbw Please fix CI Appveyor issues.

daxian-dbw · 2018-06-05T18:01:30Z

@iSazonov Will fix the build today.

This is to keep the cache in InitialSessionState in sycn with the actual available cmdlet types in System.Management.Automation.dll

daxian-dbw · 2018-06-05T21:08:06Z

@iSazonov I had to rebase the branch to build. The new changes are only in the second commit.

PaulHigin · 2018-06-05T22:25:11Z

Class QueryRunspaces and enum SessionFilterState from ConnectPSSession.cs

QueryRunspaces is only used for session disconnect, which is only supported on Windows. PSRemotingCmdlets.cs is a heavily loaded file to support many remoting cmdlet base classes. Moving QueryRunspaces there just makes it more bloated and confusing in my opinion.
I feel that QueryRunspaces should be put into a separate utils file that can be skipped for Linux.
You should also #ifdef out its use in the Get-PSSession parameter set since it is not supported on Linux.

daxian-dbw · 2018-06-05T23:58:30Z

QueryRunspaces is used in Connect-PSSession/Receive-PSSession/Get-PSSession and it's also directly used by the abstract class PSRunspaceCmdlet from PSRemotingCmdlet.cs, that's why I thought it probably fit in the "Helper Classes" region in PSRemotingCmdlet.cs.

I'm happy to move QueryRunspaces and SessionFilterState to a separate file, but we cannot skip compiling that file yet until all uses are properly diabled (if/def'ed out) for Unix platform. That work is out scope for this PR, as this PR is for fixing the crashing debug build on Unix platforms.

I opened the issue #7008 to track the work to properly disable all code related to session connection/disconnection for the Unix build.

rjmholt · 2018-06-06T00:34:23Z

I'll give this a try on my Ubuntu machine later just to check everything

iSazonov · 2018-06-06T03:05:23Z

@rjmholt Congratulations on your first review! :-)

rjmholt · 2018-06-06T16:50:38Z

Tests all pass (except for 4 known problems) on Ubuntu 16.04 in Debug configuration. No assertion failures

daxian-dbw · 2018-06-09T00:24:48Z

@rjmholt Thank you!

daxian-dbw requested review from BrucePay and rjmholt May 25, 2018 20:27

daxian-dbw requested review from PaulHigin, TravisEz13, adityapatwardhan, dantraMSFT and mirichmo as code owners May 25, 2018 20:27

daxian-dbw force-pushed the fixdebug branch from 19df2d2 to 69969fd Compare May 25, 2018 22:20

iSazonov reviewed May 26, 2018

View reviewed changes

BrucePay approved these changes Jun 1, 2018

View reviewed changes

rjmholt approved these changes Jun 1, 2018

View reviewed changes

iSazonov self-assigned this Jun 2, 2018

daxian-dbw added 2 commits June 5, 2018 12:59

[Feature] Skip compiling the non-supported cmdlets in S.M.A.dll

e372ef3

This is to keep the cache in InitialSessionState in sycn with the actual available cmdlet types in System.Management.Automation.dll

[Feature] Fix the build on windows

b7567d8

daxian-dbw force-pushed the fixdebug branch from 497244a to b7567d8 Compare June 5, 2018 20:01

daxian-dbw mentioned this pull request Jun 5, 2018

Properly disable (if/def out) all code related to session connection/disconnection for Unix builds #7008

Closed

iSazonov merged commit 2429b43 into PowerShell:master Jun 6, 2018

daxian-dbw deleted the fixdebug branch June 6, 2018 04:42

daxian-dbw mentioned this pull request Jun 9, 2018

Cleanup: remove the unneeded type 'RemotingCommandUtils' #7029

Merged

11 tasks

Skip compiling the non-supported cmdlets in System.Management.Automation.dll #6939

Skip compiling the non-supported cmdlets in System.Management.Automation.dll #6939

Uh oh!

Conversation

daxian-dbw commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Change Details

PR Checklist

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov May 26, 2018

Choose a reason for hiding this comment

Uh oh!

rjmholt May 29, 2018

Choose a reason for hiding this comment

Uh oh!

BrucePay Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

rjmholt Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Jun 2, 2018 • 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!

daxian-dbw Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov commented May 26, 2018

Uh oh!

BrucePay left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt May 29, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jun 2, 2018

Uh oh!

daxian-dbw commented Jun 5, 2018

Uh oh!

daxian-dbw commented Jun 5, 2018

Uh oh!

PaulHigin commented Jun 5, 2018

Uh oh!

daxian-dbw commented Jun 5, 2018

Uh oh!

rjmholt commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jun 6, 2018

Uh oh!

rjmholt commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Jun 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daxian-dbw commented May 25, 2018 •

edited

Loading

iSazonov Jun 2, 2018 •

edited

Loading

rjmholt commented Jun 6, 2018 •

edited

Loading

rjmholt commented Jun 6, 2018 •

edited

Loading