Reimplement debugger acquisition to use Package Manager #873

chuckries · 2016-11-04T21:09:40Z

Remove all code related to calling dotnet restore/dotnet publish
Remove coreclr-debug directory
Add debugger runtime packages to package manifest
Add an install.Begin file to the package manager so we can detect that
it is in progress
Rework the debugger proxy so that it understands the package manager
Add knowledge of the dotnet cli on the path to the debugger proxy

TODO:

Lots and lots of testing
Offline package creation

1. Remove all code related to calling dotnet restore/dotnet publish 2. Remove coreclr-debug directory 3. Add debugger runtime packages to package manifest 4. Add an install.Begin file to the package manager so we can detect that it is in progress 5. Rework the debugger proxy so that it understands the package manager 6. Add knowledge of the dotnet cli on the path to the debugger proxy TODO: 1. Lots and lots of testing 2. Offline package creation

chuckries · 2016-11-04T21:10:05Z

@gregg-miskelly @wesrupert @DustinCampbell

I need to test this all over the place and offline package creation is still incoming.

DustinCampbell · 2016-11-04T21:20:00Z

package.json

+      ]
+    },
+    {
+      "description": ".NET Core Debugger (OSX / x64)",


"macOS"? #Pending

I was basing it off omnisharp: "description": "OmniSharp (.NET Core - OSX / x64)",
I'll change it in both places. #Closed

fine be me. I was just working on my own local branch when I wrote "macOS". I don't much care. 😄

DustinCampbell · 2016-11-04T21:20:21Z

src/common.ts

-export function getBinPath() {
-    return path.resolve(getExtensionPath(), "bin");
-}
-


I'll be using this again in my next PR. #Pending

I'll put it back. #Closed

DustinCampbell · 2016-11-04T21:21:08Z

src/common.ts

+    return new Promise<void>((resolve, reject) => {
+        fs.writeFile(getInstallFilePath(type), '', err => {
+            if (err) {
+                reject(err);


I'd return here so that it doesn't carry on to the resolve below. #Pending

DustinCampbell · 2016-11-04T21:23:28Z

src/coreclr-debug/install.ts


 import { CoreClrDebugUtil } from './util';
 import * as fs from 'fs';
 import * as path from 'path';
-import * as fs_extra from 'fs-extra-promise';


is this the last reference? Can we remove the dependency from package.json?

not quite, looks like this is used in assets.ts and features/commands.ts

In reply to: 86635042 [](ancestors = 86635042)

DustinCampbell · 2016-11-04T21:25:19Z

src/coreclr-debug/util.ts

+            return dotnetInfo;
+        });
+    }
+
    public spawnChildProcess(process: string, args: string[], workingDirectory: string, onStdout?: (data: Buffer) => void, onStderr?: (data: Buffer) => void): Promise<void> {


Does spawnChildProcess in common.ts work for this? #Pending

DustinCampbell · 2016-11-04T21:30:36Z

src/coreclr-debug/activate.ts

-            moreErrors = error.hasMoreErrors ? 'true' : 'false';
-        }).then(() => {
-            // log telemetry and delete install begin file
-            logTelemetry('Acquisition', {


Should we move this into main.activate()?

There is still an open question surrounding how we want to handle telemetry now that we have moved to the package manager.

In reply to: 86636070 [](ancestors = 86636070)

gregg-miskelly · 2016-11-04T23:13:21Z

src/main.ts

+        })
+        .then(() => {
+            installationStage = 'deleteBeginFile';
+            return util.deleteInstallFile(util.InstallFileType.Begin)


Should this be in the catch block (or using the temp file stuff) so that we delete the begin file no matter what? #Pending

gregg-miskelly · 2016-11-04T23:30:43Z

src/coreclr-debug/activate.ts

+        // We have been activated but it looks like our package was not installed. This is bad.
+        logger.appendLine("[ERROR]: C# Extension failed to install the debugger package");
+        showInstallErrorMessage();
+    } else if (!CoreClrDebugUtil.existsSync(_util.installCompleteFilePath())) {


Since we also have an install complete file for the package manager, I wonder if we should rename some part of this to make the distinction more clear (ex: debugUtil instead of util, or debuggerInstallCompleteFilePath...) #Pending

gregg-miskelly · 2016-11-04T23:32:22Z

src/coreclr-debug/util.ts

+    // This function checks for the presence of dotnet on the path and ensures the Version
+    // is new enough for us. 
+    // Returns: a promise that returns a DotnetInfo class
+    // Throws: An CotNetCliError() from the return promise if either dotnet does not exist or is too old. 


CotNetCliError is wrong #Pending

chuckries · 2016-11-07T22:57:03Z

@DustinCampbell I think this is ready to be merged. Can you take another look?

DustinCampbell · 2016-11-04T21:47:47Z

package.json

+      ]
+    },
+    {
+      "description": ".NET Core Debugger (OSX / x64)",


fine be me. I was just working on my own local branch when I wrote "macOS". I don't much care. 😄

DustinCampbell · 2016-11-07T23:37:19Z

package.json

@@ -91,7 +91,7 @@
      ]
    },
    {
-      "description": "OmniSharp (.NET Core - OSX / x64)",
+      "description": "OmniSharp (.NET Core - macOS / x64)",


dnfclas added the cla-already-signed label Nov 4, 2016

DustinCampbell requested changes Nov 4, 2016

View reviewed changes

Always use the cwd of the process when checking for dotnet cli

981cf65

gregg-miskelly approved these changes Nov 4, 2016

View reviewed changes

Fix offline package creation

f6f9d64

chuckries force-pushed the debuggerPackage branch from 7c5b01f to 0797b7e Compare November 7, 2016 21:14

Respond to code review feedback

2480214

chuckries force-pushed the debuggerPackage branch from 0797b7e to 2480214 Compare November 7, 2016 21:23

DustinCampbell approved these changes Nov 7, 2016

View reviewed changes

chuckries merged commit e906f18 into dotnet:master Nov 7, 2016

TheRealPiotrP mentioned this pull request Nov 28, 2023

[Snyk] Fix for 28 vulnerabilities TheRealPiotrP/omnisharp-vscode#55

Open

TheRealPiotrP mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities TheRealPiotrP/omnisharp-vscode#62

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement debugger acquisition to use Package Manager #873

Reimplement debugger acquisition to use Package Manager #873

chuckries commented Nov 4, 2016

chuckries commented Nov 4, 2016

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

chuckries Nov 4, 2016 •

edited

Loading

DustinCampbell Nov 4, 2016

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

chuckries Nov 4, 2016 •

edited

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

chuckries Nov 7, 2016

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

DustinCampbell Nov 4, 2016

chuckries Nov 7, 2016

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading

chuckries commented Nov 7, 2016

DustinCampbell Nov 4, 2016

DustinCampbell Nov 7, 2016

Reimplement debugger acquisition to use Package Manager #873

Reimplement debugger acquisition to use Package Manager #873

Conversation

chuckries commented Nov 4, 2016

chuckries commented Nov 4, 2016

DustinCampbell Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

chuckries Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

chuckries Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

chuckries Nov 7, 2016

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016

Choose a reason for hiding this comment

chuckries Nov 7, 2016

Choose a reason for hiding this comment

gregg-miskelly Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

gregg-miskelly Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

gregg-miskelly Nov 4, 2016 • edited by chuckries Loading

Choose a reason for hiding this comment

chuckries commented Nov 7, 2016

DustinCampbell Nov 4, 2016

Choose a reason for hiding this comment

DustinCampbell Nov 7, 2016

Choose a reason for hiding this comment

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

chuckries Nov 4, 2016 •

edited

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

chuckries Nov 4, 2016 •

edited

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

DustinCampbell Nov 4, 2016 •

edited by chuckries

Loading

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading

gregg-miskelly Nov 4, 2016 •

edited by chuckries

Loading