-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Refactor command line parser to do early parsing #11482
Conversation
bd2928e
to
bd527ba
Compare
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs
Outdated
Show resolved
Hide resolved
@SteveL-MSFT @rjmholt @daxian-dbw It seems MSFT team has plans for the area. I believe the PR should be merged and then xUnit tests should be created. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@SteveL-MSFT @rjmholt @daxian-dbw Please review. |
@daxian-dbw I added some commits and don't see more issues I can fix. |
@iSazonov Why mixing style/formatting changes with functionality changes? This makes it harder to review and thus the risk of regression is higher, especially for a large refactoring PR like this one ... It looks some of confusing changes were from other merged/reverted PRs, so never mind reverting the style/formatting changes. But please avoid mixing functionality changes with unrelated style changes in future PRs. |
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs
Outdated
Show resolved
Hide resolved
@iSazonov I depend on a tool called CodeFlow to review the changes in this PR and it won't work if there are more than 63 commits in the PR. So please don't use too many commits when addressing my comments. I would prefer 1 commit to address all my comments if possible. Also, please don't make changes to the existing commits and force push, or I will lose track of my review state. |
Sorry, I misunderstand your previous request. :-( |
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs
Outdated
Show resolved
Hide resolved
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. Oh man, it's not easy for both you and me 😄
Sorry, again. As I said the original plan was only to get rid of the workarounds with "early parsing"... Additional notices:
|
@iSazonov Don't get me wrong, I appreciate you taking on refactoring work like this one, which usually would require a lot patience to get through the long review process. To be honest, I admit that I'm kinda afraid of PRs like this one -- it obviously has values but also has a high risk of regression -- which would take me a lot efforts to review to get to the point where I'm sort of confident to accept the PR. That's why I said "it's not easy for both you and me" 😄
Agreed.
No, I don't think the argument parsing is the bottleneck, so don't waste your time there. |
🎉 Handy links: |
PR Summary
Previously we had to make workarounds in #6025 and #7631 to enable logging as early as possible.
A root of the problem is that command line parser has a dependency on ConsoleHost being initialized late.
Main change in the PR is to refactor CommandLineParameterParser and remove the dependency on ConsoleHost.
Now we create CommandLineParameterParser at startup as static and can run the parser immediately after pwsh launch. Later, after ConsoleHost is initialized, we write the parser's errors, banner and helps.
Now we call PSEtwLog.LogConsoleStartup() early in one place for both Unix and Windows.
While refactoring and testing the parser I discovered NRE in an edge case:
pwsh -removeworkingdirectorytrailingcharacter
throws NRE. As result I added#nullable enable
in some files to make the code more reliable.UnmanagedPSEntry.Start() uses still old signature. It was refactored too: new method create with removed unneeded parameters and old one was marked with Obsolete attribute.
CommandLineParameterParser.WriteCommandLineError() method was renamed to SetCommandLineError().
In CommandLineParameterParser.ParseHelper() was added fast return if pwsh started with no parameters (args.Length == 0).
Unused parameters was removed from the parser - iss, isswait and modules.
Waiting a debugger in remoting scenario was moved in WaitingRemoteDebugger() method that called later after ConsoleHost initialized.
CommandLineParameterParser.EarlyParse() was removed - now main parser is called in the place.
NullHostUserInterface was not fully refactored for nullable reference types because PSHostUserInterface base class must be refactored first. We can defer the work.
After the change that we can finally add xUnix tests in follow PR.
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.