Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upVS Python analysis engine integration #1231
Conversation
MikhailArkhipov
added some commits
Feb 13, 2018
MikhailArkhipov
referenced this pull request
Mar 28, 2018
Merged
VSCode build of the analysis engine #3957
MikhailArkhipov
added some commits
Mar 28, 2018
DonJayamanne
requested changes
Mar 30, 2018
| }); | ||
|
|
||
| let firstResponse = true; | ||
| https.get(uri, (response) => { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| let firstResponse = true; | ||
| https.get(uri, (response) => { | ||
| this.checkHttpResponse(response); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| deferred.resolve(); | ||
| }).on('error', (err) => { | ||
| tempFile.cleanupCallback(); | ||
| this.handleError(`Unable to download Python Analysis Engine. Error ${err}`); |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
| try { | ||
| await this.verifyDownload(localTempFilePath); | ||
| await this.unpackArchive(context.extensionPath, localTempFilePath); | ||
| } finally { |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
| 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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
| @@ -55,4 +66,22 @@ export class ConfigurationService implements IConfigurationService { | |||
| } while (retries < 20); | |||
| } | |||
| } | |||
|
|
|||
| private async checkDotNet(): Promise<boolean> { | |||
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
| 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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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)
MikhailArkhipov commentedMar 28, 2018
•
edited
Replaces #1144
Fixes #1101
Fixes #1087
Fixes #1063
Fixes #1051
Fixes #1004
Fixes #998
Fixes #408
Fixes #82
This pull request: