From c7c4188bb9bf048aeef803995ac94c582f435faa Mon Sep 17 00:00:00 2001 From: "Hana (Hyang-Ah) Kim" Date: Fri, 14 Aug 2020 19:40:14 -0400 Subject: [PATCH] src/goDebugConfiguration: combine envFile and env With the resolveDebugConfigurationWithSubstitutedVariables, we can read envFile and combine it into the env property from the extension host. Delve DAP will handle only the env arg and not have any special handling for envFile. So, processing it from the extension side makes more sense for long term. This move allows us to centralize the .env file read support. For backwards compatibility, I left the logic in the old DA but removed it from the new delve DAP DA. Fixes golang/vscode-go#452 Updates golang/vscode-go#23 Change-Id: I641eb2e62051985ba3486901483ad796256aba2c Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/248659 Run-TryBot: Hyang-Ah Hana Kim TryBot-Result: kokoro Reviewed-by: Polina Sokolova --- src/debugAdapter/goDebug.ts | 8 +- src/debugAdapter2/goDlvDebug.ts | 10 +- src/goDebugConfiguration.ts | 31 +++--- src/goEnv.ts | 4 +- test/integration/goDebugConfiguration.test.ts | 100 ++++++++++++++++++ 5 files changed, 130 insertions(+), 23 deletions(-) create mode 100644 test/integration/goDebugConfiguration.test.ts diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index b6dd86e3dc..f669ac5d0d 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -259,8 +259,6 @@ interface LaunchRequestArguments extends DebugProtocol.LaunchRequestArguments { buildFlags?: string; init?: string; trace?: 'verbose' | 'log' | 'error'; - /** Optional path to .env file. */ - envFile?: string | string[]; backend?: string; output?: string; /** Delve LoadConfig parameters */ @@ -273,6 +271,12 @@ interface LaunchRequestArguments extends DebugProtocol.LaunchRequestArguments { showGlobalVariables?: boolean; packagePathToGoModPathMap: { [key: string]: string }; + + /** Optional path to .env file. */ + // TODO: deprecate .env file processing from DA. + // We expect the extension processes .env files + // and send the information to DA using the 'env' property. + envFile?: string | string[]; } interface AttachRequestArguments extends DebugProtocol.AttachRequestArguments { diff --git a/src/debugAdapter2/goDlvDebug.ts b/src/debugAdapter2/goDlvDebug.ts index c861f817c1..28676fc78f 100644 --- a/src/debugAdapter2/goDlvDebug.ts +++ b/src/debugAdapter2/goDlvDebug.ts @@ -20,7 +20,6 @@ import { TerminatedEvent } from 'vscode-debugadapter'; import { DebugProtocol } from 'vscode-debugprotocol'; -import { parseEnvFiles } from '../utils/envUtils'; import { envPath, getBinPathWithPreferredGopathGoroot } from '../utils/goPath'; import { killProcessTree } from '../utils/processUtils'; @@ -57,8 +56,6 @@ interface LaunchRequestArguments extends DebugProtocol.LaunchRequestArguments { buildFlags?: string; init?: string; trace?: 'verbose' | 'log' | 'error'; - /** Optional path to .env file. */ - envFile?: string | string[]; backend?: string; output?: string; /** Delve LoadConfig parameters */ @@ -610,10 +607,11 @@ export class GoDlvDapDebugSession extends LoggingDebugSession { goRunArgs.push(...launchArgs.args); } - // Read env from disk and merge into env variables. - const fileEnvs = parseEnvFiles(launchArgs.envFile); + // launchArgs.env includes all the environment variables + // including vscode-go's toolsExecutionEnvironment (PATH, GOPATH, ...), + // and those read from .env files. const launchArgsEnv = launchArgs.env || {}; - const programEnv = Object.assign({}, process.env, fileEnvs, launchArgsEnv); + const programEnv = Object.assign({}, process.env, launchArgsEnv); log(`Current working directory: ${dirname}`); const goExe = getBinPathWithPreferredGopathGoroot('go', []); diff --git a/src/goDebugConfiguration.ts b/src/goDebugConfiguration.ts index a75b847089..983269f135 100644 --- a/src/goDebugConfiguration.ts +++ b/src/goDebugConfiguration.ts @@ -12,6 +12,7 @@ import { promptForMissingTool } from './goInstallTools'; import { packagePathToGoModPathMap } from './goModules'; import { getFromGlobalState, updateGlobalState } from './stateUtils'; import { getBinPath, getCurrentGoPath, getGoConfig } from './util'; +import { parseEnvFiles } from './utils/envUtils'; export class GoDebugConfigurationProvider implements vscode.DebugConfigurationProvider { constructor(private defaultDebugAdapterType: string = 'go') { } @@ -60,20 +61,9 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr debugConfiguration['packagePathToGoModPathMap'] = packagePathToGoModPathMap; - const gopath = getCurrentGoPath(folder ? folder.uri : undefined); - if (!debugConfiguration['env']) { - debugConfiguration['env'] = { GOPATH: gopath }; - } else if (!debugConfiguration['env']['GOPATH']) { - debugConfiguration['env']['GOPATH'] = gopath; - } - const goConfig = getGoConfig(folder && folder.uri); - const goToolsEnvVars = toolExecutionEnvironment(); - Object.keys(goToolsEnvVars).forEach((key) => { - if (!debugConfiguration['env'].hasOwnProperty(key)) { - debugConfiguration['env'][key] = goToolsEnvVars[key]; - } - }); + + combineEnvFilesAndEnv(folder, debugConfiguration); const dlvConfig = goConfig.get('delveConfig'); let useApiV1 = false; @@ -146,3 +136,18 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr }); } } + +// combineEnvFilesAndEnv reads debugConfiguration.envFile and +// combines the environment variables from all the env files and +// debugConfiguration.env, on top of the tools execution environment variables. +// It also unsets 'envFile' from the user-suppled debugConfiguration +// because it is already applied. +function combineEnvFilesAndEnv( + folder: vscode.WorkspaceFolder, debugConfiguration: vscode.DebugConfiguration) { + const goToolsEnvVars = toolExecutionEnvironment(folder?.uri); // also includes GOPATH: getCurrentGoPath(). + const fileEnvs = parseEnvFiles(debugConfiguration['envFile']); + const env = debugConfiguration['env'] || {}; + + debugConfiguration['env'] = Object.assign(goToolsEnvVars, fileEnvs, env); + debugConfiguration['envFile'] = undefined; // unset, since we already processed. +} diff --git a/src/goEnv.ts b/src/goEnv.ts index 0372895278..556686a43f 100644 --- a/src/goEnv.ts +++ b/src/goEnv.ts @@ -44,9 +44,9 @@ export function toolInstallationEnvironment(): NodeJS.Dict { // toolExecutionEnvironment returns the environment in which tools should // be executed. It always returns a new object. -export function toolExecutionEnvironment(): NodeJS.Dict { +export function toolExecutionEnvironment(uri?: vscode.Uri): NodeJS.Dict { const env = newEnvironment(); - const gopath = getCurrentGoPath(); + const gopath = getCurrentGoPath(uri); if (gopath) { env['GOPATH'] = gopath; } diff --git a/test/integration/goDebugConfiguration.test.ts b/test/integration/goDebugConfiguration.test.ts new file mode 100644 index 0000000000..595ba4550b --- /dev/null +++ b/test/integration/goDebugConfiguration.test.ts @@ -0,0 +1,100 @@ +import assert = require('assert'); +import fs = require('fs'); +import os = require('os'); +import path = require('path'); +import sinon = require('sinon'); +import vscode = require('vscode'); +import { GoDebugConfigurationProvider } from '../../src/goDebugConfiguration'; +import goEnv = require('../../src/goEnv'); +import { updateGoVarsFromConfig } from '../../src/goInstallTools'; +import { getCurrentGoPath, rmdirRecursive } from '../../src/util'; + +suite('Debug Environment Variable Merge Test', () => { + const debugConfigProvider = new GoDebugConfigurationProvider(); + + suiteSetup(async () => { + await updateGoVarsFromConfig(); + + // Set up the test fixtures. + const fixtureSourcePath = path.join(__dirname, '..', '..', '..', 'test', 'fixtures'); + const filePath = path.join(fixtureSourcePath, 'baseTest', 'test.go'); + await vscode.workspace.openTextDocument(vscode.Uri.file(filePath)); + }); + + suiteTeardown(() => { + vscode.commands.executeCommand('workbench.action.closeActiveEditor'); + }); + + let sandbox: sinon.SinonSandbox; + let tmpDir = ''; + const toolExecutionEnv: NodeJS.Dict = {}; + setup(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'godebugconfig_test')); + sandbox = sinon.createSandbox(); + + }); + + teardown(() => { + sandbox.restore(); + rmdirRecursive(tmpDir); + }); + + interface Input { + env?: { [key: string]: any }; + envFile?: string|string[]; + toolsEnv?: { [key: string]: any}; + } + + function runTest(input: Input, expected: { [key: string]: any}) { + sandbox.stub(goEnv, 'toolExecutionEnvironment').returns(input.toolsEnv || {}); + const config = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(undefined, { + type: 'go', + name: 'Launch', + request: 'launch', + env: input.env, + envFile: input.envFile, + }); + + const actual = config.env; + assert.deepStrictEqual(actual, expected); + } + + test('works with empty launchArgs', () => { + runTest({}, {}); + }); + + test('toolsEnvVars is propagated', () => { + const toolsEnv = { + GOPATH: '/gopath', + GOOS: 'valueFromToolsEnv'}; + + runTest({toolsEnv}, { + GOPATH: '/gopath', + GOOS: 'valueFromToolsEnv'}); + }); + + test('preserves settings from launchArgs.env', () => { + const env = {GOPATH: 'valueFromEnv', GOOS: 'valueFromEnv2'}; + runTest({env}, { + GOPATH: 'valueFromEnv', + GOOS: 'valueFromEnv2'}); + }); + + test('preserves settings from launchArgs.envFile', () => { + const envFile = path.join(tmpDir, 'env'); + fs.writeFileSync(envFile, 'GOPATH=valueFromEnvFile'); + runTest({envFile}, {GOPATH: 'valueFromEnvFile'}); + }); + + test('launchArgs.env overwrites launchArgs.envFile', () => { + const env = {SOMEVAR1: 'valueFromEnv'}; + const envFile = path.join(tmpDir, 'env'); + fs.writeFileSync(envFile, [ + 'SOMEVAR1=valueFromEnvFile1', + 'SOMEVAR2=valueFromEnvFile2'].join('\n')); + + runTest({ env, envFile }, { + SOMEVAR1: 'valueFromEnv', + SOMEVAR2: 'valueFromEnvFile2'}); + }); +});