-
Notifications
You must be signed in to change notification settings - Fork 716
feat(amazonq): Bundle LSP with the extension as fallback. #7421
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
|
try { | ||
await lspSetupStage('all', async () => { | ||
await lspSetupStage('launch', async () => await startLanguageServer(ctx, getBundledResourcePaths(ctx))) | ||
}) | ||
} catch (error) { |
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 fallback logic should live in the resolver, just like all the other fallback cases. it doesn't make sense to add a fallback at this level.
this fallback is no different than the other fallback cases in the resolver. it's just yet another fallback scenario.
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.
No. In the case of a firewall/anti virus, the resolver believes it successfully finished downloading and it then proceed to start the language server with the download path, but at that moment anti virus kicked in and broke it.
This outmost try catch can handle such cases.
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 resolver believes it successfully finished downloading and it then proceed to start the language server with the download path
isn't that a bug in the resolver that should be fixed / redesigned ?
let resourcesRoots = [lspDir, dist] | ||
if (this.mynahUIPath?.startsWith(globals.context.extensionUri.fsPath)) { | ||
getLogger('amazonqLsp').info(`Using bundled webview resources ${bundledResources.fsPath}`) |
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 is this decision being made here instead of in LanguageServerResolver
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.
Same reason as I mentioned above. When it failed to launch, there are cases when the anti virus modified/removed the downloaded artifacts later on. The LanguageServerResolver
does not throw, it indeed successfully unzipped those artifacts.
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.
If LanguageServerResolver
does not throw, I will not be able to resolve to bundled artifact
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.
When it failed to launch, there are cases when the anti virus modified/removed the downloaded artifacts later on. The
LanguageServerResolver
does not throw, it indeed successfully unzipped those artifacts.
Can it do a step that verifies the contents after unzipping? I.e. whatever causes the AV to quarantine the artifacts, the resolver should do itself before continuing.
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
await ensureDirectoryExists(resourcesDir) | ||
|
||
return new Promise((resolve, reject) => { | ||
const manifestUrl = 'https://aws-toolkit-language-servers.amazonaws.com/qAgenticChatServer/0/manifest.json' |
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.
Problem
The LSP start failure because
Solution
This was tested by
also tested by manually corrupting the aws-lsp-codewhisperer.js
Limitations:
Ref: aws/aws-toolkit-jetbrains#5772
feature/x
branches will not be squash-merged at release time.