Powershell: Remove FullCLR code

Created on 27 Jul 2017  路  28Comments  路  Source: PowerShell/PowerShell

This project started as a fork of the Windows PowerShell 5.1 code base. As we've progressed towards a PowerShell Core 6.0 release and with the capability of dotnet core 2.0 and dotnet std 2.0 being able to load dotnet framework 4.5.1+ assemblies that are compatible with dotnet std 2.0, we can simplify the code base by removing the legacy FullCLR code so that we no longer have #ifdefs for CORECLR (and !CORECLR). This was discussed and approved by @PowerShell/powershell-committee. When reviewing the code, do not blindly just remove the FullCLR code, but validate if updates to .NET Core means using the FullCLR code instead of the limited .NET Core code.

Remaining #if [!]CORECLR usages

param($path = "../repos/PowerShell")

$sources = Get-ChildItem -Recurse -Path $path -Include *.cs
foreach ($source in $sources) {
    $content = Get-Content $source -Raw
    if ($content -match "#if CORECLR" -or $content -match "#if !CORECLR") {
        $source.FullName
    }
}

src/Microsoft.PowerShell.Commands.Diagnostics/CoreCLR/Stubs.cs
src/Microsoft.PowerShell.Commands.Diagnostics/CommonUtils.cs
src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs
src/Microsoft.PowerShell.Commands.Diagnostics/GetCounterCommand.cs
src/Microsoft.PowerShell.Commands.Diagnostics/ImportCounterCommand.cs
src/Microsoft.PowerShell.Commands.Diagnostics/NewWinEventCommand.cs
src/Microsoft.PowerShell.Commands.Utility/commands/utility/new-object.cs
src/Microsoft.PowerShell.LocalAccounts/LocalAccounts/Extensions.cs
src/Microsoft.PowerShell.LocalAccounts/LocalAccounts/Sam.cs
src/Microsoft.PowerShell.Security/security/AclCommands.cs
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs
src/Microsoft.PowerShell.Security/security/ExecutionPolicyCommands.cs
src/Microsoft.WSMan.Management/CredSSP.cs
src/Microsoft.WSMan.Management/Interop.cs
src/Microsoft.WSMan.Management/WsManHelper.cs
src/System.Management.Automation/cimSupport/cmdletization/xml/CoreCLR/cmdlets-over-objects.xmlSerializer.autogen.cs
src/System.Management.Automation/cimSupport/cmdletization/ScriptWriter.cs
src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs
src/System.Management.Automation/DscSupport/CimDSCParser.cs
src/System.Management.Automation/engine/debugger/debugger.cs
src/System.Management.Automation/engine/hostifaces/Command.cs
src/System.Management.Automation/engine/hostifaces/Connection.cs
src/System.Management.Automation/engine/hostifaces/ConnectionBase.cs
src/System.Management.Automation/engine/hostifaces/HostUtilities.cs
src/System.Management.Automation/engine/hostifaces/InternalHost.cs
src/System.Management.Automation/engine/hostifaces/LocalConnection.cs
src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs
src/System.Management.Automation/engine/hostifaces/Parameter.cs
src/System.Management.Automation/engine/hostifaces/PowerShell.cs
src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs
src/System.Management.Automation/engine/hostifaces/RunspacePool.cs
src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs
src/System.Management.Automation/engine/interpreter/ControlFlowInstructions.cs
src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
src/System.Management.Automation/engine/parser/Compiler.cs
src/System.Management.Automation/engine/parser/FusionAssemblyIdentity.cs
src/System.Management.Automation/engine/parser/GlobalAssemblyCache.cs
src/System.Management.Automation/engine/parser/Parser.cs
src/System.Management.Automation/engine/parser/TypeResolver.cs
src/System.Management.Automation/engine/remoting/client/remoterunspace.cs
src/System.Management.Automation/engine/remoting/commands/CustomShellCommands.cs
src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs
src/System.Management.Automation/engine/remoting/commands/remotingcommandutil.cs
src/System.Management.Automation/engine/remoting/commands/ResumeJob.cs
src/System.Management.Automation/engine/remoting/commands/SuspendJob.cs
src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs
src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs
src/System.Management.Automation/engine/remoting/common/throttlemanager.cs
src/System.Management.Automation/engine/remoting/fanin/BaseTransportManager.cs
src/System.Management.Automation/engine/remoting/fanin/InitialSessionStateProvider.cs
src/System.Management.Automation/engine/remoting/fanin/WSManPlugin.cs
src/System.Management.Automation/engine/remoting/fanin/WSManPluginFacade.cs
src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs
src/System.Management.Automation/engine/remoting/server/OutOfProcServerMediator.cs
src/System.Management.Automation/engine/remoting/server/ServerPowerShellDriver.cs
src/System.Management.Automation/engine/remoting/server/serverremotesession.cs
src/System.Management.Automation/engine/remoting/server/ServerRunspacePoolDriver.cs
src/System.Management.Automation/engine/remoting/server/ServerSteppablePipelineDriver.cs
src/System.Management.Automation/engine/Attributes.cs
src/System.Management.Automation/engine/CommandDiscovery.cs
src/System.Management.Automation/engine/CommandMetadata.cs
src/System.Management.Automation/engine/ExecutionContext.cs
src/System.Management.Automation/engine/GetEvent_Types_Ps1Xml.cs
src/System.Management.Automation/engine/InitialSessionState.cs
src/System.Management.Automation/engine/LanguagePrimitives.cs
src/System.Management.Automation/engine/MshCommandRuntime.cs
src/System.Management.Automation/engine/pipeline.cs
src/System.Management.Automation/engine/serialization.cs
src/System.Management.Automation/engine/Types_Ps1Xml.cs
src/System.Management.Automation/engine/Utils.cs
src/System.Management.Automation/help/HelpSystem.cs
src/System.Management.Automation/help/UpdatableHelpCommandBase.cs
src/System.Management.Automation/help/UpdatableHelpSystem.cs
src/System.Management.Automation/help/UpdateHelpCommand.cs
src/System.Management.Automation/logging/MshLog.cs
src/System.Management.Automation/namespaces/FileSystemProvider.cs
src/System.Management.Automation/namespaces/RegistryProvider.cs
src/System.Management.Automation/namespaces/Win32Native.cs
src/System.Management.Automation/security/SecureStringHelper.cs
src/System.Management.Automation/singleshell/config/MshSnapinInfo.cs
src/System.Management.Automation/utils/ClrFacade.cs
src/System.Management.Automation/utils/StructuredTraceSource.cs

Committee-Reviewed Issue-Code Cleanup

Most helpful comment

EDIT: Moved the list into the top-level issue description at the request of @daxian-dbw.

[big list used to be here]

All 28 comments

Could we save the code history in a reference branch? It can help with backward compatibility issues.

What is a plan - one PR from MSFT or multiple PRs from community too?

Search results for #if .*CORECLR - 543 results in 186 files.

Related. From #2969 @daxian-dbw:

We need to clean up ApartmentState related workarounds in powershell now.

Search results for ApartmentState 389 results in 52 files

I found #if V2 in src\System.Management.Automation\minishell\apiRunspaceConfiguration.cs 馃槙

3565 is related. I reviewed CORECLR (and !CORECLR) - most of these relate with moving to .Net 2.0 - assembly load, ApartmentState/STA, unimplemented classes/methods/properties in .Net 1.0 and so on. Sometimes it's hard to distinguish. So it's probably easier to clean up the file by file.

What is a plan - one PR from MSFT or multiple PRs from community too?

The work should be split to multiple PRs for sure, for example, the AprtmentState related changes should be one PR.
I think this work needs to be done by people who are very familiar with the code base because it's not a simple code removal work -- some code blocks with #if !CORECLR might need to enabled because the missing types are back in .NET Core 2.0; some code blocks might need to be kept because the desired functionality was disabled in powershell core because the code needs to be somehow adjusted or rewritten.

Agree that we should divide up this work as multiple PRs from multiple people. Since we're formally deprecated v2, we should clean up that code as well.

Thanks for clarify!

I am trying to clean up ClrFacade.cs and see all mentioned cases. I can fix simple cases but leave AprtmentState and assemblies to you :-) - I believe the full cleanup require a huge work and I save your time if make obvious cleanups?

As I'm making other changes, I'm removing legacy code if I see it.

@iSazonov yes, please make obvious cleanups as you see them. But please try to keep each PR small so that we don't miss anything in the code review :)

@SteveL-MSFT cmdlets like Enable-PSRemoting and Disable-PSRemoting contain a lot of scripts that deal with Windows PowerShell, shall we also clean up those scripts?

@daxian-dbw yes

Note that if there is a module that we would eventually split out and publish separately on PSGallery. That module should support Windows PowerShell (in general) and would retain FullCLR code (don't know if they have specific code paths, but Archive and ODataUtils would be examples)

@daxian-dbw @adityapatwardhan Thanks for fast reviews!

We have many '#if CORE' for "assemlies" and hude for STA/apartment. Also we should review code base on .Net Core 1.0 -> 2.0 enhancements. I think it is better to make explicit points in #3565 for that and close the Issue.

3565 is more about reviewing '#if UNIX' blocks, while this one is focused on '#if CORECLR' blocks.

I think it's better to separate these two depending on how the cleanup work is being done. Someone probably wants to focus on removing the dead Full CLR code, so he/she can leave the related #if UNIX block untouched without worrying that the change will cause a regression in Unix plats.

I'm in process of taking care of ApartmentState

Who we can ask to clean up web cmdlets?

@dantraMSFT should clean up the web cmdlets

We should continue to clean up the code, but it's not a blocker for 6.0.0 release

EDIT: Moved the list into the top-level issue description at the request of @daxian-dbw.

[big list used to be here]

@rjmholt Great work!
Could you please move your post in new Meta-Issue and add link here?
Can MSFT team prioritize?

Note: The CustomPSSnapin class is not even available therefore Snapins are probably not even achievable for FullCLR with the open source code given by this repo. I do not think this is an issue since no one likes/wants Snapin any more due to modules being a better choice. Therefore the better solution could be to completely remove PSSnapin related code.

It is not difficult until the code is used in serialization.

@SteveL-MSFT Could you please update a status of the Issue? Seems most was fixed and now it is not clear that we can fix yet.

@iSazonov I made an attempt to see what in the check list is already fixed and what isn't. The problem is that the hyperlinks go to older commits and don't reflect current state. Might be easier to write a script to scan the code and generate a new checklist but we won't know what has been fixed, just what needs to be fixed.

I caught the same and thought that you have an idea - good point about script. We could review "file by file".

One thing we should be careful of is not straight removing the FullClr code, but seeing if the Windows PowerShell code now works because of updates to .NET Core.

@SteveL-MSFT Seems we can close the Issue: all #if FullCLR code was removed (I found FullCLR only in comments) and we have #3565 to track updates of .NET Core.

Update: I found still up to 100 '#if !CORECLR' (like appdomain, apartment) so maybe not close the PR.

Search results for #if .*CORECLR - 543 results in 186 files.

Now search results for #if .*CORECLR - 174 results in 70 files.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

rudolfvesely picture rudolfvesely  路  3Comments

manofspirit picture manofspirit  路  3Comments

garegin16 picture garegin16  路  3Comments

alx9r picture alx9r  路  3Comments

Michal-Ziemba picture Michal-Ziemba  路  3Comments