Skip to content

Make all errors on an unsupported platform ConfigurationErrors #3005

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 7, 2025
Merged

Make all errors on an unsupported platform ConfigurationErrors #3005

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b33b61
Make all errors on an unsupported platform `ConfigurationError`s
redsun82 Aug 7, 2025
1cfc0c2
Add tests for `cli-errors` and fix one bug
redsun82 Aug 7, 2025
bb56324
Address review
redsun82 Aug 7, 2025
739fb03
Merge branch 'main' into redsun82/unsupported-plat
redsun82 Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Make all errors on an unsupported platform ConfigurationErrors
  • Loading branch information
@redsun82
redsun82 committed Aug 7, 2025
commit 7b33b610d4e72050a43339fd3ef1c113b6e692b4
28 changes: 25 additions & 3 deletions lib/cli-errors.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/cli-errors.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 37 additions & 3 deletions src/cli-errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ export const cliErrorsConfig: Record<
* the error messages in the config record, or the exit codes match, return the error category;
* if not, return undefined.
*/
export function getCliConfigCategoryIfExists(
function getCliConfigCategoryIfExists(
cliError: CliError,
): CliConfigErrorCategory | undefined {
for (const [category, configuration] of Object.entries(cliErrorsConfig)) {
Expand All @@ -317,11 +317,45 @@ export function getCliConfigCategoryIfExists(
}

/**
* Changes an error received from the CLI to a ConfigurationError with optionally an extra
* error message appended, if it exists in a known set of configuration errors. Otherwise,
* Check if we are running on an unsupported platform/architecture combination.
* If so, return a ConfigurationError with a message that explains that, mentioning
* the underlying `cliError`. Otherwise, reutrn `undefined`.
*/
function getUnsupportedPlatformError(
cliError: CliError,
): ConfigurationError | undefined {
if (
![
["linux", "x64"],
["win32", "x64"],
["darwin", "x64"],
["darwin", "arm64"],
].some(
Copy link
Member

Choose a reason for hiding this comment

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

Minor: It might be nicer if this array was a top-level constant, rather than inlined here.

([platform, arch]) =>
platform === process.platform && arch === process.arch,
)
) {
return new ConfigurationError(
"The CodeQL CLI does not support the platform/architecture combination of " +
`${process.platform}/${process.arch} ` +
"(see https://codeql.github.com/docs/codeql-overview/system-requirements). " +
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Should we add this to the DocUrl enum?

`The underlying error was: ${cliError.message}`,
);
}
return undefined;
}

/**
* Changes an error received from the CLI to a ConfigurationError with the message
* optionally being transformed, if it is a known configuration error. Otherwise,
* simply returns the original error.
*/
export function wrapCliConfigurationError(cliError: CliError): Error {
const unsupportedPlatformError = getUnsupportedPlatformError(cliError);
if (unsupportedPlatformError !== undefined) {
return unsupportedPlatformError;
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit like an improvised try/catch block. Why not just return a bool from a function that checks if the platform is supported and then throw the error here?


const cliConfigErrorCategory = getCliConfigCategoryIfExists(cliError);
if (cliConfigErrorCategory === undefined) {
return cliError;
Expand Down