Skip to content
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

VS Python analysis engine integration #1231

Merged
merged 70 commits into from Mar 30, 2018

Conversation

Projects
None yet
2 participants
@MikhailArkhipov
Copy link
Member

MikhailArkhipov commented Mar 28, 2018

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 #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 Microsoft/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

MikhailArkhipov added some commits Mar 28, 2018

});

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

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

please use npm package like request-package for this.

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

Done


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

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

Return value of checkHttpResponse not checked

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

N/A as replaced by request-package

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

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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);
        });

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

resolve will be done by VSC progress. Added reject

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

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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)

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

Done

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.');

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

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.

@@ -32,6 +39,10 @@ export class ConfigurationService implements IConfigurationService {
return process.env.VSC_PYTHON_CI_TEST === '1';
}

public async checkDependencies(): Promise<boolean> {

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

Done

@@ -55,4 +66,22 @@ export class ConfigurationService implements IConfigurationService {
} while (retries < 20);
}
}

private async checkDotNet(): Promise<boolean> {

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

Removed

test_script:
- yarn run clean
- yarn run vscode:prepublish
- yarn run testDebugger --silent
- yarn run testSingleWorkspace --silent
- yarn run testMultiWorkspace --silent
# - yarn run testAnalysisEngine --silent

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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.

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

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.


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

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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",

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

Might want to store the hashes in here too.

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

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.


// 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 = '';

This comment has been minimized.

@DonJayamanne

DonJayamanne Mar 30, 2018

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)

This comment has been minimized.

@MikhailArkhipov

MikhailArkhipov Mar 30, 2018

Author Member

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

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

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.