From 68f4b3b65a95e35f1d3df0d20fb5d5818bf0f516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 15:38:08 +0100 Subject: [PATCH 1/7] add check for python --- src/lib/tools.ts | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/lib/tools.ts b/src/lib/tools.ts index 20c31fa6..efeaa378 100644 --- a/src/lib/tools.ts +++ b/src/lib/tools.ts @@ -142,9 +142,47 @@ export async function promptForMissingTool( * @param pyPackage name of python package in PyPi */ export async function pipInstall(pyPackage: string): Promise { - const py = 'python3'; // Fetches the top-most python in the Shell + const py = await checkPython(); + const args = ['-m', 'pip', 'install', '--user', '--upgrade', pyPackage]; - return await shellTask(py, args, `pip: ${pyPackage}`); + return await shellTask(py, args, `python3 -m pip install ${pyPackage}`); +} + +/** + * Checks whether python can be called from the shell. + * + * Tries `python` on Windows and `python3` on other platforms. + * + * TODO: this could also check for python version, which has to be > 3.7 for fortls. + * + * @returns name of the command to run python on the current platform + */ +export async function checkPython(): Promise { + let py = ""; + if (os.platform() == "win32") { + py = 'python'; + } else { + py = 'python3'; + } + const args = ['--version']; + + try { + await shellTask(py, args, 'getting python version'); + return py; + } catch (e) { + let errMsg = ""; + if (os.platform() == "win32") { + errMsg = py + " isn't callable from the shell. " + + "Please make sure python is installed and added to the PATH."; + } else { + errMsg = py + " isn't callable from the shell. Please make sure python is installed"; + } + + return await new Promise((result, reject) => { + reject(errMsg); + }); + } + } export async function shellTask(command: string, args: string[], name: string): Promise { From 8e86d44a5126610eefe1ffd10081bf334c38dd2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 18:57:19 +0100 Subject: [PATCH 2/7] feat: look for fortls in the user Scripts folder on Windows pip installs fortls in the %appdata%\Roaming\Python\Python311\Scripts\ folder, which is typically not in PATH, so the extension wouldn't find fortls after installing it. Now it also looks for fortls in this folder. Other changes: - The user configured path to fortls must now be absolute. This simplified a lot of things and it doesn't make sense to me to have multiple versions of fortls on the system, per workspace. Please let me know if this is not OK. - The fortls.disabled config value now gets stored in the USER settings instead of workspace. Similar reasons as above, it seems easier to find. --- src/lsp/client.ts | 213 ++++++++++++++++++++++++---------------------- 1 file changed, 110 insertions(+), 103 deletions(-) diff --git a/src/lsp/client.ts b/src/lsp/client.ts index 7c782f05..9c8dd56d 100644 --- a/src/lsp/client.ts +++ b/src/lsp/client.ts @@ -37,27 +37,63 @@ export class FortlsClient { } private client: LanguageClient | undefined; - private version: string | undefined; + private path: string | undefined; // path to the forls binary + private version: string | undefined; // fortls version private readonly name: string = 'Fortran Language Server'; public async activate() { - // Detect if fortls is present, download if missing or disable LS functionality - // Do not allow activating the LS functionality if no fortls is detected - await this.fortlsDownload().then(fortlsDisabled => { - if (fortlsDisabled) return; - workspace.onDidOpenTextDocument(this.didOpenTextDocument, this); - workspace.textDocuments.forEach(this.didOpenTextDocument, this); - workspace.onDidChangeWorkspaceFolders(event => { - for (const folder of event.removed) { - const client = clients.get(folder.uri.toString()); - if (client) { - clients.delete(folder.uri.toString()); - client.stop(); + const config = workspace.getConfiguration(EXTENSION_ID); + + if (!config.get['fortls.disabled']) { + // Detect if fortls is present, download if missing or disable LS functionality + // Do not allow activating the LS functionality if no fortls is detected + const fortlsFound = this.getLSPath(); + + if (!fortlsFound) { + const msg = `Forlts wasn't found on your system. + It is highly recommended to use the fortls to enable IDE features like hover, peeking, GoTos and many more. + For a full list of features the language server adds see: https://fortls.fortran-lang.org`; + + const selection = window.showInformationMessage(msg, 'Install', 'Disable'); + selection.then(async opt => { + if (opt === 'Install') { + try { + this.logger.info(`[lsp.client] Downloading ${LS_NAME}`); + const msg = await pipInstall(LS_NAME); + window.showInformationMessage(msg); + this.logger.info(`[lsp.client] ${LS_NAME} installed`); + + // restart this class + this.deactivate(); + this.activate(); + + } catch (error) { + this.logger.error(`[lsp.client] Error installing ${LS_NAME}: ${error}`); + window.showErrorMessage(error); + } + } else if (opt == 'Disable') { + config.update('fortls.disabled', true, vscode.ConfigurationTarget.Global); + this.logger.info(`[lsp.client] ${LS_NAME} disabled in settings`); } - } - }); - }); + }); + + } else { + workspace.onDidOpenTextDocument(this.didOpenTextDocument, this); + workspace.textDocuments.forEach(this.didOpenTextDocument, this); + workspace.onDidChangeWorkspaceFolders(event => { + for (const folder of event.removed) { + const client = clients.get(folder.uri.toString()); + if (client) { + clients.delete(folder.uri.toString()); + client.stop(); + } + } + }); + } + } + return; + } public async deactivate(): Promise { @@ -84,7 +120,7 @@ export class FortlsClient { if (!isFortran(document)) return; const args: string[] = await this.fortlsArguments(); - const executablePath: string = await this.fortlsPath(document); + const executablePath: string = this.path; // Detect language server version and verify selected options this.version = this.getLSVersion(executablePath, args); @@ -251,6 +287,63 @@ export class FortlsClient { return args; } + /** + * Tries to find fortls and saves its path to this.path. + * + * If a user path is configured, then only use this. + * If not, try running fortls globally, or from python user scripts folder on Windows. + * + * @returns true if fortls found, false if not + */ + private getLSPath(): boolean { + const config = workspace.getConfiguration(EXTENSION_ID); + const configuredPath = resolveVariables(config.get('fortls.path')); + + const pathsToCheck: string[] = []; + + // if there's a user configured path to the executable, check if it's absolute + if (configuredPath !== '') { + if (!path.isAbsolute(configuredPath)) { + throw Error("The path to fortls (fortls.path) must be absolute."); + } + + pathsToCheck.push(configuredPath); + + } else { // no user configured path => perform standard search for fortls + + pathsToCheck.push('fortls'); + + // On Windows, `pip install fortls --user` installs fortls to the userbase\PythonXY\Scripts path, + // so we want to look for it in this path as well. + if (os.platform() == 'win32') { + const result = spawnSync('python', ['-c', 'import site; print(site.getusersitepackages())']); + const userSitePackagesStr = result.stdout.toString().trim(); + + // check if the call above returned something, in case the site module in python ever changes... + if (userSitePackagesStr) { + const userScriptsPath = path.resolve(userSitePackagesStr, '../Scripts/fortls'); + pathsToCheck.push(userScriptsPath); + } + } + + } + + // try to run `fortls --version` for all the given paths + // if any succeed, save it to this.path and stop. + for (const pathToCheck of pathsToCheck) { + const result = spawnSync(pathToCheck, ['--version']); + if (result.status == 0) { + this.path = pathToCheck; + this.logger.info('Successfully spawned fortls with path ' + pathToCheck); + return true; + } else { + this.logger.info('Failed to spawn fortls with path ' + pathToCheck); + } + } + + return false; // fortls not found + } + /** * Check if `fortls` is present and the arguments being passed are correct * The presence check has already been done in the extension activate call @@ -299,92 +392,6 @@ export class FortlsClient { return results.stdout.toString().trim(); } - /** - * Check if fortls is present in the system, if not show prompt to install/disable. - * If disabling or erroring the function will return true. - * For all normal cases it should return false. - * - * @returns false if the fortls has been detected or installed successfully - */ - private async fortlsDownload(): Promise { - const config = workspace.getConfiguration(EXTENSION_ID); - const ls = await this.fortlsPath(); - - // Check for version, if this fails fortls provided is invalid - const results = spawnSync(ls, ['--version']); - const msg = `It is highly recommended to use the fortls to enable IDE features like hover, peeking, GoTos and many more. - For a full list of features the language server adds see: https://fortls.fortran-lang.org`; - return new Promise(resolve => { - if (results.error) { - const selection = window.showInformationMessage(msg, 'Install', 'Disable'); - selection.then(async opt => { - if (opt === 'Install') { - try { - this.logger.info(`[lsp.client] Downloading ${LS_NAME}`); - const msg = await pipInstall(LS_NAME); - window.showInformationMessage(msg); - this.logger.info(`[lsp.client] ${LS_NAME} installed`); - resolve(false); - } catch (error) { - this.logger.error(`[lsp.client] Error installing ${LS_NAME}: ${error}`); - window.showErrorMessage(error); - resolve(true); - } - } else if (opt == 'Disable') { - config.update('fortls.disabled', true); - this.logger.info(`[lsp.client] ${LS_NAME} disabled in settings`); - resolve(true); - } - }); - } else { - resolve(false); - } - }); - } - - /** - * 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 8af551bc7be737b1d2662b9896cb3be86a93fcfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 19:14:42 +0100 Subject: [PATCH 3/7] change default value of fortran.fortls.path to an empty string --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index d800d560..277a0a4c 100644 --- a/package.json +++ b/package.json @@ -394,8 +394,8 @@ "properties": { "fortran.fortls.path": { "type": "string", - "default": "fortls", - "markdownDescription": "Path to the Fortran language server (`fortls`).", + "default": "", + "markdownDescription": "Path to the Fortran language server (`fortls`) (must be absolute).", "order": 10 }, "fortran.fortls.configure": { From b3d1766935f7309421ce1cd93dfcd4c2f9e8cca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 19:28:40 +0100 Subject: [PATCH 4/7] add error message when fortls spawn fails from user defined path --- src/lsp/client.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/lsp/client.ts b/src/lsp/client.ts index 9c8dd56d..97fadecb 100644 --- a/src/lsp/client.ts +++ b/src/lsp/client.ts @@ -49,6 +49,13 @@ export class FortlsClient { // Do not allow activating the LS functionality if no fortls is detected const fortlsFound = this.getLSPath(); + const configuredPath = resolveVariables(config.get('fortls.path')); + if (configuredPath) { + const msg = `Failed to run fortls from user configured path '` + configuredPath + `'`; + await window.showErrorMessage(msg); + return; + } + if (!fortlsFound) { const msg = `Forlts wasn't found on your system. It is highly recommended to use the fortls to enable IDE features like hover, peeking, GoTos and many more. @@ -304,7 +311,8 @@ export class FortlsClient { // if there's a user configured path to the executable, check if it's absolute if (configuredPath !== '') { if (!path.isAbsolute(configuredPath)) { - throw Error("The path to fortls (fortls.path) must be absolute."); + window.showErrorMessage("The path to fortls (fortran.fortls.path) must be absolute."); + return false; } pathsToCheck.push(configuredPath); From b268e0bfc033d46b3b571b02ee23f1543d3fc266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 20:05:34 +0100 Subject: [PATCH 5/7] fix prettier call fixes pre-commit hook failing with the following message: ``` npm run format: [error] No files matching the pattern were found: "'src/**/*.{ts,json}'". [error] No files matching the pattern were found: "'test/**/*.ts'". [error] No files matching the pattern were found: "'syntaxes/**/*.json'". [error] No files matching the pattern were found: "'snippets/**/*.json'". [error] No files matching the pattern were found: "'./**/*.{md,json,yaml,yml}'". ``` --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 277a0a4c..3e7525c6 100644 --- a/package.json +++ b/package.json @@ -742,7 +742,7 @@ "posttest": "npm run format", "lint": "eslint . --ext .ts,.tsx", "lint-fix": "npm run lint -- --fix", - "format": "prettier --write 'src/**/*.{ts,json}' 'test/**/*.ts' 'syntaxes/**/*.json' 'snippets/**/*.json' './**/*.{md,json,yaml,yml}'", + "format": "prettier --write src/**/*.{ts,json} test/**/*.ts syntaxes/**/*.json snippets/**/*.json ./**/*.{md,json,yaml,yml}", "prepare": "husky install", "pre-commit": "lint-staged", "coverage": "c8 --clean npm run test" From 066ca0841b82508729a24fb6af9b51b47f4ccceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 20:12:32 +0100 Subject: [PATCH 6/7] prettier: add --end-of-line auto argument to maintain sanity on Windows --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 3e7525c6..3e71f4e6 100644 --- a/package.json +++ b/package.json @@ -742,7 +742,7 @@ "posttest": "npm run format", "lint": "eslint . --ext .ts,.tsx", "lint-fix": "npm run lint -- --fix", - "format": "prettier --write src/**/*.{ts,json} test/**/*.ts syntaxes/**/*.json snippets/**/*.json ./**/*.{md,json,yaml,yml}", + "format": "prettier --write --end-of-line auto src/**/*.{ts,json} test/**/*.ts syntaxes/**/*.json snippets/**/*.json ./**/*.{md,json,yaml,yml}", "prepare": "husky install", "pre-commit": "lint-staged", "coverage": "c8 --clean npm run test" From 174780284ddceb10cb79a6ac71aaabf7065bcedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Kone=C4=8Dn=C3=BD?= Date: Sat, 10 Dec 2022 20:13:08 +0100 Subject: [PATCH 7/7] formatting after prettier managed to run --- schemas/fortls.schema.json | 2 +- src/lib/tools.ts | 23 ++++++++++++----------- src/lsp/client.ts | 26 ++++++++++++-------------- 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/schemas/fortls.schema.json b/schemas/fortls.schema.json index a29394b8..fd3ddf70 100644 --- a/schemas/fortls.schema.json +++ b/schemas/fortls.schema.json @@ -184,4 +184,4 @@ "type": "boolean" } } -} \ No newline at end of file +} diff --git a/src/lib/tools.ts b/src/lib/tools.ts index efeaa378..fadb3707 100644 --- a/src/lib/tools.ts +++ b/src/lib/tools.ts @@ -150,16 +150,16 @@ export async function pipInstall(pyPackage: string): Promise { /** * Checks whether python can be called from the shell. - * + * * Tries `python` on Windows and `python3` on other platforms. - * + * * TODO: this could also check for python version, which has to be > 3.7 for fortls. - * + * * @returns name of the command to run python on the current platform */ export async function checkPython(): Promise { - let py = ""; - if (os.platform() == "win32") { + let py = ''; + if (os.platform() == 'win32') { py = 'python'; } else { py = 'python3'; @@ -170,19 +170,20 @@ export async function checkPython(): Promise { await shellTask(py, args, 'getting python version'); return py; } catch (e) { - let errMsg = ""; - if (os.platform() == "win32") { - errMsg = py + " isn't callable from the shell. " + - "Please make sure python is installed and added to the PATH."; + let errMsg = ''; + if (os.platform() == 'win32') { + errMsg = + py + + " isn't callable from the shell. " + + 'Please make sure python is installed and added to the PATH.'; } else { errMsg = py + " isn't callable from the shell. Please make sure python is installed"; - } + } return await new Promise((result, reject) => { reject(errMsg); }); } - } export async function shellTask(command: string, args: string[], name: string): Promise { diff --git a/src/lsp/client.ts b/src/lsp/client.ts index 97fadecb..7493d607 100644 --- a/src/lsp/client.ts +++ b/src/lsp/client.ts @@ -73,7 +73,6 @@ export class FortlsClient { // restart this class this.deactivate(); this.activate(); - } catch (error) { this.logger.error(`[lsp.client] Error installing ${LS_NAME}: ${error}`); window.showErrorMessage(error); @@ -83,7 +82,6 @@ export class FortlsClient { this.logger.info(`[lsp.client] ${LS_NAME} disabled in settings`); } }); - } else { workspace.onDidOpenTextDocument(this.didOpenTextDocument, this); workspace.textDocuments.forEach(this.didOpenTextDocument, this); @@ -100,7 +98,6 @@ export class FortlsClient { } return; - } public async deactivate(): Promise { @@ -296,11 +293,11 @@ export class FortlsClient { /** * Tries to find fortls and saves its path to this.path. - * + * * If a user path is configured, then only use this. * If not, try running fortls globally, or from python user scripts folder on Windows. - * - * @returns true if fortls found, false if not + * + * @returns true if fortls found, false if not */ private getLSPath(): boolean { const config = workspace.getConfiguration(EXTENSION_ID); @@ -311,29 +308,31 @@ export class FortlsClient { // if there's a user configured path to the executable, check if it's absolute if (configuredPath !== '') { if (!path.isAbsolute(configuredPath)) { - window.showErrorMessage("The path to fortls (fortran.fortls.path) must be absolute."); + window.showErrorMessage('The path to fortls (fortran.fortls.path) must be absolute.'); return false; } pathsToCheck.push(configuredPath); + } else { + // no user configured path => perform standard search for fortls - } else { // no user configured path => perform standard search for fortls - pathsToCheck.push('fortls'); - + // On Windows, `pip install fortls --user` installs fortls to the userbase\PythonXY\Scripts path, // so we want to look for it in this path as well. if (os.platform() == 'win32') { - const result = spawnSync('python', ['-c', 'import site; print(site.getusersitepackages())']); + const result = spawnSync('python', [ + '-c', + 'import site; print(site.getusersitepackages())', + ]); const userSitePackagesStr = result.stdout.toString().trim(); - + // check if the call above returned something, in case the site module in python ever changes... if (userSitePackagesStr) { const userScriptsPath = path.resolve(userSitePackagesStr, '../Scripts/fortls'); pathsToCheck.push(userScriptsPath); } } - } // try to run `fortls --version` for all the given paths @@ -400,7 +399,6 @@ export class FortlsClient { return results.stdout.toString().trim(); } - /** * Restart the language server */