PR feedback

Author: JoeRobich

package.json

@@ -414,8 +414,7 @@
         "neutral"
       ],
       "installTestPath": "./.roslynCopilot/Microsoft.VisualStudio.Copilot.Roslyn.LanguageServer.dll",
-      "integrity": "0BD733B23A706226F2B429AB0CE35576FB474493C8E19C7B88311007876DE5CC",
-      "isOptional": true
+      "integrity": "0BD733B23A706226F2B429AB0CE35576FB474493C8E19C7B88311007876DE5CC"
     },
     {
       "id": "Debugger",

src/lsptoolshost/extensions/builtInComponents.ts

@@ -27,13 +27,11 @@ export const componentInfo: { [key: string]: ComponentInfo } = {
             'Microsoft.VisualStudio.DesignTools.CodeAnalysis.dll',
             'Microsoft.VisualStudio.DesignTools.CodeAnalysis.Diagnostics.dll',
         ],
-        isOptional: true,
     },
     razorDevKit: {
         defaultFolderName: '.razorDevKit',
         optionName: 'razorDevKit',
         componentDllPaths: ['Microsoft.VisualStudio.DevKit.Razor.dll'],
-        isOptional: true,
     },
     razorExtension: {
         defaultFolderName: '.razorExtension',

src/packageManager/IPackage.ts

@@ -13,5 +13,4 @@ export interface IPackage {
     platformId?: string;
     integrity?: string;
     isFramework?: boolean;
-    isOptional?: boolean;
 }

src/packageManager/absolutePathPackage.ts

@@ -20,8 +20,7 @@ export class AbsolutePathPackage implements IPackage {
         public fallbackUrl?: string,
         public platformId?: string,
         public integrity?: string,
-        public isFramework?: boolean,
-        public isOptional?: boolean
+        public isFramework?: boolean
     ) {}
 
     public static getAbsolutePathPackage(pkg: Package, extensionPath: string) {
@@ -37,8 +36,7 @@ export class AbsolutePathPackage implements IPackage {
             pkg.fallbackUrl,
             pkg.platformId,
             pkg.integrity,
-            pkg.isFramework,
-            pkg.isOptional
+            pkg.isFramework
         );
     }
 }

src/packageManager/downloadAndInstallPackages.ts

@@ -26,29 +26,7 @@ export async function downloadAndInstallPackages(
     telemetryReporter?: ITelemetryReporter,
     token?: CancellationToken
 ): Promise<boolean> {
-    function sendInstallationFailureTelemetry(pkg: AbsolutePathPackage, installationStage: string, error: any): void {
-        if (!telemetryReporter) {
-            return;
-        }
-
-        const telemetryProperties: { [key: string]: string } = {
-            installStage: installationStage,
-            packageId: pkg.id,
-            isOptional: pkg.isOptional ? 'true' : 'false',
-        };
-
-        if (error instanceof NestedError && error.err instanceof PackageError) {
-            telemetryProperties['error.message'] = error.err.message;
-            telemetryProperties['error.packageUrl'] = error.err.pkg.url;
-        } else if (error instanceof PackageError) {
-            telemetryProperties['error.message'] = error.message;
-            telemetryProperties['error.packageUrl'] = error.pkg.url;
-        }
-
-        const eventName = pkg.isOptional ? 'OptionalPackageInstallationFailed' : 'PackageInstallationFailed';
-        telemetryReporter.sendTelemetryEvent(eventName, telemetryProperties);
-    }
-
+    let downloadFailed = false;
     eventStream.post(new PackageInstallStart());
     for (const pkg of packages) {
         let installationStage = 'touchBeginFile';
@@ -89,12 +67,7 @@ export async function downloadAndInstallPackages(
             // Send telemetry for the failure
             sendInstallationFailureTelemetry(pkg, installationStage, error);
 
-            // If the package is optional, log and continue with the next package
-            if (pkg.isOptional) {
-                continue;
-            }
-
-            return false;
+            downloadFailed = true;
         } finally {
             try {
                 if (await installFileExists(pkg.installPath, InstallFileType.Begin)) {
@@ -106,5 +79,27 @@ export async function downloadAndInstallPackages(
         }
     }
 
-    return true;
+    return !downloadFailed;
+
+    function sendInstallationFailureTelemetry(pkg: AbsolutePathPackage, installationStage: string, error: any): void {
+        if (!telemetryReporter) {
+            return;
+        }
+
+        const telemetryProperties: { [key: string]: string } = {
+            installStage: installationStage,
+            packageId: pkg.id,
+            isOptional: pkg.isOptional ? 'true' : 'false',
+        };
+
+        if (error instanceof NestedError && error.err instanceof PackageError) {
+            telemetryProperties['error.message'] = error.err.message;
+            telemetryProperties['error.packageUrl'] = error.err.pkg.url;
+        } else if (error instanceof PackageError) {
+            telemetryProperties['error.message'] = error.message;
+            telemetryProperties['error.packageUrl'] = error.pkg.url;
+        }
+
+        telemetryReporter.sendTelemetryEvent('PackageInstallationFailed', telemetryProperties);
+    }
 }

test/omnisharp/omnisharpUnitTests/packages/downloadAndInstallPackages.test.ts

@@ -218,74 +218,6 @@ describe(`${downloadAndInstallPackages.name}`, () => {
         });
     });
 
-    describe('Optional packages', () => {
-        test('Returns true and continues when an optional package fails to download', async () => {
-            const optionalPackage = <AbsolutePathPackage[]>[
-                {
-                    url: `${server.baseUrl}/notDownloadablePackage`,
-                    description: 'Optional Package',
-                    installPath: new AbsolutePath(tmpDirPath),
-                    isOptional: true,
-                },
-            ];
-
-            const result = await downloadAndInstallPackages(
-                optionalPackage,
-                networkSettingsProvider,
-                eventStream,
-                downloadValidator
-            );
-            expect(result).toBe(true);
-        });
-
-        test('Continues to install remaining packages after optional package fails', async () => {
-            const tmpInstallDir2 = await CreateTmpDir(true);
-            const mixedPackages = <AbsolutePathPackage[]>[
-                {
-                    url: `${server.baseUrl}/notDownloadablePackage`,
-                    description: 'Optional Package',
-                    installPath: new AbsolutePath(tmpDirPath),
-                    isOptional: true,
-                },
-                {
-                    url: `${server.baseUrl}/downloadablePackage`,
-                    description: packageDescription,
-                    installPath: new AbsolutePath(tmpInstallDir2.name),
-                },
-            ];
-
-            const result = await downloadAndInstallPackages(
-                mixedPackages,
-                networkSettingsProvider,
-                eventStream,
-                downloadValidator
-            );
-            expect(result).toBe(true);
-            expect(await util.fileExists(path.join(tmpInstallDir2.name, 'install.Lock'))).toBe(true);
-            tmpInstallDir2.dispose();
-        });
-
-        test('InstallationFailure event is logged for optional package', async () => {
-            const optionalPackage = <AbsolutePathPackage[]>[
-                {
-                    url: `${server.baseUrl}/notDownloadablePackage`,
-                    description: 'Optional Package',
-                    installPath: new AbsolutePath(tmpDirPath),
-                    isOptional: true,
-                },
-            ];
-
-            eventBus.getEvents(); // Clear any previous events
-            await downloadAndInstallPackages(optionalPackage, networkSettingsProvider, eventStream, downloadValidator);
-            const obtainedEvents = eventBus.getEvents();
-            const installationFailureEvent = obtainedEvents.find(
-                (event) => event instanceof InstallationFailure
-            ) as InstallationFailure;
-            expect(installationFailureEvent).toBeDefined();
-            expect(installationFailureEvent.stage).toEqual('downloadPackage');
-        });
-    });
-
     afterEach(async () => {
         if (tmpInstallDir) {
             tmpInstallDir.dispose();