VS Python analysis engine integration by MikhailArkhipov · Pull Request #1231 · microsoft/vscode-python

MikhailArkhipov · 2018-03-28T21:43:37Z

Replaces #1144

Fixes #1101
Fixes #1087
Fixes #1063
Fixes #1051
Fixes #1004
Fixes #998
Fixes #408
Fixes #82

VSC part, not active for general audience yet. (Needs integration in the PTVS repo first).
Not all functionality is exactly like Jedi, see Meta for tracking Language Server issues #1006
Tests are not active on Appveyor
There is a task to run tests with the VS Analysis engine (couple of signature tests can fail b/c of Generate SignatureAnalysis without using GetExpressionRange PTVS#3171.
Jedi and VS Analysis engine are activated via python.jediEnabled setting (default true)
Tests pass on Mac except issue with import *
Activation is separated into 'classic' and 'analysis`
Not tested on Linux
.NET Core 2.x SDK or at least runtime is required for running VS Python Engine
Building the engine will be documented separately
There is nothing to add to news just yet

This pull request:

Has a title summarizes what is changing
Includes a news entry file
Has unit tests & code coverage is not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS

This reverts commit e240c3f.

This reverts commit af573be.

…nto vsc

DonJayamanne · 2018-03-30T15:26:59Z

src/client/activation/downloader.ts

+        });
+
+        let firstResponse = true;
+        https.get(uri, (response) => {


please use npm package like request-package for this.

DonJayamanne · 2018-03-30T15:28:56Z

src/client/activation/downloader.ts

+
+        let firstResponse = true;
+        https.get(uri, (response) => {
+            this.checkHttpResponse(response);


Return value of checkHttpResponse not checked

N/A as replaced by request-package

DonJayamanne · 2018-03-30T15:30:34Z

src/client/activation/downloader.ts

+            deferred.resolve();
+        }).on('error', (err) => {
+            tempFile.cleanupCallback();
+            this.handleError(`Unable to download Python Analysis Engine. Error ${err}`);


Errors thrown by this.handleError will not be bubbled up.
You're better off changing as follows:

fileStream.on('finish', () => { fileStream.close(); deferred.resolve(); }).on('error', (err) => { tempFile.cleanupCallback(); deferred.reject(err); });

resolve will be done by VSC progress. Added reject

DonJayamanne · 2018-03-30T15:34:44Z

src/client/activation/downloader.ts

+        try {
+            await this.verifyDownload(localTempFilePath);
+            await this.unpackArchive(context.extensionPath, localTempFilePath);
+        } finally {


better to add a catch here and then invoke this.handleError
This way, all errors are handled and logged in one place (you won't need to call this.handleError in a number of places)

DonJayamanne · 2018-03-30T15:35:32Z

src/client/activation/downloader.ts

+        this.output.append('Verifying download... ');
+        const verifier = new HashVerifier();
+        if (!await verifier.verifyHash(filePath, this.platformData.getExpectedHash())) {
+            this.handleError('Hash of the downloaded file does not match.');


better to just throw an exception here, let calling code log the error.

It is more for the output formatting. It starts with Verifying download... and I'd like it to look as
Verifying download... valid on the same line and on different lines if there is an error. Same as in other cases: download and unpack.

DonJayamanne · 2018-03-30T15:40:30Z

src/client/common/configuration/service.ts

        return process.env.VSC_PYTHON_CI_TEST === '1';
    }

+    public async checkDependencies(): Promise<boolean> {


Please move to some other place. Checking dependencies is not the role of a Configuration service.

DonJayamanne · 2018-03-30T15:41:01Z

src/client/common/configuration/service.ts

        }
    }
+
+    private async checkDotNet(): Promise<boolean> {


Please move out of this class ( Checking dependencies is not the role of a Configuration service)

DonJayamanne · 2018-03-30T15:44:44Z

appveyor.yml

  - yarn run testDebugger --silent
  - yarn run testSingleWorkspace --silent
  - yarn run testMultiWorkspace --silent
+  # - yarn run testAnalysisEngine --silent


Can't remember, but please could you confirm that the tests only run on Appveyor and not on Travis. If this is true, then we might want to create an issue for this, to ensure we try to get this resolved.

They currently only on Appveyor b/c of setup issues. We need to discuss if we want a) install .NET Runtime on Travis or b) rely on some other means. (b) may work for us as we download from Azure but third party contributors cant.

DonJayamanne · 2018-03-30T15:51:47Z

src/client/activation/downloader.ts

+
+const downloadUriPrefix = 'https://pvsc.blob.core.windows.net/python-analysis';
+const downloadBaseFileName = 'python-analysis-vscode';
+const downloadVersion = '0.1.0';


We might want to store the version info, in package.json.
This way its easier when we make upgrades (all dependencies and versions are stored and listed in a central location - package.json).
E.g. package.json:

}, "dotNetDepedencies":{ "python-analysis-vscode":"0.1.0" }, "dependencies": { "arch": "^2.1.0",

Might want to store the hashes in here too.

Hashes are generated in the build. And we don't want users to be able to modify them (or any malicious code running under user creds). Also, don't want users to be able to change engine version or URLs.

DonJayamanne · 2018-03-30T15:55:51Z

src/client/activation/analysisEngineHashes.ts

+
+// This file will be replaced by a generated one during the release build
+// with actual hashes of the uploaded packages.
+export const analysis_engine_win_x86_sha512 = '';


Wouldn't it be better to generate some json file to store this information, rather than generating a .js. Feels better to store in a json file (possibly package.json or other).

The goal is to ensure all depedencies (file names, versions, etc are stored in a central location such as package.json or as in .net, we'd stored these in the project file)

No. We don't want it visible and easily modifiable.

…into vsc

…into fidd

MikhailArkhipov added 30 commits March 27, 2018 14:58

Undo changes

a764bc9

Test fixes

9d1b2cc

Increase timeout

a91291a

Remove double event listening

bf266af

Remove test

7bc6bd6

Revert "Remove test"

8ce8b48

This reverts commit e240c3f.

Revert "Remove double event listening"

e3a549e

This reverts commit af573be.

Undo changes

c879aa6

Test fixes

716968f

.NET Core check

63a1385

Better find dotnet

7ea7717

Fix pip test

75be0da

Linting tests

2701768

Undo accidental changes

a22e705

Add clone and build PTVS

5441644

Appveyor PTVS build

97175e1

Fix slashes

1e97e6a

Enable build

86ac269

Try absolute path

7b4e73c

Fix xcopy switch

bd1fd77

Activate Analysis Engine test on Appveyor

72b91f1

Temporary only run new tests

e32320b

Disable PEP hint tests

55bb349

Test fix

3826ffa

Disable appveyor build and tests for PTVS for now

b62ba25

Remove analysis engine test from the set

a13770b

Remove VS image for now

ac646c8

Build/sign VSXI project

f754812

Run vsce from cmd

826dcee

Rename

8cb0c44

Add Mac/Linux info

5c87964

MikhailArkhipov mentioned this pull request Mar 28, 2018

VSCode build of the analysis engine microsoft/PTVS#3957

Merged

MikhailArkhipov added 5 commits March 28, 2018 15:36

Disable csproj build

68e3384

Add unzip to dependencies

abb3daa

Minor fixes to doc

78fd495

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

9b9b51c

…nto vsc

Change setting type to bool

84c8b0f

DonJayamanne requested changes Mar 30, 2018

View reviewed changes

MikhailArkhipov added 5 commits March 30, 2018 11:15

Report progress on status bar

7761a15

Simplify

240d26f

CR feedback

736846a

Merge branch 'vsc' of https://github.com/MikhailArkhipov/vscode-python …

1c73143

…into vsc

Fix launching fx-independent code on Mac/Linux

f03c18a

DonJayamanne approved these changes Mar 30, 2018

View reviewed changes

MikhailArkhipov added 3 commits March 30, 2018 13:05

Add title

70ce69b

Proper download when .NET is missing

c33ae8d

Merge branch 'vsc' of https://github.com/MikhailArkhipov/vscode-python …

be268fb

…into fidd

MikhailArkhipov merged commit 962c850 into microsoft:master Mar 30, 2018

This was referenced Apr 10, 2018

Create CI test (integration test) that loads the extension and confirms it works #1359

Closed

Add request npm package to extension #1360

Closed

Update npm packages #1226

Closed

brettcannon mentioned this pull request Apr 16, 2018

Cannot load extension with 'natives' as an indirect dependency #1404

Closed

marnovo mentioned this pull request May 11, 2018

Update Python linting docs w/ info on linting on change microsoft/vscode-docs#1613

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Conversation

MikhailArkhipov commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikhailArkhipov Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MikhailArkhipov commented Mar 28, 2018 •

edited

Loading

MikhailArkhipov Mar 30, 2018 •

edited

Loading