fix: ensure system path is created and show notification if not in PA…

…TH (#10176)

* fix: ensure system path is created and show notification if not in PATH

Fixes #5776

Signed-off-by: Jeff MAURY <[email protected]>

* fix: apply suggestion from @feloy

Co-authored-by: Philippe Martin <[email protected]>
Signed-off-by: Jeff MAURY <[email protected]>

---------

Signed-off-by: Jeff MAURY <[email protected]>
Co-authored-by: Philippe Martin <[email protected]>

extensions/compose/src/cli-run.spec.ts

@@ -20,9 +20,9 @@ import * as fs from 'node:fs';
 import * as path from 'node:path';
 
 import * as extensionApi from '@podman-desktop/api';
-import { beforeEach, expect, test, vi } from 'vitest';
+import { afterEach, beforeEach, expect, test, vi } from 'vitest';
 
-import { installBinaryToSystem } from './cli-run';
+import { installBinaryToSystem, localBinDir } from './cli-run';
 
 vi.mock('@podman-desktop/api', async () => {
   return {
@@ -31,6 +31,7 @@ vi.mock('@podman-desktop/api', async () => {
       showErrorMessage: vi.fn(),
       withProgress: vi.fn(),
       showNotification: vi.fn(),
+      showWarningMessage: vi.fn(),
     },
     process: {
       exec: vi.fn(),
@@ -48,9 +49,17 @@ vi.mock('node:fs', async () => {
   };
 });
 
+let previousPath: string | undefined;
+
 beforeEach(() => {
   vi.resetAllMocks();
   vi.restoreAllMocks();
+  previousPath = process.env.PATH;
+  process.env.PATH = localBinDir;
+});
+
+afterEach(() => {
+  process.env.PATH = previousPath;
 });
 
 test('error: expect installBinaryToSystem to fail with a non existing binary', async () => {
@@ -127,3 +136,31 @@ test('success: installBinaryToSystem on linux with /usr/local/bin NOT created ye
     expect.objectContaining({ isAdmin: true }),
   );
 });
+
+test('success: installBinaryToSystem to show warning if binary path not in PATH', async () => {
+  // Mock the platform to be darwin
+  Object.defineProperty(process, 'platform', {
+    value: 'linux',
+  });
+
+  process.env.PATH = '';
+
+  // Mock existsSync to be false since within the function it's doing: !fs.existsSync(localBinDir)
+  vi.spyOn(fs, 'existsSync').mockImplementation(() => {
+    return false;
+  });
+
+  // Run installBinaryToSystem which will trigger the spyOn mock
+  await installBinaryToSystem('test', 'tmpBinary');
+
+  // check called with admin being true
+  expect(extensionApi.process.exec).toBeCalledWith(
+    '/bin/sh',
+    expect.arrayContaining([
+      '-c',
+      `mkdir -p /usr/local/bin && cp test ${path.sep}usr${path.sep}local${path.sep}bin${path.sep}tmpBinary`,
+    ]),
+    expect.objectContaining({ isAdmin: true }),
+  );
+  expect(extensionApi.window.showWarningMessage).toBeCalled();
+});

extensions/compose/src/cli-run.ts

@@ -22,19 +22,13 @@ import * as path from 'node:path';
 
 import * as extensionApi from '@podman-desktop/api';
 
-const localBinDir = '/usr/local/bin';
+export const localBinDir = path.join('/', 'usr', 'local', 'bin');
+export const localWindowsBinDir = path.join(os.homedir(), 'AppData', 'Local', 'Microsoft', 'WindowsApps');
 
 export function getSystemBinaryPath(binaryName: string): string {
   switch (process.platform) {
     case 'win32':
-      return path.join(
-        os.homedir(),
-        'AppData',
-        'Local',
-        'Microsoft',
-        'WindowsApps',
-        binaryName.endsWith('.exe') ? binaryName : `${binaryName}.exe`,
-      );
+      return path.join(localWindowsBinDir, binaryName.endsWith('.exe') ? binaryName : `${binaryName}.exe`);
     case 'darwin':
     case 'linux':
       return path.join(localBinDir, binaryName);
@@ -75,12 +69,15 @@ export async function installBinaryToSystem(binaryPath: string, binaryName: stri
 
   // If on macOS or Linux, check to see if the /usr/local/bin directory exists,
   // if it does not, then add mkdir -p /usr/local/bin to the start of the command when moving the binary.
-  if ((system === 'linux' || system === 'darwin') && !fs.existsSync(localBinDir)) {
+  const destinationFolder = path.dirname(destinationPath);
+  if (!fs.existsSync(destinationFolder)) {
     if (system === 'darwin') {
-      args.unshift('mkdir', '-p', localBinDir, '&&');
-    } else {
+      args.unshift('mkdir', '-p', destinationFolder, '&&');
+    } else if (system === 'linux') {
       // add mkdir -p /usr/local/bin just after the first item or args array (so it'll be in the -c shell instruction)
       args[args.length - 1] = `mkdir -p /usr/local/bin && ${args[args.length - 1]}`;
+    } else {
+      args.unshift('mkdir', destinationFolder, '&&');
     }
   }
 
@@ -91,6 +88,12 @@ export async function installBinaryToSystem(binaryPath: string, binaryName: stri
     // Use admin prileges / ask for password for copying to /usr/local/bin
     await extensionApi.process.exec(command, args, { isAdmin: true });
     console.log(`Successfully installed '${binaryName}' binary.`);
+    if (!process.env.PATH?.includes(destinationFolder)) {
+      await extensionApi.window.showWarningMessage(
+        `The compose binary has been installed into ${destinationFolder} but it is not in the system path. You should add it manually if you want to use compose from cli.`,
+        'OK',
+      );
+    }
     return destinationPath;
   } catch (error) {
     console.error(`Failed to install '${binaryName}' binary: ${error}`);