Skip to content
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

Remove FullCLR code #4357

Closed
SteveL-MSFT opened this issue Jul 27, 2017 · 32 comments
Closed

Remove FullCLR code #4357

SteveL-MSFT opened this issue Jul 27, 2017 · 32 comments
Labels
Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality Resolution-No Activity Issue has had no activity for 6 months or more

Comments

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jul 27, 2017

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

@SteveL-MSFT SteveL-MSFT added the Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality label Jul 27, 2017
@SteveL-MSFT SteveL-MSFT added this to the 6.0.0-HighPriority milestone Jul 27, 2017
@iSazonov iSazonov added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Jul 27, 2017
@iSazonov
Copy link
Collaborator

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

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 27, 2017

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

@iSazonov
Copy link
Collaborator

I found #if V2 in src\System.Management.Automation\minishell\api\RunspaceConfiguration.cs 😕

@iSazonov
Copy link
Collaborator

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

@daxian-dbw
Copy link
Member

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.

@SteveL-MSFT
Copy link
Member Author

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.

@iSazonov
Copy link
Collaborator

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?

@SteveL-MSFT
Copy link
Member Author

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

@daxian-dbw
Copy link
Member

@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 :)

@daxian-dbw
Copy link
Member

@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?

@SteveL-MSFT
Copy link
Member Author

@daxian-dbw yes

@SteveL-MSFT
Copy link
Member Author

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)

@SteveL-MSFT
Copy link
Member Author

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

@rjmholt
Copy link
Collaborator

rjmholt commented Apr 3, 2018

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

[big list used to be here]

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 3, 2018

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

@bergmeister
Copy link
Contributor

bergmeister commented Aug 5, 2018

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.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 9, 2018

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

@iSazonov
Copy link
Collaborator

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

@SteveL-MSFT
Copy link
Member Author

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

@iSazonov
Copy link
Collaborator

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

@SteveL-MSFT
Copy link
Member Author

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.

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 26, 2018

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

@iSazonov
Copy link
Collaborator

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

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

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

1 similar comment
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution-No Activity Issue has had no activity for 6 months or more label Nov 16, 2023
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality Resolution-No Activity Issue has had no activity for 6 months or more
Projects
None yet
Development

No branches or pull requests

5 participants