SAM-detect: give priority to user setting

Instead of eager-detecting SAM CLI and force-updating the user setting,

1. Never force-update the user setting (unless user invokes `AWS: Detect SAM CLI` command).
    * Eliminates lots of complexity, e.g.:
      * Because the `aws.samcli.location` config is `Global`, a change in
        _any_ VSCode instance will trigger `onDidChangeConfiguration` in _all_
        running VSCode instances.
      * VSCode shares settings between local and remote instances. (This
        may depend on the `remote.SSH.useLocalServer` setting?)
2. At various sites where SAM CLI is needed, auto-detect SAM CLI if and
   only if the user setting is empty.
    * We cannot "validate" SAM CLI location because in a remote session,
      `require('fs').existsSync('/home/linuxbrew/.linuxbrew/bin/sam')`
      returns false even if the path is valid in WSL. #1300

Followup (and partial revert) of #1241.

src/shared/sam/activation.ts

@@ -65,11 +65,11 @@ export async function activate(ctx: ExtContext): Promise<void> {
         )
     )
 
-    await detectSamCli({ passive: true, showMessage: false })
     ctx.extensionContext.subscriptions.push(
         vscode.workspace.onDidChangeConfiguration(configurationChangeEvent => {
             if (configurationChangeEvent.affectsConfiguration('aws.samcli.location')) {
-                detectSamCli({ passive: true, showMessage: undefined })
+                // This only shows a message (passive=true), does not set anything.
+                detectSamCli({ passive: true, showMessage: true })
             }
         })
     )

src/shared/sam/cli/samCliConfiguration.ts

@@ -4,16 +4,31 @@
  */
 
 import * as vscode from 'vscode'
-import * as filesystemUtilities from '../../filesystemUtilities'
 import { SettingsConfiguration } from '../../settingsConfiguration'
 import { SamCliLocationProvider } from './samCliLocator'
 
 export interface SamCliConfiguration {
+    /** Gets the current SAM CLI location from the VSCode settings store. */
     getSamCliLocation(): string | undefined
 
+    /** Sets the SAM CLI location in the VSCode settings store. */
     setSamCliLocation(location: string | undefined): Promise<void>
 
+    /**
+     * Initializes this SamCliConfiguration object from the VSCode user settings,
+     * or tries to auto-detect `sam` in the environment.
+     *
+     * @returns true if auto-detection failed; false if auto-detection succeeded or was not attempted.
+     */
     initialize(): Promise<void>
+
+    /**
+     * Gets location of `sam` from user config, or tries to find `sam` on the
+     * system if the user config is invalid.
+     *
+     * @returns empty string if `sam` was not found on the system
+     */
+    getOrDetectSamCli(): Promise<{ path: string; autoDetected: boolean }>
 }
 
 export class DefaultSamCliConfiguration implements SamCliConfiguration {
@@ -26,12 +41,10 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration {
         this._samCliLocationProvider = samCliLocationProvider
     }
 
-    /** Gets the current SAM CLI location from the VSCode settings store. */
     public getSamCliLocation(): string | undefined {
         return this._configuration.readSetting<string>(DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION)
     }
 
-    /** Sets the SAM CLI location in the VSCode settings store. */
     public async setSamCliLocation(location: string | undefined): Promise<void> {
         await this._configuration.writeSetting(
             DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION,
@@ -41,22 +54,32 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration {
     }
 
     /**
-     * Initializes this SamCliConfiguration object from the VSCode user settings,
-     * or tries to auto-detect `sam` in the environment.
+     * Gets the `samcli.location` setting if set by the user, else searches for
+     * `sam` on the system and returns the result.
+     *
+     * Returns `autoDetected=true` if auto-detection was _attempted_. If `sam`
+     * was not found on the system then `path=""`.
      */
-    public async initialize(): Promise<void> {
-        const configLocation = this.getSamCliLocation() ?? ''
-        if (configLocation) {
-            if (await filesystemUtilities.fileExists(configLocation)) {
-                return
-            }
+    public async getOrDetectSamCli(): Promise<{ path: string; autoDetected: boolean }> {
+        const fromConfig =
+            this._configuration.readSetting<string>(DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION) ?? ''
+        // Respect user setting, do not validate it. fileExists() does not
+        // understand WSL paths, for example.  https://github.com/aws/aws-toolkit-vscode/issues/1300
+        if (fromConfig) {
+            return { path: fromConfig, autoDetected: false }
         }
+        const fromSearch = (await this._samCliLocationProvider.getLocation()) ?? ''
+        return { path: fromSearch, autoDetected: true }
+    }
 
-        const detectedLocation = (await this._samCliLocationProvider.getLocation()) ?? ''
-        // Avoid setting the value redundantly (could cause a loop because we
-        // listen to the `onDidChangeConfiguration` event).
-        if (detectedLocation && configLocation !== detectedLocation) {
-            await this.setSamCliLocation(detectedLocation)
+    // TODO: remove this, it's only used by tests. Maybe the tests can call
+    // detectSamCli() instead, or likely the tests need to be revisited entirely.
+    public async initialize(): Promise<void> {
+        const samPath = await this.getOrDetectSamCli()
+        if (!samPath.autoDetected) {
+            return
         }
+
+        await this.setSamCliLocation(samPath.path)
     }
 }

src/shared/sam/cli/samCliDetection.ts

@@ -18,17 +18,15 @@ const lock = new AsyncLock()
 const learnMore = localize('AWS.samcli.userChoice.visit.install.url', 'Get SAM CLI')
 const browseToSamCli = localize('AWS.samcli.userChoice.browse', 'Locate SAM CLI...')
 const settingsUpdated = localize('AWS.samcli.detect.settings.updated', 'Settings updated.')
-const settingsNotUpdated = localize('AWS.samcli.detect.settings.not.updated', 'No settings changes necessary.')
-
-/** Most-recent resolved SAM CLI location. */
-let currentsamCliLocation: string | undefined = undefined
 
 /**
+ * Searches for SAM CLI on the system, updates the user setting if
+ * `passive=false`, and shows a message if `showMessage=true`.
  *
- * @param args.passive  If true, this was _not_ a user-initiated action.
+ * @param args.passive  Controls whether user setting is updated. Also sets the
+ * telemetry "passive" flag.
  * @param args.showMessage true: always show message, false: never show
- * message, undefined: show message only if the new setting differs from
- * the old setting.
+ * message (except if SAM was not found)
  */
 export async function detectSamCli(args: { passive: boolean; showMessage: boolean | undefined }): Promise<void> {
     await lock.acquire('detect SAM CLI', async () => {
@@ -37,28 +35,34 @@ export async function detectSamCli(args: { passive: boolean; showMessage: boolea
             new DefaultSamCliLocationProvider()
         )
 
-        const valueBeforeInit = samCliConfig.getSamCliLocation()
-        await samCliConfig.initialize()
-        const valueAfterInit = samCliConfig.getSamCliLocation()
-        const isAutoDetected = valueBeforeInit !== valueAfterInit
-        const isUserSettingChanged = currentsamCliLocation !== valueAfterInit
-        currentsamCliLocation = valueAfterInit
+        if (!args.passive) {
+            // Force getOrDetectSamCli() to search for SAM CLI.
+            await samCliConfig.setSamCliLocation('')
+        }
+
+        const sam = await samCliConfig.getOrDetectSamCli()
+        const notFound = sam.path === ''
 
-        if (args.showMessage !== false) {
-            if (!valueAfterInit) {
-                notifyUserSamCliNotDetected(samCliConfig)
-            } else if (isUserSettingChanged || args.showMessage === true) {
-                const message =
-                    !isAutoDetected && !isUserSettingChanged
-                        ? getSettingsNotUpdatedMessage(valueBeforeInit ?? '?')
-                        : getSettingsUpdatedMessage(currentsamCliLocation ?? '?')
+        // Update the user setting.
+        //
+        // NOTE: We must NOT _passively_ auto-update the user setting, that
+        // conflicts with VSCode "remote": each VSCode instance will update the
+        // setting based on its local environment, but the user settings are
+        // shared across VSCode instances...
+        if (!args.passive && sam.autoDetected) {
+            await samCliConfig.setSamCliLocation(sam.path)
+        }
 
-                vscode.window.showInformationMessage(message)
+        if (args.showMessage !== false || notFound) {
+            if (notFound) {
+                notifyUserSamCliNotDetected(samCliConfig)
+            } else if (args.showMessage === true) {
+                vscode.window.showInformationMessage(getSettingsUpdatedMessage(sam.path ?? '?'))
             }
         }
 
         if (!args.passive) {
-            recordSamDetect({ result: currentsamCliLocation === undefined ? 'Failed' : 'Succeeded' })
+            recordSamDetect({ result: sam.path ? 'Succeeded' : 'Failed' })
         }
     })
 }
@@ -100,9 +104,3 @@ function getSettingsUpdatedMessage(location: string): string {
 
     return `${settingsUpdated} ${configuredLocation}`
 }
-
-function getSettingsNotUpdatedMessage(location: string): string {
-    const configuredLocation = localize('AWS.samcli.configured.location', 'SAM CLI Location: {0}', location)
-
-    return `${settingsNotUpdated} ${configuredLocation}`
-}

src/shared/sam/cli/samCliInvoker.ts

@@ -42,24 +42,21 @@ export class DefaultSamCliProcessInvoker implements SamCliProcessInvoker {
     public async invoke(options?: SamCliProcessInvokeOptions): Promise<ChildProcessResult> {
         const invokeOptions = makeRequiredSamCliProcessInvokeOptions(options)
 
+        const sam = await this.context.cliConfig.getOrDetectSamCli()
+        if (sam.path === '') {
+            const err = new Error('SAM CLI not found')
+            getLogger().error(err)
+            throw err
+        } else if (sam.autoDetected) {
+            getLogger().warn('SAM CLI not configured, using SAM found at: %O', sam.path)
+        }
+
         const childProcess: ChildProcess = new ChildProcess(
-            this.samCliLocation,
+            sam.path,
             invokeOptions.spawnOptions,
             ...invokeOptions.arguments
         )
 
         return await childProcess.run()
     }
-
-    // Gets SAM CLI Location, throws if not found
-    private get samCliLocation(): string {
-        const samCliLocation: string | undefined = this.context.cliConfig.getSamCliLocation()
-        if (!samCliLocation) {
-            const err = new Error('SAM CLI location not configured')
-            getLogger().error(err)
-            throw err
-        }
-
-        return samCliLocation
-    }
 }

src/shared/sam/cli/samCliLocalInvoke.ts

@@ -209,7 +209,15 @@ export class SamCliLocalInvokeInvocation {
     public async execute(timeout?: Timeout): Promise<void> {
         await this.validate()
 
-        const samCommand = this.invokerContext.cliConfig.getSamCliLocation() ?? 'sam'
+        const sam = await this.invokerContext.cliConfig.getOrDetectSamCli()
+        if (sam.path === '') {
+            // Don't throw here, let the command fail below.
+            getLogger().error('SAM CLI not found')
+        } else if (sam.autoDetected) {
+            getLogger().warn('SAM CLI not configured, using SAM found at: %O', sam.path)
+        }
+
+        const samCommand = sam.path ? sam.path : 'sam'
         const invokeArgs = [
             'local',
             'invoke',

src/shared/sam/cli/samCliValidator.ts

@@ -55,7 +55,7 @@ export interface SamCliValidator {
 }
 
 export interface SamCliValidatorContext {
-    samCliLocation: string | undefined
+    samCliLocation(): Promise<string>
     getSamCliExecutableId(): Promise<string>
     getSamCliInfo(): Promise<SamCliInfoResponse>
 }
@@ -71,10 +71,9 @@ export class DefaultSamCliValidator implements SamCliValidator {
             samCliFound: false,
         }
 
-        const samCliLocation = this.context.samCliLocation
-        if (samCliLocation) {
+        const sam = await this.context.samCliLocation()
+        if (sam) {
             result.samCliFound = true
-
             result.versionValidation = await this.getVersionValidatorResult()
         }
 
@@ -134,18 +133,18 @@ export class DefaultSamCliValidatorContext implements SamCliValidatorContext {
         private readonly invoker: SamCliProcessInvoker
     ) {}
 
-    public get samCliLocation(): string | undefined {
-        return this.samCliConfiguration.getSamCliLocation()
+    public async samCliLocation(): Promise<string> {
+        return (await this.samCliConfiguration.getOrDetectSamCli()).path
     }
 
     public async getSamCliExecutableId(): Promise<string> {
         // Function should never get called if there is no SAM CLI
-        if (!this.samCliLocation) {
+        if (!(await this.samCliLocation())) {
             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(this.samCliLocation)
+        const stats = await stat(await this.samCliLocation())
 
         return stats.mtime.valueOf().toString()
     }

src/shared/utilities/childProcess.ts

@@ -72,7 +72,7 @@ export class ChildProcess {
             throw new Error('process already started')
         }
 
-        getLogger().info(`Running command: ${this.process} ${this.args.join(' ')}`)
+        getLogger().info(`Running: ${this.process} ${this.args.join(' ')}`)
 
         // Async.
         // See also crossSpawn.spawnSync().

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

@@ -45,30 +45,6 @@ describe('SamCliConfiguration', () => {
         assert.strictEqual(samCliConfig.getSamCliLocation(), fakeCliLocation)
     })
 
-    it('calls location provider when config references file that does not exist', async () => {
-        let timesCalled: number = 0
-        const fakeCliLocation = path.join(tempFolder, 'fakeSamCli')
-
-        await settingsConfiguration.writeSetting(
-            DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION,
-            fakeCliLocation,
-            ''
-        )
-
-        const samCliConfig: SamCliConfiguration = new DefaultSamCliConfiguration(settingsConfiguration, {
-            getLocation: async (): Promise<string | undefined> => {
-                timesCalled++
-
-                return Promise.resolve(fakeCliLocation)
-            },
-        })
-
-        await samCliConfig.initialize()
-
-        assert.strictEqual(samCliConfig.getSamCliLocation(), fakeCliLocation)
-        assert.strictEqual(timesCalled, 1)
-    })
-
     it('calls location provider when config not set', async () => {
         let timesCalled: number = 0
 
@@ -108,7 +84,7 @@ describe('SamCliConfiguration', () => {
 
         await samCliConfig.initialize()
 
-        assert.strictEqual(samCliConfig.getSamCliLocation(), undefined)
+        assert.strictEqual(samCliConfig.getSamCliLocation(), '')
     })
 
     function createSampleFile(filename: string): void {

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

@@ -17,10 +17,14 @@ describe('DefaultSamCliValidator', async () => {
     class TestSamCliValidatorContext implements SamCliValidatorContext {
         public samCliVersionId: string = new Date().valueOf().toString()
         public getInfoCallCount: number = 0
-        public samCliLocation: string | undefined
+        public mockSamLocation: string = ''
 
         public constructor(public samCliVersion: string) {}
 
+        public async samCliLocation(): Promise<string> {
+            return this.mockSamLocation
+        }
+
         public async getSamCliExecutableId(): Promise<string> {
             return this.samCliVersionId
         }
@@ -65,7 +69,7 @@ describe('DefaultSamCliValidator', async () => {
         samCliVersionTestScenarios.forEach(test => {
             it(`handles case where SAM CLI exists and ${test.situation}`, async () => {
                 const validatorContext = new TestSamCliValidatorContext(test.version)
-                validatorContext.samCliLocation = 'somesamclipath'
+                validatorContext.mockSamLocation = 'somesamclipath'
                 const samCliValidator = new DefaultSamCliValidator(validatorContext)
 
                 const validatorResult = await samCliValidator.detectValidSamCli()
@@ -84,7 +88,7 @@ describe('DefaultSamCliValidator', async () => {
 
         it('handles case where SAM CLI is not found', async () => {
             const validatorContext = new TestSamCliValidatorContext('')
-            validatorContext.samCliLocation = undefined
+            validatorContext.mockSamLocation = ''
             const samCliValidator = new DefaultSamCliValidator(validatorContext)
 
             const validatorResult = await samCliValidator.detectValidSamCli()