Merge #3429 perf: avoid searching for SAM CLI

Author: justinmk3

src/shared/logger/logger.ts

@@ -142,3 +142,24 @@ export function getNullLogger(type?: 'channel' | 'debugConsole' | 'main'): Logge
 export function setLogger(logger: Logger | undefined, type?: 'channel' | 'debugConsole' | 'main') {
     toolkitLoggers[type ?? 'main'] = logger
 }
+
+export class PerfLog {
+    private log
+    public readonly start
+
+    public constructor(public readonly topic: string) {
+        const log = getLogger()
+        this.log = log
+        if (log.logLevelEnabled('verbose')) {
+            this.start = Date.now()
+        }
+    }
+
+    public done(): void {
+        if (!this.start) {
+            return
+        }
+        const elapsed = Date.now() - this.start
+        this.log.verbose('%s took %dms', this.topic, elapsed.toFixed(1))
+    }
+}

src/shared/sam/cli/samCliLocator.ts

@@ -9,16 +9,40 @@ import * as filesystemUtilities from '../../filesystemUtilities'
 import { getLogger, Logger } from '../../logger'
 import { SamCliInfoInvocation } from './samCliInfo'
 import { DefaultSamCliValidator, SamCliValidatorContext, SamCliVersionValidation } from './samCliValidator'
+import { SystemUtilities } from '../../systemUtilities'
+import { PerfLog } from '../../logger/logger'
 
 export interface SamCliLocationProvider {
     getLocation(): Promise<{ path: string; version: string } | undefined>
 }
 
 export class DefaultSamCliLocationProvider implements SamCliLocationProvider {
     private static samCliLocator: BaseSamCliLocator | undefined
+    protected static cachedSamLocation: { path: string; version: string } | undefined
 
+    /** Checks that the given `sam` actually works by invoking `sam --version`. */
+    private static async isValidSamLocation(samPath: string) {
+        return await SystemUtilities.tryRun(samPath, ['--version'], 'no', 'SAM CLI')
+    }
+
+    /**
+     * Gets the last-found `sam` location, or searches the system if a working
+     * `sam` wasn't previously found and cached.
+     */
     public async getLocation() {
-        return DefaultSamCliLocationProvider.getSamCliLocator().getLocation()
+        const perflog = new PerfLog('samCliLocator: getLocation')
+        const cachedLoc = DefaultSamCliLocationProvider.cachedSamLocation
+
+        // Avoid searching the system for `sam` (especially slow on Windows).
+        if (cachedLoc && (await DefaultSamCliLocationProvider.isValidSamLocation(cachedLoc.path))) {
+            perflog.done()
+            return cachedLoc
+        }
+
+        DefaultSamCliLocationProvider.cachedSamLocation =
+            await DefaultSamCliLocationProvider.getSamCliLocator().getLocation()
+        perflog.done()
+        return DefaultSamCliLocationProvider.cachedSamLocation
     }
 
     public static getSamCliLocator(): SamCliLocationProvider {
@@ -43,7 +67,6 @@ abstract class BaseSamCliLocator {
         this.verifyOs()
     }
 
-    // TODO: this method is being called multiple times on my Windows machine and is really slow
     public async getLocation() {
         let location = await this.findFileInFolders(this.getExecutableFilenames(), this.getExecutableFolders())
 
@@ -69,11 +92,10 @@ abstract class BaseSamCliLocator {
 
         for (const fullPath of fullPaths) {
             if (!BaseSamCliLocator.didFind) {
-                this.logger.verbose(`Searching for SAM CLI in: ${fullPath}`)
+                this.logger.verbose(`samCliLocator: searching in: ${fullPath}`)
             }
             const context: SamCliValidatorContext = {
                 samCliLocation: async () => fullPath,
-                getSamCliExecutableId: async () => 'bogus',
                 getSamCliInfo: async () => new SamCliInfoInvocation(fullPath).execute(),
             }
             const validator = new DefaultSamCliValidator(context)
@@ -84,10 +106,12 @@ abstract class BaseSamCliLocator {
                         BaseSamCliLocator.didFind = true
                         return { path: fullPath, version: validationResult.version }
                     }
-                    this.logger.warn(`Found invalid SAM executable (${validationResult.validation}): ${fullPath}`)
+                    this.logger.warn(
+                        `samCliLocator: found invalid SAM CLI: (${validationResult.validation}): ${fullPath}`
+                    )
                 } catch (e) {
                     const err = e as Error
-                    this.logger.error(err)
+                    this.logger.error('samCliLocator failed: %s', err.message)
                 }
             }
         }
@@ -156,6 +180,7 @@ class WindowsSamCliLocator extends BaseSamCliLocator {
 
 class UnixSamCliLocator extends BaseSamCliLocator {
     private static readonly locationPaths: string[] = [
+        '/opt/homebrew/bin/sam',
         '/usr/local/bin',
         '/usr/bin',
         // WEIRD BUT TRUE: brew installs to /home/linuxbrew/.linuxbrew if

src/shared/sam/cli/samCliSettings.ts

@@ -94,8 +94,10 @@ export class SamCliSettings extends fromExtensionManifest('aws.samcli', descript
     }
 
     /**
-     * Gets location of `sam` from user config, or tries to find `sam` on the
-     * system if the user config is invalid.
+     * Gets location of `sam` from:
+     * 1. user config (if any; overrides auto-detected location)
+     * 2. previous cached location (if valid)
+     * 3. searching for `sam` on the system
      *
      * @returns `autoDetected=true` if auto-detection was _attempted_.
      */

src/shared/sam/cli/samCliValidator.ts

@@ -3,7 +3,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { stat } from 'fs-extra'
 import * as semver from 'semver'
 import { ClassToInterfaceType } from '../../utilities/tsUtils'
 import { SamCliSettings } from './samCliSettings'
@@ -63,9 +62,6 @@ export interface SamCliValidator {
 export type SamCliValidatorContext = ClassToInterfaceType<DefaultSamCliValidatorContext>
 
 export class DefaultSamCliValidator implements SamCliValidator {
-    private cachedSamInfoResponse?: SamCliInfoResponse
-    private cachedSamCliVersionId?: string
-
     public constructor(private readonly context: SamCliValidatorContext) {}
 
     public async detectValidSamCli(): Promise<SamCliValidatorResult> {
@@ -83,29 +79,11 @@ export class DefaultSamCliValidator implements SamCliValidator {
     }
 
     public async getVersionValidatorResult(): Promise<SamCliVersionValidatorResult> {
-        const samCliId: string = await this.context.getSamCliExecutableId()
-        if (!this.isSamCliVersionCached(samCliId)) {
-            this.cachedSamInfoResponse = await this.context.getSamCliInfo()
-            this.cachedSamCliVersionId = samCliId
-        }
-
-        const version = this.cachedSamInfoResponse!.version
-
+        const r = await this.context.getSamCliInfo()
         return {
-            version,
-            validation: DefaultSamCliValidator.validateSamCliVersion(version),
-        }
-    }
-
-    private isSamCliVersionCached(samCliVersionId: string): boolean {
-        if (!this.cachedSamInfoResponse) {
-            return false
+            version: r.version,
+            validation: DefaultSamCliValidator.validateSamCliVersion(r.version),
         }
-        if (!this.cachedSamCliVersionId) {
-            return false
-        }
-
-        return this.cachedSamCliVersionId === samCliVersionId
     }
 
     public static validateSamCliVersion(version?: string): SamCliVersionValidation {
@@ -136,19 +114,6 @@ export class DefaultSamCliValidatorContext implements SamCliValidatorContext {
         return (await this.config.getOrDetectSamCli()).path
     }
 
-    public async getSamCliExecutableId(): Promise<string> {
-        // Function should never get called if there is no SAM CLI
-        const location = await this.samCliLocation()
-        if (!location) {
-            throw new Error('SAM CLI does not exist')
-        }
-
-        // The modification timestamp of SAM CLI is used as the "distinct executable id"
-        const stats = await stat(location)
-
-        return stats.mtime.valueOf().toString()
-    }
-
     public async getSamCliInfo(): Promise<SamCliInfoResponse> {
         const samPath = await this.samCliLocation()
         if (!samPath) {

src/shared/systemUtilities.ts

@@ -183,6 +183,32 @@ export class SystemUtilities {
     // TODO: implement this by checking the file mode
     // public static async checkExactPerms(file: string | vscode.Uri, perms: `${PermissionsTriplet}${PermissionsTriplet}${PermissionsTriplet}`)
 
+    /**
+     * Tries to execute a program at path `p` with the given args and
+     * optionally checks the output for `expected`.
+     *
+     * @param p path to a program to execute
+     * @param args program args
+     * @param doLog log failures
+     * @param expected output must contain this string
+     */
+    public static async tryRun(
+        p: string,
+        args: string[],
+        logging: 'yes' | 'no' | 'noresult' = 'yes',
+        expected?: string
+    ): Promise<boolean> {
+        const proc = new ChildProcess(p, args, { logging: 'no' })
+        const r = await proc.run()
+        const ok = r.exitCode === 0 && (expected === undefined || r.stdout.includes(expected))
+        if (logging === 'noresult') {
+            getLogger().info('tryRun: %s: %s', ok ? 'ok' : 'failed', proc)
+        } else if (logging !== 'no') {
+            getLogger().info('tryRun: %s: %s %O', ok ? 'ok' : 'failed', proc, proc.result())
+        }
+        return ok
+    }
+
     /**
      * Gets the fullpath to `code` (VSCode CLI), or falls back to "code" (not
      * absolute) if it works.
@@ -217,13 +243,10 @@ export class SystemUtilities {
             if (!vsc || (vsc !== 'code' && !fs.existsSync(vsc))) {
                 continue
             }
-            const proc = new ChildProcess(vsc, ['--version'])
-            const r = await proc.run()
-            if (r.exitCode === 0) {
+            if (await SystemUtilities.tryRun(vsc, ['--version'])) {
                 SystemUtilities.vscPath = vsc
                 return vsc
             }
-            getLogger().warn('getVscodeCliPath: failed: %s %O', proc, proc.result())
         }
 
         return undefined
@@ -245,8 +268,7 @@ export class SystemUtilities {
 
         for (const tsc of tscPaths) {
             // Try to run "tsc -v".
-            const result = await new ChildProcess(tsc, ['-v']).run()
-            if (result.exitCode === 0 && result.stdout.includes('Version')) {
+            if (await SystemUtilities.tryRun(tsc, ['-v'], 'yes', 'Version')) {
                 return tsc
             }
         }
@@ -275,13 +297,10 @@ export class SystemUtilities {
             if (!p || ('ssh' !== p && !fs.existsSync(p))) {
                 continue
             }
-            const proc = new ChildProcess(p, ['-G', 'x'])
-            const r = await proc.run()
-            if (r.exitCode === 0) {
+            if (await SystemUtilities.tryRun(p, ['-G', 'x'], 'noresult' /* "ssh -G" prints quasi-sensitive info. */)) {
                 SystemUtilities.sshPath = p
                 return p
             }
-            getLogger().warn('findSshPath: failed: %s', proc)
         }
     }
 
@@ -300,13 +319,10 @@ export class SystemUtilities {
             if (!p || ('git' !== p && !fs.existsSync(p))) {
                 continue
             }
-            const proc = new ChildProcess(p, ['--version'])
-            const r = await proc.run()
-            if (r.exitCode === 0) {
+            if (await SystemUtilities.tryRun(p, ['--version'])) {
                 SystemUtilities.gitPath = p
                 return p
             }
-            getLogger().warn('findGitPath: failed: %s', proc)
         }
     }
 
@@ -323,13 +339,10 @@ export class SystemUtilities {
             if (!p || ('bash' !== p && !fs.existsSync(p))) {
                 continue
             }
-            const proc = new ChildProcess(p, ['--version'])
-            const r = await proc.run()
-            if (r.exitCode === 0) {
+            if (await SystemUtilities.tryRun(p, ['--version'])) {
                 SystemUtilities.bashPath = p
                 return p
             }
-            getLogger().warn('findBashPath: failed: %s', proc)
         }
     }
 }

src/shared/utilities/asyncCollection.ts

@@ -222,11 +222,11 @@ async function promise<T>(iterable: AsyncIterable<T>): Promise<T[]> {
 function addToMap<T, U extends string>(map: Map<string, T>, selector: KeySelector<T, U> | StringProperty<T>, item: T) {
     const key = typeof selector === 'function' ? selector(item) : item[selector]
     if (key) {
-        if (map.has(key as keyof typeof map['keys'])) {
+        if (map.has(key as keyof (typeof map)['keys'])) {
             throw new Error(`Duplicate key found when converting AsyncIterable to map: ${key}`)
         }
 
-        map.set(key as keyof typeof map['keys'], item)
+        map.set(key as keyof (typeof map)['keys'], item)
     }
 }
 

src/shared/utilities/functionUtils.ts

@@ -56,9 +56,10 @@ export function onceChanged<T, U extends any[]>(fn: (...args: U) => T): (...args
     let ran = false
     let prevArgs = ''
 
-    return (...args) => ((ran && prevArgs === args.map(String).join(':'))
-        ? val
-        : ((val = fn(...args)), (ran = true), (prevArgs = args.map(String).join(':')), val))
+    return (...args) =>
+        ran && prevArgs === args.map(String).join(':')
+            ? val
+            : ((val = fn(...args)), (ran = true), (prevArgs = args.map(String).join(':')), val)
 }
 
 /**

src/shared/utilities/typeConstructors.ts

@@ -51,7 +51,7 @@ export type FromDescriptor<T extends TypeDescriptor> = {
 
 // `Symbol` and `BigInt` are included here, though in-practice
 const primitives = [Number, String, Boolean, Object, Symbol, BigInt]
-function isPrimitiveConstructor(type: unknown): type is typeof primitives[number] {
+function isPrimitiveConstructor(type: unknown): type is (typeof primitives)[number] {
     return !!primitives.find(p => p === type)
 }
 

src/stepFunctions/activation.ts

@@ -88,7 +88,9 @@ async function registerStepFunctionCommands(
     )
 }
 
-export function initalizeWebviewPaths(context: vscode.ExtensionContext): typeof globals['visualizationResourcePaths'] {
+export function initalizeWebviewPaths(
+    context: vscode.ExtensionContext
+): (typeof globals)['visualizationResourcePaths'] {
     // Location for script in body of webview that handles input from user
     // and calls the code to render state machine graph
 

src/test/shared/sam/cli/samCliTestUtils.ts

@@ -21,9 +21,9 @@ export class MockSamCliProcessInvoker implements SamCliProcessInvoker {
 
         this.validateArgs(invokeSettings.arguments)
 
-        return ({
+        return {
             exitCode: 0,
-        } as any) as ChildProcessResult
+        } as any as ChildProcessResult
     }
 }