VSCode build of the analysis engine by MikhailArkhipov · Pull Request #3957 · microsoft/PTVS

MikhailArkhipov · 2018-03-26T18:52:21Z

Separated common build setting to Common.Build.Core.settings
Ported Rest -> Markdown documentation converter from VSC extension (RestTextConverter) (will ports tests next)
Turned some structs to classes for VSC
Fixed some [handled] exceptions for easier debugging
Made sure document text change processing finishes before handling completion request
Fixed few Mac/Windows paths issues
Added discovery of so and dyLib Python compiled modules
Multi-platform build and packaging of the engine as nuget package for Windows, Mac OS and Linux

AlexanderSher · 2018-03-26T19:13:43Z

Python/Product/Analysis/AnalysisNetStandard.csproj

+        <PackageReference Include="NewtonSoft.Json" Version="10.0.1" />
+        <PackageReference Include="Python2" version="2.7.14" Condition="'$(OS)' == 'Windows_NT'" />
+        <PackageReference Include="Python" version="3.6.4" Condition="'$(OS)' == 'Windows_NT'" />
+    </ItemGroup> <ItemGroup>


AlexanderSher · 2018-03-26T19:15:43Z

Python/Product/Analysis/Infrastructure/ProcessHelper.cs

        public void Kill() {
            try {
-                _process?.Kill();
+                if (!_process.HasExited) {


_process?.Kill() has null check. Do we need it here as well?

@MikhailArkhipov Still want the null check here

zooba · 2018-03-26T21:03:34Z

Python/Product/Analysis/Infrastructure/UriExtensions.cs

+                // VS Code always sends /-based paths. Also, its file URL looks like
+                // file:///C:/foo which yields AbsolutePath as /C:/foo which is wrong.
+                // We need to clean this up.
+                var path = WebUtility.UrlDecode(uri.AbsolutePath).Replace('/', '\\');


We should use LocalPath to extract local paths, not AbsolutePath. (If there are path forms that do not work well with LocalPath, we can special case those. But the normal case works fine.)

OK. LocalPath still needs to be cleaned up. It looks like /f:/VSCP/src/test/pythonFiles

zooba · 2018-03-26T21:05:49Z

Python/Product/Analysis/Interpreter/Ast/AstScrapedPythonModule.cs

+                    try {
+                        exitCode = proc.Wait(60000);
+                    } catch (Exception ex) {
+                        fact.Log(TraceLevel.Error, $"ScrapeException {ex.Message}", proc.FileName, proc.Arguments);


Put the exception message as its own field. It makes processing the log file easier.

zooba · 2018-03-26T21:06:36Z

Python/Product/Analysis/Interpreter/InterpreterConfiguration.cs

-            } catch (ArgumentException) {
-                Version = new Version();
-            } catch (FormatException) {
+            } catch (Exception ex) when (ex is ArgumentException || ex is ArgumentNullException || ex is FormatException) {


Doesn't ex is ArgumentException imply that it is also ArgumentNullException?

zooba · 2018-03-26T21:07:49Z

Python/Product/Analysis/LanguageServer/RestTextConverter.cs

+
+// Based on https://github.com/Microsoft/vscode-python/blob/master/src/client/common/markdown/restTextConverter.ts
+
+namespace Microsoft.PythonTools.Analysis.LanguageServer {


Let's bump this up to Microsoft.PythonTools.Analysis. It isn't directly related to the language server.

zooba · 2018-03-26T21:09:03Z

Python/Product/Analysis/LanguageServer/Server.cs

        internal readonly ParseQueue _parseQueue;
        private readonly Dictionary<IDocument, VolatileCounter> _pendingParse;
        private readonly VolatileCounter _pendingAnalysisEnqueue;
+        private readonly RestTextConverter _textConverter = new RestTextConverter();


If this is not stateful, can we have a static instance? If it is stateful, we should instantiate it when needed (it looks cheap enough).

Technically it has states as it is converting. Not that it is called from multiple threads concurrently, but theoretically it could.

Okay, let's instantiate it for each conversion, just to be safe.

zooba · 2018-03-26T21:51:09Z

Python/Product/Analysis/LanguageServer/Server.cs

+                // It is called from DidChangeTextDocument which must fully finish
+                // since otherwise Complete() may come before the change is enqueued
+                // for processing and the completion list will be driven off the stale data.
+                cookieTask.ContinueWith(t => {


For the sake of indentation, can we extract this lambda to its own function ("FinishParse" or similar)? At least for the non-faulting case.

zooba · 2018-03-26T21:53:30Z

Python/Product/Analysis/LanguageServer/Server.cs

+        private PythonAst GetParseTree(IPythonProjectEntry entry, Uri documentUri, out int? version) {
+            version = null;
+            PythonAst tree = null;
+            var parse = entry.WaitForCurrentParse(_clientCaps?.python?.completionsTimeout ?? Timeout.Infinite, CancellationToken);


Completions may not be the only timeout we want here (e.g. we have a longer timeout for hover in VS today). This may need to be a parameter

zooba · 2018-03-26T21:54:20Z

Python/Product/Analysis/ModulePath.cs

            RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);
-        private static readonly Regex PythonBinaryRegex = new Regex(@"^(?!\d)(?<name>(\w|_)+)\.((\w|_|-)+?\.)?pyd$",
+        private static readonly Regex PythonBinaryRegex = new Regex(@"^(?!\d)(?<name>(\w|_)+)\.((\w|_|-)+?\.)?(pyd|so|dyLib)$",
            RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);


This (and below) regex ignore case - does that need to change for other OS?

I think so and dyLib cover OSX and Linux

zooba · 2018-03-26T21:55:47Z

Python/Product/Analysis/ProjectEntry.cs

            // package (checked by module name first, then sanity-checked by path), and register any that match.
            if (ModulePath.IsInitPyFile(FilePath)) {
-                string pathPrefix = Path.GetDirectoryName(FilePath) + "\\";
+                string pathPrefix = Path.GetDirectoryName(FilePath) + Path.DirectorySeparatorChar;


EnsureEndSeparator?

zooba · 2018-03-26T21:59:55Z

Python/Product/VSCode/Publish/Publish.csproj

+    <!-- Nuget packaging -->
+    <PropertyGroup>
+        <NuGetPackageRoot Condition=" '$(NuGetPackageRoot)' == '' ">$(UserProfile)\.nuget\packages\</NuGetPackageRoot>
+        <NugetPackageVersion>0.1.0</NugetPackageVersion>


Is this something we should promote to a build property?

zooba · 2018-03-26T22:36:38Z

And when we merge this, we should do a squash merge :)

AlexanderSher · 2018-03-27T19:04:44Z

Python/Product/VSCode/AnalysisVsc/LanguageServer.cs

+                    if (SafeGetProcessById(pid) == null) {
+                        Environment.Exit(0);
+                    }
+                    Thread.Sleep(5000);


await Task.Delay(5000)
And you can pass CancellationToken

AlexanderSher · 2018-03-27T19:06:01Z

Python/Product/VSCode/AnalysisVsc/LanguageServer.cs


        [JsonRpcMethod("textDocument/didChange")]
-        public Task DidChangeTextDocument(JToken token)
+        public void DidChangeTextDocument(JToken token) 


Is it a public API?

Sort of. Call comes from VSC process over RPC. StreamJsonRPC handles is async or plain depending on the signature.

I'm just thinking if this method can become async in future version of API

AlexanderSher · 2018-03-27T19:06:32Z

Python/Product/VSCode/AnalysisVsc/LanguageServer.cs

+
+#if WAIT_FOR_DEBUGGER
+            while (!System.Diagnostics.Debugger.IsAttached) {
+                Thread.Sleep(1000);


await Task.Delay(1000)

It's current in Program.cs which is not async

AlexanderSher · 2018-03-27T19:07:01Z

Python/Product/VSCode/AnalysisVsc/LanguageServer.cs

                diagnostics = e.diagnostics.ToArray()
            };
-            _rpc.InvokeAsync("textDocument/publishDiagnostics", parameters);
+            _vscode.NotifyWithParameterObjectAsync("textDocument/publishDiagnostics", parameters);


DoNotWait()

AlexanderSher · 2018-03-27T19:08:37Z

Python/Product/Analysis/ProjectEntry.cs


-        public IPythonParse WaitForCurrentParse(int timeout = -1) {
-            if (!_pendingParse.Wait(timeout)) {
+        public IPythonParse WaitForCurrentParse(int timeout, CancellationToken token) {


Can we make this method async?

I'd rather not. This is only ever used in contexts where blocking briefly (and it's only a parse, so it's brief) is the right action.

How many times GetCompletions can be called while we're waiting for a parse?

MikhailArkhipov · 2018-03-27T23:54:18Z

Python/Product/Analysis/PythonDb.Generator.targets

@@ -0,0 +1,74 @@
+<Project>


Unused, but keeping it around in case we want to pre-scrape on Mac

AlexanderSher · 2018-03-28T17:13:30Z

Python/Product/Analysis/LanguageServer/Messages.cs


    [Serializable]
-    public struct ShowMessageRequestParams {
+    public class ShowMessageRequestParams {


You have converted some of the structs into classes. Does it matter only for those several types?

Those which are important are the JSON serialization. Others can remain structs.

AlexanderSher · 2018-03-28T17:22:23Z

Some of the files have disappeared from the PR. Is it intentional?

zooba · 2018-03-28T21:00:32Z

Python/Product/Analysis/Infrastructure/TaskExtensions.cs


 namespace Microsoft.PythonTools.Analysis.Infrastructure {
-    static class TaskExtensions {
+    public static class TaskExtensions {


Does this need to be public? InternalsVisibleTo is preferable.

They are generally useful extensions but up to you. @AlexanderSher - your opinion?

Made internal

zooba · 2018-03-28T21:00:45Z

Python/Product/Analysis/Infrastructure/UriExtensions.cs

+using System.Net;
+
+namespace Microsoft.PythonTools.Analysis.Infrastructure {
+    public static class UriExtensions {


Internal is preferred.

zooba · 2018-03-28T21:03:38Z

Python/Product/Analysis/LanguageServer/Server.cs


        // For pending changes, we use alternate comparer that checks #fragment
        private readonly ConcurrentDictionary<Uri, List<DidChangeTextDocumentParams>> _pendingChanges;
+        private ManualResetEventSlim _documentChangeProcessingComplete = new ManualResetEventSlim(true);


readonly?

MikhailArkhipov · 2018-03-28T22:24:32Z

Branch was squished, had to restore some changes

MikhailArkhipov · 2018-03-28T22:25:04Z

See also microsoft/vscode-python#1231

zooba · 2018-03-29T00:12:13Z

Generally useful does not mean we make them public supported API. This assembly is public/supported now, so we only want things to be public when it’s deliberate.

MikhailArkhipov · 2018-03-29T23:56:07Z

OK, made internal

MikhailArkhipov · 2018-03-29T23:58:24Z

MonitorParentProcess is mostly for Mac where dotnet does not exit when launching process dies.

AlexanderSher reviewed Mar 26, 2018

View reviewed changes

zooba requested changes Mar 26, 2018

View reviewed changes

AlexanderSher reviewed Mar 27, 2018

View reviewed changes

MikhailArkhipov added 21 commits March 27, 2018 16:38

Place versions in a separate file

ec13b3e

Multi-platform packaging

6606c0d

Diagnotcis, Uri transfer, root folder init

b48eb24

Pass interpreter

8510ad1

License update to Apache and Python files publishing

80a5a09

Fix Mac paths

b097c3e

Handle process termination better

c5b2b86

Fix path handling

4920cc4

Fix desktop build

085aedc

Undo change

2a365a8

Remove old file

a648683

Dot trigger

a90b49b

Mac path fixes

fc88556

Fix Mac/Linux extensionless executable handling

9160065

Add Mac cache handling

2aa9630

Include DB in publishing

bbc4817

Reference python for scraping

9012941

Python on Mac

156df57

Fix target names

29211ed

VSIX

d310c9d

Python3 path

85c65a6

MikhailArkhipov added 2 commits March 27, 2018 16:41

Fix path

a0c012a

PR feedback

8c4b21c

MikhailArkhipov commented Mar 27, 2018

View reviewed changes

MikhailArkhipov added 2 commits March 27, 2018 16:56

PR feedback

afc418b

Remove scraping

f1a5be5

AlexanderSher reviewed Mar 28, 2018

View reviewed changes

PT feedback

155a5c2

Restore changes lost

dc7c511

zooba reviewed Mar 28, 2018

View reviewed changes

MikhailArkhipov added 3 commits March 28, 2018 15:28

PR feedback

4f9e15d

Fix output path for lab

4cce969

Restore lost changes

fb8cf0e

Mikhail Arkhipov added 5 commits March 28, 2018 22:22

Some extra assert for Mac

bcff783

Monitor parent process

3fc3897

Reflect interface change

a4c46db

Null check

5bd270f

Make TaskExtensions internal

451c282

Add back assemblyinfo

0f341f5

MikhailArkhipov added 2 commits March 29, 2018 17:09

Fix which folder we publish

6496a26

Flatten a bit folder structure

e8f015d

zooba approved these changes Apr 2, 2018

View reviewed changes

MikhailArkhipov merged commit 9ffbf76 into microsoft:master Apr 2, 2018


		// Based on https://github.com/Microsoft/vscode-python/blob/master/src/client/common/markdown/restTextConverter.ts

		namespace Microsoft.PythonTools.Analysis.LanguageServer {

Conversation

MikhailArkhipov commented Mar 26, 2018

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!

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!

zooba commented Mar 26, 2018

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!

AlexanderSher commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment