Fix linting under conda run (#18390)

karthiknadig · web-flow · commit 4758b3536bbd · 2022-01-27T18:28:17.000-08:00
* Fix linting under conda run.

* Fix tests

* Fix for 2.7
diff --git a/news/2 Fixes/18364.md b/news/2 Fixes/18364.md
@@ -0,0 +1 @@
+Ensure linting works under `conda run` (work-around for https://github.com/conda/conda/issues/10972).
diff --git a/pythonFiles/linter.py b/pythonFiles/linter.py
@@ -0,0 +1,56 @@
+import subprocess
+import sys
+
+
+linter_settings = {
+    "pylint": {
+        "args": ["--reports=n", "--output-format=json"],
+    },
+    "flake8": {
+        "args": ["--format", "%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s"],
+    },
+    "bandit": {
+        "args": [
+            "-f",
+            "custom",
+            "--msg-template",
+            "{line},{col},{severity},{test_id}:{msg}",
+            "-n",
+            "-1",
+        ],
+    },
+    "mypy": {"args": []},
+    "prospector": {
+        "args": ["--absolute-paths", "--output-format=json"],
+    },
+    "pycodestyle": {
+        "args": ["--format", "%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s"],
+    },
+    "pydocstyle": {
+        "args": [],
+    },
+    "pylama": {"args": ["--format=parsable"]},
+}
+
+
+def main():
+    invoke = sys.argv[1]
+    if invoke == "-m":
+        linter = sys.argv[2]
+        args = (
+            [sys.executable, "-m", linter]
+            + linter_settings[linter]["args"]
+            + sys.argv[3:]
+        )
+    else:
+        linter = sys.argv[2]
+        args = [sys.argv[3]] + linter_settings[linter]["args"] + sys.argv[4:]
+
+    if hasattr(subprocess, "run"):
+        subprocess.run(args, encoding="utf-8", stdout=sys.stdout, stderr=sys.stderr)
+    else:
+        subprocess.call(args, stdout=sys.stdout, stderr=sys.stderr)
+
+
+if __name__ == "__main__":
+    main()
diff --git a/src/client/common/process/internal/scripts/index.ts b/src/client/common/process/internal/scripts/index.ts
@@ -144,3 +144,10 @@ export function tensorboardLauncher(args: string[]): string[] {
     const script = path.join(SCRIPTS_DIR, 'tensorboard_launcher.py');
     return [script, ...args];
 }
+
+// linter.py
+
+export function linterScript(): string {
+    const script = path.join(SCRIPTS_DIR, 'linter.py');
+    return script;
+}
diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts
@@ -191,5 +191,6 @@ function createPythonService(
         execModuleObservable: (m, a, o) => procs.execModuleObservable(m, a, o),
         exec: (a, o) => procs.exec(a, o),
         execModule: (m, a, o) => procs.execModule(m, a, o),
+        execForLinter: (m, a, o) => procs.execForLinter(m, a, o),
     };
 }
diff --git a/src/client/common/process/pythonProcess.ts b/src/client/common/process/pythonProcess.ts
@@ -64,6 +64,26 @@ class PythonProcessService {
 
         return result;
     }
+
+    public async execForLinter(
+        moduleName: string,
+        args: string[],
+        options: SpawnOptions,
+    ): Promise<ExecutionResult<string>> {
+        const opts: SpawnOptions = { ...options };
+        const executable = this.deps.getExecutionInfo(args);
+        const result = await this.deps.exec(executable.command, executable.args, opts);
+
+        // If a module is not installed we'll have something in stderr.
+        if (moduleName && ErrorUtils.outputHasModuleNotInstalledError(moduleName, result.stderr)) {
+            const isInstalled = await this.deps.isModuleInstalled(moduleName);
+            if (!isInstalled) {
+                throw new ModuleNotInstalledError(moduleName);
+            }
+        }
+
+        return result;
+    }
 }
 
 export function createPythonProcessService(
diff --git a/src/client/common/process/pythonToolService.ts b/src/client/common/process/pythonToolService.ts
@@ -57,4 +57,23 @@ export class PythonToolExecutionService implements IPythonToolExecutionService {
             return processService.exec(executionInfo.execPath!, executionInfo.args, { ...options });
         }
     }
+
+    public async execForLinter(
+        executionInfo: ExecutionInfo,
+        options: SpawnOptions,
+        resource: Uri,
+    ): Promise<ExecutionResult<string>> {
+        if (options.env) {
+            throw new Error('Environment variables are not supported');
+        }
+        const pythonExecutionService = await this.serviceContainer
+            .get<IPythonExecutionFactory>(IPythonExecutionFactory)
+            .create({ resource });
+
+        if (executionInfo.execPath) {
+            return pythonExecutionService.exec(executionInfo.args, options);
+        }
+
+        return pythonExecutionService.execForLinter(executionInfo.moduleName!, executionInfo.args, options);
+    }
 }
diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts
@@ -100,6 +100,7 @@ export interface IPythonExecutionService {
 
     exec(args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;
     execModule(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;
+    execForLinter(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;
 }
 
 export class StdErrError extends Error {
@@ -117,4 +118,5 @@ export interface IPythonToolExecutionService {
         resource: Uri,
     ): Promise<ObservableExecutionResult<string>>;
     exec(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>>;
+    execForLinter(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>>;
 }
diff --git a/src/client/linters/bandit.ts b/src/client/linters/bandit.ts
@@ -26,20 +26,7 @@ export class Bandit extends BaseLinter {
 
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
         // View all errors in bandit <= 1.5.1 (https://github.com/PyCQA/bandit/issues/371)
-        const messages = await this.run(
-            [
-                '-f',
-                'custom',
-                '--msg-template',
-                '{line},{col},{severity},{test_id}:{msg}',
-                '-n',
-                '-1',
-                document.uri.fsPath,
-            ],
-            document,
-            cancellation,
-            BANDIT_REGEX,
-        );
+        const messages = await this.run([document.uri.fsPath], document, cancellation, BANDIT_REGEX);
 
         messages.forEach((msg) => {
             msg.severity = severityMapping[msg.type];
diff --git a/src/client/linters/baseLinter.ts b/src/client/linters/baseLinter.ts
@@ -165,7 +165,7 @@ export abstract class BaseLinter implements ILinter {
             IPythonToolExecutionService,
         );
         try {
-            const result = await pythonToolsExecutionService.exec(
+            const result = await pythonToolsExecutionService.execForLinter(
                 executionInfo,
                 { cwd, token: cancellation, mergeStdOutErr: false },
                 document.uri,
diff --git a/src/client/linters/flake8.ts b/src/client/linters/flake8.ts
@@ -13,11 +13,7 @@ export class Flake8 extends BaseLinter {
     }
 
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
-        const messages = await this.run(
-            ['--format= %(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath],
-            document,
-            cancellation,
-        );
+        const messages = await this.run([document.uri.fsPath], document, cancellation);
         messages.forEach((msg) => {
             msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.flake8CategorySeverity);
             // flake8 uses 0th line for some file-wide problems
diff --git a/src/client/linters/linterInfo.ts b/src/client/linters/linterInfo.ts
@@ -3,6 +3,7 @@
 
 import * as path from 'path';
 import { Uri } from 'vscode';
+import { linterScript } from '../common/process/internal/scripts';
 import { ExecutionInfo, IConfigurationService, ILintingSettings, Product } from '../common/types';
 import { ILinterInfo, LinterId } from './types';
 
@@ -74,13 +75,20 @@ export class LinterInfo implements ILinterInfo {
     public getExecutionInfo(customArgs: string[], resource?: Uri): ExecutionInfo {
         const execPath = this.pathName(resource);
         const args = this.linterArgs(resource).concat(customArgs);
-        let moduleName: string | undefined;
-
-        // If path information is not available, then treat it as a module,
+        const script = linterScript();
         if (path.basename(execPath) === execPath) {
-            moduleName = execPath;
+            return {
+                execPath: undefined,
+                args: [script, '-m', this.id, ...args],
+                product: this.product,
+                moduleName: execPath,
+            };
         }
-
-        return { execPath, moduleName, args, product: this.product };
+        return {
+            execPath,
+            moduleName: this.id,
+            args: [script, '-p', this.id, execPath, ...args],
+            product: this.product,
+        };
     }
 }
diff --git a/src/client/linters/prospector.ts b/src/client/linters/prospector.ts
@@ -32,7 +32,7 @@ export class Prospector extends BaseLinter {
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
         const cwd = this.getWorkingDirectoryPath(document);
         const relativePath = path.relative(cwd, document.uri.fsPath);
-        return this.run(['--absolute-paths', '--output-format=json', relativePath], document, cancellation);
+        return this.run([relativePath], document, cancellation);
     }
 
     protected async parseMessages(
diff --git a/src/client/linters/pycodestyle.ts b/src/client/linters/pycodestyle.ts
@@ -13,11 +13,7 @@ export class Pycodestyle extends BaseLinter {
     }
 
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
-        const messages = await this.run(
-            ['--format= %(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath],
-            document,
-            cancellation,
-        );
+        const messages = await this.run([document.uri.fsPath], document, cancellation);
         messages.forEach((msg) => {
             msg.severity = this.parseMessagesSeverity(
                 msg.type,
diff --git a/src/client/linters/pylama.ts b/src/client/linters/pylama.ts
@@ -15,7 +15,7 @@ export class PyLama extends BaseLinter {
     }
 
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
-        const messages = await this.run(['--format=parsable', document.uri.fsPath], document, cancellation, REGEX);
+        const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX);
         // All messages in pylama are treated as warnings for now.
         messages.forEach((msg) => {
             msg.severity = LintMessageSeverity.Warning;
diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts
@@ -27,12 +27,11 @@ export class Pylint extends BaseLinter {
     protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
         const { uri } = document;
         const settings = this.configService.getSettings(uri);
-        const args = ['--reports=n', '--output-format=json', uri.fsPath];
+        const args = [uri.fsPath];
         const messages = await this.run(args, document, cancellation);
         messages.forEach((msg) => {
             msg.severity = this.parseMessagesSeverity(msg.type, settings.linting.pylintCategorySeverity);
         });
-
         return messages;
     }
 
diff --git a/src/test/linters/lint.args.test.ts b/src/test/linters/lint.args.test.ts
@@ -142,25 +142,25 @@ suite('Linting - Arguments', () => {
                     }
                     test('Flake8', async () => {
                         const linter = new Flake8(serviceContainer);
-                        const expectedArgs = ['--format= %(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', fileUri.fsPath];
+                        const expectedArgs = [fileUri.fsPath];
                         await testLinter(linter, expectedArgs);
                     });
                     test('Pycodestyle', async () => {
                         const linter = new Pycodestyle(serviceContainer);
-                        const expectedArgs = ['--format= %(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', fileUri.fsPath];
+                        const expectedArgs = [fileUri.fsPath];
                         await testLinter(linter, expectedArgs);
                     });
                     test('Prospector', async () => {
                         const linter = new Prospector(serviceContainer);
                         const expectedPath = workspaceUri
                             ? fileUri.fsPath.substring(workspaceUri.length + 2)
                             : path.basename(fileUri.fsPath);
-                        const expectedArgs = ['--absolute-paths', '--output-format=json', expectedPath];
+                        const expectedArgs = [expectedPath];
                         await testLinter(linter, expectedArgs);
                     });
                     test('Pylama', async () => {
                         const linter = new PyLama(serviceContainer);
-                        const expectedArgs = ['--format=parsable', fileUri.fsPath];
+                        const expectedArgs = [fileUri.fsPath];
                         await testLinter(linter, expectedArgs);
                     });
                     test('MyPy', async () => {
@@ -175,29 +175,12 @@ suite('Linting - Arguments', () => {
                     });
                     test('Pylint', async () => {
                         const linter = new Pylint(serviceContainer);
-                        document.setup((d) => d.uri).returns(() => fileUri);
-
-                        let invoked = false;
-                        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-                        (linter as any).run = (args: any[]) => {
-                            expect(args[args.length - 1]).to.equal(fileUri.fsPath);
-                            invoked = true;
-                            return Promise.resolve([]);
-                        };
-                        await linter.lint(document.object, cancellationToken);
-                        expect(invoked).to.be.equal(true, 'method not invoked');
+                        const expectedArgs = [fileUri.fsPath];
+                        await testLinter(linter, expectedArgs);
                     });
                     test('Bandit', async () => {
                         const linter = new Bandit(serviceContainer);
-                        const expectedArgs = [
-                            '-f',
-                            'custom',
-                            '--msg-template',
-                            '{line},{col},{severity},{test_id}:{msg}',
-                            '-n',
-                            '-1',
-                            fileUri.fsPath,
-                        ];
+                        const expectedArgs = [fileUri.fsPath];
                         await testLinter(linter, expectedArgs);
                     });
                 },
diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts
@@ -759,7 +759,7 @@ suite('Linting Functional Tests', () => {
         sinon.restore();
     });
 
-    const pythonPath = childProcess.execSync(`${PYTHON_PATH} -c "import sys;print(sys.executable)"`);
+    const pythonPath = childProcess.execSync(`"${PYTHON_PATH}" -c "import sys;print(sys.executable)"`);
 
     console.log(`Testing linter with python ${pythonPath}`);
 
diff --git a/src/test/linters/lint.multilinter.test.ts b/src/test/linters/lint.multilinter.test.ts
@@ -40,7 +40,7 @@ class MockPythonToolExecService extends PythonToolExecutionService {
 ]`;
 
     // Depending on moduleName being exec'd, return the appropriate sample.
-    public async exec(
+    public async execForLinter(
         executionInfo: ExecutionInfo,
         _options: SpawnOptions,
         _resource: Uri,
diff --git a/src/test/linters/lint.unit.test.ts b/src/test/linters/lint.unit.test.ts
@@ -645,7 +645,7 @@ class TestFixture extends BaseTestFixture {
 
     public setStdout(stdout: string) {
         this.pythonToolExecService
-            .setup((s) => s.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .setup((s) => s.execForLinter(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
             .returns(() => Promise.resolve({ stdout }));
     }
 }
diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts
@@ -130,7 +130,7 @@ suite('Linting - Pylint', () => {
             ']',
         ].join(os.EOL);
         execService
-            .setup((x) => x.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .setup((x) => x.execForLinter(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
             .returns(() => Promise.resolve({ stdout: linterOutput, stderr: '' }));
 
         const lintSettings = new MockLintingSettings();
diff --git a/src/test/linters/pylint.unit.test.ts b/src/test/linters/pylint.unit.test.ts
@@ -26,7 +26,7 @@ suite('Pylint - Function runLinter()', () => {
     const doc = {
         uri: vscode.Uri.file('path/to/doc'),
     };
-    const args = ['--reports=n', '--output-format=json', doc.uri.fsPath];
+    const args = [doc.uri.fsPath];
     class PylintTest extends Pylint {
         // eslint-disable-next-line class-methods-use-this
         public async run(
diff --git a/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts b/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts
@@ -216,6 +216,7 @@ suite('Terminal - Django Shell Code Execution', () => {
             execModuleObservable: procs.execModuleObservable,
             exec: procs.exec,
             execModule: procs.execModule,
+            execForLinter: procs.execForLinter,
         };
         const expectedTerminalArgs = [...terminalArgs, 'manage.py', 'shell'];
         pythonExecutionFactory
diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts
@@ -367,6 +367,7 @@ suite('Terminal - Code Execution', () => {
                     execModuleObservable: procs.execModuleObservable,
                     exec: procs.exec,
                     execModule: procs.execModule,
+                    execForLinter: procs.execForLinter,
                 };
                 pythonExecutionFactory
                     .setup((p) =>
@@ -479,6 +480,7 @@ suite('Terminal - Code Execution', () => {
                     execModuleObservable: procs.execModuleObservable,
                     exec: procs.exec,
                     execModule: procs.execModule,
+                    execForLinter: procs.execForLinter,
                 };
                 pythonExecutionFactory
                     .setup((p) =>

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Ensure linting works under `conda run` (work-around for https://github.com/conda/conda/issues/10972).
Original file line number	Diff line number	Diff line change
`@@ -191,5 +191,6 @@ function createPythonService(`
`191`	`191`	`execModuleObservable: (m, a, o) => procs.execModuleObservable(m, a, o),`
`192`	`192`	`exec: (a, o) => procs.exec(a, o),`
`193`	`193`	`execModule: (m, a, o) => procs.execModule(m, a, o),`
	`194`	`+ execForLinter: (m, a, o) => procs.execForLinter(m, a, o),`
`194`	`195`	`};`
`195`	`196`	`}`
Original file line number	Diff line number	Diff line change
`@@ -100,6 +100,7 @@ export interface IPythonExecutionService {`
`100`	`100`
`101`	`101`	`exec(args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;`
`102`	`102`	`execModule(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;`
	`103`	`+ execForLinter(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>>;`
`103`	`104`	`}`
`104`	`105`
`105`	`106`	`export class StdErrError extends Error {`
`@@ -117,4 +118,5 @@ export interface IPythonToolExecutionService {`
`117`	`118`	`resource: Uri,`
`118`	`119`	`): Promise<ObservableExecutionResult<string>>;`
`119`	`120`	`exec(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>>;`
	`121`	`+ execForLinter(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>>;`
`120`	`122`	`}`