-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Adding hex format for native command exit codes #21067
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
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -48,7 +48,7 @@ Describe 'Native command error handling tests' -Tags 'CI' { | |||
|
|||
$error[0].FullyQualifiedErrorId | Should -BeExactly 'ProgramExitedWithNonZeroCode' | |||
$error[0].TargetObject | Should -BeExactly $exePath | |||
$stderr[1].Exception.Message | Should -BeExactly "Program `"$exeName`" ended with non-zero exit code: 1." | |||
$stderr[1].Exception.Message | Should -BeExactly "Program `"$exeName`" ended with non-zero exit code: 1 (0x00000001)." |
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.
Why not (0x1)
?
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.
Because the hex format used is the fixed-sized 0xNNNNNNNN
, for readability's sake. Call it a matter of taste, but I find reading values such as 0xFE234
"less pretty/readable/usable" than 0x000FE234
.
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.
How this lokks in docs https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499-
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.
Just an FYI while on Windows the rc is a 32-bit integer, on Unix they are just a single byte. I don't have a particular preference either way just wanted to share that in case it helps to make a decision.
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.
Just an FYI while on Windows the rc is a 32-bit integer, on Unix they are just a single byte. I don't have a particular preference either way just wanted to share that in case it helps to make a decision.
Are you somehow suggesting that the parenthesized hex value should use 0x{1:X}
format on non-Windows platforms and 0x{1:X8}
format on Windows?
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 say about remove leading zeros.
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.
@SteveL-MSFT shouldn't that be discussed / decided upon by the team? Who is supposed to have the final say on this?
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.
The Engine WG discussed this today and agree that on Linux & macOS the hexadecimal format should be 0x{1:X2}
. We also suggest removing the formatting string from the resx and put it in code. That is change this:
<value>Program "{0}" ended with non-zero exit code: {1} (0x{1:X8}).</value>
to
<value>Program "{0}" ended with non-zero exit code: {1} (0x{2}).</value>
Then in code, do something like this:
const string errorId = nameof(CommandBaseStrings.ProgramExitedWithNonZeroCode);
const string hexFormatStr = Platform.IsWindows ? "{0:X8}" : "{0:X2}";
string errorMsg = StringUtil.Format(
CommandBaseStrings.ProgramExitedWithNonZeroCode,
NativeCommandName,
_nativeProcess.ExitCode,
String.Format(CultureInfo.InvariantCulture, hexFormatStr, _nativeProcess.ExitCode));
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
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.
Changes made and committed to sba923:master. How do I add this to the PR?
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
2 similar comments
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -17,7 +17,7 @@ | |||
<ItemGroup> | |||
<!-- This section is to force the version of non-direct dependencies --> | |||
<!-- the following package(s) are from https://github.com/dotnet/fxdac --> | |||
<PackageReference Include="System.Data.SqlClient" Version="4.8.5" /> | |||
<PackageReference Include="System.Data.SqlClient" Version="4.8.6" /> |
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.
Does this change need to be part of this PR?
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.
Nope, this clearly doesn't, but the CI builds all fail without that change....
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.
Other than this issue, the PR LGTM.
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.
OK, will let someone from the PS team chime in on what's going on here.
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.
Great.
Will fix the Code Factor issue, BTW.
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.
Will fix the Code Factor issue, BTW.
Done
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.
OK, will let someone from the PS team chime in on what's going on here.
I've done this in a separate commit, JIC.
If this has to be done elsewhere (which would totally make sense) I'll probably need some help to exclude that commit from the PR, 'cos that's something I've never done.
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 change was made in #21084.
I would suggest updating the PR with a merge commit.
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've done this in a separate commit, JIC.
If this has to be done elsewhere (which would totally make sense) I'll probably need some help to exclude that commit from the PR, 'cos that's something I've never done.
@sba923 Just revert that specific commit now that it's been in the PR mentioned above by @xtqqczze and ensure that you rebase from the powershell/main branch as that's cleaner than adding merge commits in your PR
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.
Hi, I have reverted the commit (which results in the CI build to now fail with /home/runner/work/PowerShell/PowerShell/src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj : error NU1903: Warning As Error: Package 'System.Data.SqlClient' 4.8.5 has a known high severity vulnerability, https://github.com/advisories/GHSA-98g6-xh36-x2p7 [/home/runner/work/PowerShell/PowerShell/src/powershell-unix/powershell-unix.csproj]
), but I can't seem to get the rebase to work. Can you please help me out with that (either using the git CLI or VScode)?
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Reopen to restart CIs. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -969,11 +969,13 @@ internal override void Complete() | |||
} | |||
|
|||
const string errorId = nameof(CommandBaseStrings.ProgramExitedWithNonZeroCode); | |||
string hexFormatStr = Platform.IsWindows ? "0x{0:X8}" : "0x{0:X2}"; |
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.
We prefer #if UNIX
over runtime check.
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.
We prefer
#if UNIX
over runtime check.
Change made.
Note: the codebase contains a significant number of runtime tests e.g. if (Platform.IsWindows)...
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
What's still blocking this PR? |
@sba923 this will get another review before being accepted - also please see the comment I made above about reverting that commit & rebasing from main |
What's still blocking this PR? I'll probably need help with that rebasing action... |
|
Last CI run reports What should I do next? |
@daxian-dbw what's still blocking this PR? Is there anything I can / should do? |
@daxian-dbw @TravisEz13 @adityapatwardhan @anmenaga |
🏓 |
If I understand correctly, this is blocked because it hasn't been reviewed.... |
@daxian-dbw Any chance you could review this? It's a pretty small & straightforward PR. |
@rkeithhill / @daxian-dbw is there anything I can / should do to unblock this? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
📣 Hey @@sba923, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
PR Summary
This implementation adds the hex-format version of the native command exit code message (#19886)
PR Context
PR Checklist
[X ] PR has a meaningful title
[ X] Summarized changes
[X ] Make sure all
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright header[ X] This PR is ready to merge and is not Work in Progress.
WIP:
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.Breaking changes
User-facing changes
Testing - New and feature
Tooling
(which runs in a different PS Host).