-
Notifications
You must be signed in to change notification settings - Fork 73
Skip invalid executables #585
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
d235400
3363033
81f9ea2
e928ffb
307cbf2
a072eee
467af1e
25bcdd4
bb26de8
5b31c94
bb58750
9b78802
09cb3b5
3e43982
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,8 @@ import { | |
| supportsNativeServer, | ||
| versionToString, | ||
| VersionInfo, | ||
| MINIMUM_SUPPORTED_EXECUTABLE_VERSION, | ||
| supportsExecutable, | ||
| MINIMUM_NATIVE_SERVER_VERSION, | ||
| supportsStableNativeServer, | ||
| NATIVE_SERVER_STABLE_VERSION, | ||
|
|
@@ -83,6 +85,28 @@ async function getRuffVersion(executable: string): Promise<VersionInfo> { | |
| return { major, minor, patch }; | ||
| } | ||
|
|
||
| /** | ||
| * Validate and log executable from given path. | ||
| */ | ||
| async function validateUsingExecutable(executable: string, strategy: string) { | ||
| try { | ||
| const ruffVersion = await getRuffVersion(executable); | ||
| if (!supportsExecutable(ruffVersion)) { | ||
| var message = `Skip unsupported executable from ${strategy}: ${executable}`; | ||
| message += `(Reqiuired at least ${versionToString( | ||
| MINIMUM_SUPPORTED_EXECUTABLE_VERSION, | ||
| )}, but found ${versionToString(ruffVersion)} instead)`; | ||
| traceError(message); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should also display this message to the users using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. during
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message shouldn't be required because we already check that later on. Refer to my other comment.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷♂️ |
||
| return false; | ||
| } | ||
| traceInfo(`Using ${strategy}: ${executable}`); | ||
| return true; | ||
| } catch (ex) { | ||
| traceInfo(`Skip invalid executable from ${strategy}: ${executable}`); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we include the exception message as well? Are we able to capture the one in #426 ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, makes sense. Since traceInfo(`Skip invalid executable from ${strategy}: ${executable}:\n${ex}`);is this correct to you?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be fine but I'd need to play around with it to check how it renders in the output window. I can do that when I'm testing before merging the PR. |
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Finds the Ruff binary path and returns it. | ||
| * | ||
|
|
@@ -110,8 +134,7 @@ async function findRuffBinaryPath( | |
| // 'path' setting takes priority over everything. | ||
| if (settings.path.length > 0) { | ||
| for (const path of settings.path) { | ||
| if (await fsapi.pathExists(path)) { | ||
| traceInfo(`Using 'path' setting: ${path}`); | ||
| if (await fsapi.pathExists(path) && await validateUsingExecutable(path, "'path' setting")) { | ||
| return path; | ||
| } | ||
| } | ||
|
|
@@ -142,16 +165,14 @@ async function findRuffBinaryPath( | |
| traceError(`Error while trying to find the Ruff binary: ${err}`); | ||
| } | ||
|
|
||
| if (ruffBinaryPath && ruffBinaryPath.length > 0) { | ||
| // First choice: the executable found by the script. | ||
| traceInfo(`Using the Ruff binary: ${ruffBinaryPath}`); | ||
| // First choice: the executable found by the script. | ||
| if (ruffBinaryPath && ruffBinaryPath.length > 0 && await validateUsingExecutable(ruffBinaryPath, "the Ruff binary")) { | ||
| return ruffBinaryPath; | ||
| } | ||
|
|
||
| // Second choice: the executable in the global environment. | ||
| const environmentPath = await which(RUFF_BINARY_NAME, { nothrow: true }); | ||
| if (environmentPath) { | ||
| traceInfo(`Using environment executable: ${environmentPath}`); | ||
| if (environmentPath && await validateUsingExecutable(environmentPath, "environment executable")) { | ||
| return environmentPath; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.