From 7b3f5ca6f658b651e53a6eb14405d36ff8dbf63f Mon Sep 17 00:00:00 2001 From: gnikit Date: Wed, 5 Oct 2022 18:36:49 +0100 Subject: [PATCH 1/4] bug: `fortls` relative path resolution is unsuccessful Fixes #693 --- CHANGELOG.md | 2 ++ src/lsp/client.ts | 59 +++++++++++++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df01460c..1675cbc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Fixed bugs in relative path resolution for `fortls` + ([#693](https://github.com/fortran-lang/vscode-fortran-support/issues/693)) - Fixed issues with linter unittests running asynchronously ([#623](https://github.com/fortran-lang/vscode-fortran-support/pull/623)) - Fixed `npm run watch-dev` not syncing changes to spawned Extension Dev Host diff --git a/src/lsp/client.ts b/src/lsp/client.ts index 0cc1bdfe..35326552 100644 --- a/src/lsp/client.ts +++ b/src/lsp/client.ts @@ -1,5 +1,6 @@ 'use strict'; +import * as os from 'os'; import * as path from 'path'; import * as vscode from 'vscode'; import { spawnSync } from 'child_process'; @@ -13,7 +14,6 @@ import { getOuterMostWorkspaceFolder, pipInstall, resolveVariables, - pathRelToAbs, } from '../lib/tools'; import { Logger } from '../services/logging'; import { RestartLS } from '../features/commands'; @@ -84,9 +84,7 @@ export class FortlsClient { if (!isFortran(document)) return; const args: string[] = await this.fortlsArguments(); - const fortlsPath = workspace.getConfiguration(EXTENSION_ID).get('fortls.path'); - let executablePath = resolveVariables(fortlsPath); - if (fortlsPath !== 'fortls') executablePath = pathRelToAbs(fortlsPath, document.uri); + const executablePath: string = await this.fortlsPath(document); // Detect language server version and verify selected options this.version = this.getLSVersion(executablePath, args); @@ -308,14 +306,7 @@ export class FortlsClient { */ private async fortlsDownload(): Promise { const config = workspace.getConfiguration(EXTENSION_ID); - let ls = resolveVariables(config.get('fortls.path')); - // The path can be resolved as a relative path if it's part of a workspace - // AND it does not have the default value `fortls` or is an absolute path - if (workspace.workspaceFolders == undefined && ls !== 'fortls' && !path.isAbsolute(ls)) { - const root = workspace.workspaceFolders[0]; - this.logger.debug(`[lsp.client] Assuming relative fortls path is to ${root.uri.fsPath}`); - ls = pathRelToAbs(ls, root.uri); - } + const ls = await this.fortlsPath(); // Check for version, if this fails fortls provided is invalid const results = spawnSync(ls, ['--version']); @@ -349,6 +340,50 @@ export class FortlsClient { }); } + /** + * Try and find the path to the `fortls` executable. + * It will first try and fetch the top-most workspaceFolder from `document`. + * If that fails because the document is standalone and does not belong in a + * workspace it will assume that relative paths are wrt `os.homedir()`. + * + * If the `document` argument is missing, then it will try and find the + * first workspaceFolder and use that as the root. If that fails it will + * revert back to `os.homedir()`. + * + * @param document Optional textdocument + * @returns a promise with the path to the fortls executable + */ + private async fortlsPath(document?: TextDocument): Promise { + // Get the workspace folder that contains the document, this can be undefined + // which means that the document is standalone and not part of any workspace. + let folder: vscode.WorkspaceFolder | undefined; + if (document) { + folder = workspace.getWorkspaceFolder(document.uri); + } + // If the document argument is missing, such as in the case of the Client's + // activation, then try and fetch the first workspace folder to use as a root. + else { + folder = workspace.workspaceFolders[0] ? workspace.workspaceFolders[0] : undefined; + } + + // Get the outer most workspace folder to resolve relative paths, but if + // the folder is undefined then use the home directory of the OS + const root = folder ? getOuterMostWorkspaceFolder(folder).uri : vscode.Uri.parse(os.homedir()); + + const config = workspace.getConfiguration(EXTENSION_ID); + let executablePath = resolveVariables(config.get('fortls.path')); + + // The path can be resolved as a relative path if: + // 1. it does not have the default value `fortls` AND + // 2. is not an absolute path + if (executablePath !== 'fortls' && !path.isAbsolute(executablePath)) { + this.logger.debug(`[lsp.client] Assuming relative fortls path is to ${root.fsPath}`); + executablePath = path.join(root.fsPath, executablePath); + } + + return executablePath; + } + /** * Restart the language server */ From 1c6d49b4febfd23a5d2e4e6d8375ef51fd0d1439 Mon Sep 17 00:00:00 2001 From: gnikit Date: Wed, 5 Oct 2022 18:38:06 +0100 Subject: [PATCH 2/4] refactor(test): write ui files in separate dir --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 3402e430..7a5143aa 100644 --- a/package.json +++ b/package.json @@ -722,7 +722,7 @@ "webpack": "webpack --mode production", "pretest": "npm run compile-dev && tsc -p tsconfig.test.json", "test": "npm run test:unit && npm run test:integration && npm run test:ui", - "test:ui": "extest setup-and-run -i out/test/ui/*.js -m test/ui/.mocharc.js -s .vscode-test -e .vscode-test/extensions", + "test:ui": "extest setup-and-run -i out/test/ui/*.js -m test/ui/.mocharc.js -s .vscode-test/ui -e .vscode-test/ui/extensions", "test:unit": "node ./out/test/unitTest/runTest.js", "test:integration": "node ./out/test/integration/runTest.js", "test:grammar-free": "vscode-tmgrammar-snap -s source.fortran.free -g ./syntaxes/fortran_free-form.tmLanguage.json \"./test/fortran/syntax/**/*{.f90,F90}\"", From 586a08cb466b29eda46e20fb9b61765c35501d2b Mon Sep 17 00:00:00 2001 From: gnikit Date: Wed, 5 Oct 2022 19:10:37 +0100 Subject: [PATCH 3/4] test(lsp): added integration test for path resolution --- test/fortran/.vscode/settings.json | 1 + test/unitTest/client.test.ts | 43 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 test/unitTest/client.test.ts diff --git a/test/fortran/.vscode/settings.json b/test/fortran/.vscode/settings.json index 6f9ea947..eaeedcc4 100644 --- a/test/fortran/.vscode/settings.json +++ b/test/fortran/.vscode/settings.json @@ -7,6 +7,7 @@ "DEBUG": "1", "VAL": "" }, + "fortran.fortls.path": "fortls", "fortran.fortls.disableAutoupdate": true, "fortran.fortls.preprocessor.definitions": { "HAVE_ZOLTAN": "", diff --git a/test/unitTest/client.test.ts b/test/unitTest/client.test.ts new file mode 100644 index 00000000..0e3ddb81 --- /dev/null +++ b/test/unitTest/client.test.ts @@ -0,0 +1,43 @@ +import * as vscode from 'vscode'; +import * as path from 'path'; +import { strictEqual } from 'assert'; +import { FortlsClient } from '../../src/lsp/client'; +import { Logger, LogLevel } from '../../src/services/logging'; +import { EXTENSION_ID } from '../../src/lib/tools'; + +const logger = new Logger( + vscode.window.createOutputChannel('Modern Fortran', 'log'), + LogLevel.DEBUG +); + +suite('Language Server integration tests', () => { + const server = new FortlsClient(logger); + const fileUri = vscode.Uri.file(path.resolve(__dirname, '../../../test/fortran/sample.f90')); + const config = vscode.workspace.getConfiguration(EXTENSION_ID); + const oldVal = config.get('fortls.path'); + let doc: vscode.TextDocument; + + suiteSetup(async () => { + await config.update('fortls.path', './venv/bin/fortls', false); + doc = await vscode.workspace.openTextDocument(fileUri); + }); + + test('Local path resolution (Document URI)', async () => { + const ls = await server['fortlsPath'](doc); + const root = path.resolve(__dirname, '../../../test/fortran/'); + const ref = path.resolve(root, './venv/bin/fortls'); + console.log(`Reference: ${ref}`); + strictEqual(ls, ref); + }); + + test('Local path resolution (Document missing workspaceFolders[0])', async () => { + const ls = await server['fortlsPath'](); + const root = path.resolve(__dirname, '../../../test/fortran/'); + const ref = path.resolve(root, './venv/bin/fortls'); + strictEqual(ls, ref); + }); + + suiteTeardown(async () => { + await config.update('fortls.path', oldVal, false); + }); +}); From 7117630c119438b0d9d3ad5ee97ce090af3699ac Mon Sep 17 00:00:00 2001 From: gnikit Date: Wed, 5 Oct 2022 19:18:29 +0100 Subject: [PATCH 4/4] test(lsp): add test for restarting fortls --- test/integration/lsp-client.test.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/integration/lsp-client.test.ts b/test/integration/lsp-client.test.ts index 188ff517..e73042d2 100644 --- a/test/integration/lsp-client.test.ts +++ b/test/integration/lsp-client.test.ts @@ -46,4 +46,32 @@ suite('Language Server integration tests', () => { const res = server['client']?.initializeResult; strictEqual(JSON.stringify(ref), JSON.stringify(res)); }); + + test('Restart the Language Server', async () => { + await server['restartLS'](); + await delay(3000); // wait for server to initialize + + const ref = { + capabilities: { + completionProvider: { + resolveProvider: false, + triggerCharacters: ['%'], + }, + definitionProvider: true, + documentSymbolProvider: true, + referencesProvider: true, + hoverProvider: true, + implementationProvider: true, + renameProvider: true, + workspaceSymbolProvider: true, + textDocumentSync: 2, + signatureHelpProvider: { + triggerCharacters: ['(', ','], + }, + codeActionProvider: true, + }, + }; + const res = server['client']?.initializeResult; + strictEqual(JSON.stringify(ref), JSON.stringify(res)); + }); });