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

Rework the error list and add a command to get error information #3615

Merged
merged 7 commits into from Sep 14, 2023

Conversation

JohnMcPMS
Copy link
Member

Change

This change reworks the error description switch statement into a sorted array of data. We then use binary search to find the appropriate description when needed. In addition, it now supports these values being localized.

This change also adds the error command (hidden) to enable a somewhat easier path to getting error information specific to winget. The command takes in a single value. If this is wholly a number, it is treated as the error number itself. If it is not a number, it is treated as a string to search within the symbols and descriptions of the winget errors.

PS > wingetdev error 0x8a15c001
0x8a15c001 : WINGET_CONFIG_ERROR_INVALID_CONFIGURATION_FILE
The configuration file is invalid.
PS > wingetdev error 5
0x00000005
Access is denied.
PS > wingetdev error shell
0x8a150006 : APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED
Running ShellExecute failed
PS > wingetdev error archive
0x8a15005c : APPINSTALLER_CLI_ERROR_EXTRACT_ARCHIVE_FAILED
Failed to extract archive.
0x8a150060 : APPINSTALLER_CLI_ERROR_ARCHIVE_SCAN_FAILED
Archive malware scan failed.
0x8a15005b : APPINSTALLER_CLI_ERROR_NESTEDINSTALLER_NOT_FOUND
Failed to find the nested installer in the archive.

Validation

Added a unit test to ensure that the error data is sorted.
Added E2E tests for the error command.

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner September 11, 2023 23:29
@Trenly
Copy link
Contributor

Trenly commented Sep 12, 2023

Does this work when the integer is negative? For example, the return code for APPINSTALLER_CLI_ERROR_MANIFEST_VALIDATION_FAILURE is ((HRESULT)0x8A150029). Based on your example for integer 5,, I would be curious if this was tested, since PowerShell interprets the HRESULT as -1978335191

@JohnMcPMS
Copy link
Member Author

Does this work when the integer is negative? For example, the return code for APPINSTALLER_CLI_ERROR_MANIFEST_VALIDATION_FAILURE is ((HRESULT)0x8A150029). Based on your example for integer 5,, I would be curious if this was tested, since PowerShell interprets the HRESULT as -1978335191

I can add another automated test specifically for this scenario, but yes, it should work (I did test negatives manually). The only complicating factor is that the minus sign is interpreted as an argument specifier, so one needs to provide the "stop looking for arguments" override (--) or the non-separated syntax (-a=b).

> winget error -- -1978335191
> winget error --input=-1978335191

@JohnMcPMS
Copy link
Member Author

And here are the actual commands run against the dev build:

PS > wingetdev error -- -1978335191
0x8a150029 : APPINSTALLER_CLI_ERROR_MANIFEST_VALIDATION_FAILURE
Manifest validation failed
PS > wingetdev error --input=-1978335191
0x8a150029 : APPINSTALLER_CLI_ERROR_MANIFEST_VALIDATION_FAILURE
Manifest validation failed

REQUIRE(IsSameVolume(path1, path2));
REQUIRE(IsSameVolume(path5, path7));
if (IsSameVolume(path5, path5))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if check intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is poorly considered, as it requires one to have a D drive, otherwise it will fail. The underlying OS API fails if the path does not reference an extant volume, and our function treats that as a sort of NaN result and always returns false if either path results in it. This means that D:\path1 and D:\path2 will result in false if D is not a volume. As a minimal workaround to that, I attempted to determine if D exists by checking if equal paths point to the same volume.

The test would be better if it constructed the paths based on the existing volumes rather than hard coding specific paths. But I was just trying to put a bandaid on the issue.

@JohnMcPMS JohnMcPMS merged commit a5bffb2 into microsoft:master Sep 14, 2023
8 checks passed
@JohnMcPMS JohnMcPMS deleted the error-command branch September 14, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants