Skip to content

Commit e3a7a49

Browse files
hyangahhyangah
committed
[release] src/goDebugConfiguration: do not merge toolExecutionEnvironment to 'env'
toolExecutionEnvironment is populated based on settings.json. The extension merged these env vars into 'env' automatically in order to make the separate legacy debug adapter process use the same env vars when invoking `dlv`. In dlv-dap mode, the extension host spawns dlv processes and can control the environment variables easily. Instead of mutating the launch parameter's env property by merging the toolExecutionEnvironment, this CL uses the env vars only when spawning the dlv process. This makes the debug adapter trace's launch message logging more compact and easier to share/inspect the request. Change-Id: I352528e4907e9b62e79c0f2b4d79f97fcd32c5dc Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/349749 Trust: Hyang-Ah Hana Kim <[email protected]> Run-TryBot: Hyang-Ah Hana Kim <[email protected]> TryBot-Result: kokoro <[email protected]> Reviewed-by: Suzy Mueller <[email protected]> (cherry picked from commit 97265e8) Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/349952 Trust: Suzy Mueller <[email protected]>
1 parent 03cb3c6 commit e3a7a49

File tree

5 files changed

+162
-14
lines changed

5 files changed

+162
-14
lines changed

‎src/goDebugConfiguration.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -372,12 +372,24 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr
372372
debugConfiguration: vscode.DebugConfiguration,
373373
token?: vscode.CancellationToken
374374
): vscode.DebugConfiguration {
375-
// Reads debugConfiguration.envFile and
376-
// combines the environment variables from all the env files and
377-
// debugConfiguration.env, on top of the tools execution environment variables.
378-
// It also unsets 'envFile' from the user-suppled debugConfiguration
375+
const debugAdapter = debugConfiguration['debugAdapter'];
376+
if (debugAdapter === '') {
377+
return null;
378+
}
379+
380+
// Read debugConfiguration.envFile and
381+
// combine the environment variables from all the env files and
382+
// debugConfiguration.env.
383+
// We also unset 'envFile' from the user-suppled debugConfiguration
379384
// because it is already applied.
380-
const goToolsEnvVars = toolExecutionEnvironment(folder?.uri); // also includes GOPATH: getCurrentGoPath().
385+
//
386+
// For legacy mode, we merge the environment variables on top of
387+
// the tools execution environment variables and update the debugConfiguration
388+
// because VS Code directly handles launch of the legacy debug adapter.
389+
// For dlv-dap mode, we do not merge the tools execution environment
390+
// variables here to reduce the number of environment variables passed
391+
// as launch/attach parameters.
392+
const goToolsEnvVars = debugAdapter === 'legacy' ? toolExecutionEnvironment(folder?.uri) : {};
381393
const fileEnvs = parseEnvFiles(debugConfiguration['envFile']);
382394
const env = debugConfiguration['env'] || {};
383395

@@ -387,7 +399,7 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr
387399
const entriesWithRelativePaths = ['cwd', 'output', 'program'].filter(
388400
(attr) => debugConfiguration[attr] && !path.isAbsolute(debugConfiguration[attr])
389401
);
390-
if (debugConfiguration['debugAdapter'] === 'dlv-dap') {
402+
if (debugAdapter === 'dlv-dap') {
391403
// relative paths -> absolute paths
392404
if (entriesWithRelativePaths.length > 0) {
393405
const workspaceRoot = folder?.uri.fsPath;

‎src/goDebugFactory.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { getTool } from './goTools';
1616
import { Logger, TimestampedLogger } from './goLogging';
1717
import { DebugProtocol } from 'vscode-debugprotocol';
1818
import { getWorkspaceFolderPath } from './util';
19+
import { toolExecutionEnvironment } from './goEnv';
1920

2021
export class GoDebugAdapterDescriptorFactory implements vscode.DebugAdapterDescriptorFactory {
2122
constructor(private outputChannel?: vscode.OutputChannel) {}
@@ -342,7 +343,9 @@ function spawnDlvDapServerProcess(
342343
logConsole: (msg: string) => void
343344
): Promise<ChildProcess> {
344345
const launchArgsEnv = launchAttachArgs.env || {};
345-
const env = Object.assign({}, process.env, launchArgsEnv);
346+
const goToolsEnvVars = toolExecutionEnvironment();
347+
// launchArgsEnv is user-requested env vars (envFiles + env).
348+
const env = Object.assign(goToolsEnvVars, launchArgsEnv);
346349

347350
const dlvPath = launchAttachArgs.dlvToolPath ?? getTool('dlv-dap');
348351

‎test/integration/goDebug.test.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import * as net from 'net';
1313
import * as path from 'path';
1414
import * as sinon from 'sinon';
1515
import * as proxy from '../../src/goDebugFactory';
16+
import * as vscode from 'vscode';
1617
import { DebugConfiguration, DebugProtocolMessage } from 'vscode';
1718
import { DebugClient } from 'vscode-debugadapter-testsupport';
1819
import { ILocation } from 'vscode-debugadapter-testsupport/lib/debugClient';
@@ -24,6 +25,7 @@ import {
2425
PackageBuildInfo,
2526
RemoteSourcesAndPackages
2627
} from '../../src/debugAdapter/goDebug';
28+
import * as extConfig from '../../src/config';
2729
import { GoDebugConfigurationProvider } from '../../src/goDebugConfiguration';
2830
import { getBinPath, rmdirRecursive } from '../../src/util';
2931
import { killProcessTree } from '../../src/utils/processUtils';
@@ -552,6 +554,70 @@ const testAll = (ctx: Mocha.Context, isDlvDap: boolean) => {
552554
});
553555
});
554556

557+
suite('env', () => {
558+
let sandbox: sinon.SinonSandbox;
559+
560+
setup(() => {
561+
sandbox = sinon.createSandbox();
562+
});
563+
teardown(async () => sandbox.restore());
564+
565+
test('env var from go.toolsEnvVars is respected', async () => {
566+
const PROGRAM = path.join(DATA_ROOT, 'envTest');
567+
const FILE = path.join(PROGRAM, 'main.go');
568+
const BREAKPOINT_LINE = 10;
569+
570+
const goConfig = Object.create(vscode.workspace.getConfiguration('go'), {
571+
toolsEnvVars: {
572+
value: { FOO: 'BAR' }
573+
}
574+
});
575+
const configStub = sandbox.stub(extConfig, 'getGoConfig').returns(goConfig);
576+
577+
const config = {
578+
name: 'Launch',
579+
type: 'go',
580+
request: 'launch',
581+
mode: 'debug',
582+
program: PROGRAM,
583+
args: ['FOO']
584+
};
585+
const debugConfig = await initializeDebugConfig(config);
586+
await dc.hitBreakpoint(debugConfig, getBreakpointLocation(FILE, BREAKPOINT_LINE));
587+
await assertLocalVariableValue('v', '"BAR"');
588+
589+
await dc.continueRequest({ threadId: 1 }); // continue until completion for cleanup.
590+
});
591+
592+
test('env var from launch config is respected', async () => {
593+
const PROGRAM = path.join(DATA_ROOT, 'envTest');
594+
const FILE = path.join(PROGRAM, 'main.go');
595+
const BREAKPOINT_LINE = 10;
596+
597+
const goConfig = Object.create(vscode.workspace.getConfiguration('go'), {
598+
toolsEnvVars: {
599+
value: { FOO: 'BAR' }
600+
}
601+
});
602+
const configStub = sandbox.stub(extConfig, 'getGoConfig').returns(goConfig);
603+
604+
const config = {
605+
name: 'Launch',
606+
type: 'go',
607+
request: 'launch',
608+
mode: 'debug',
609+
program: PROGRAM,
610+
args: ['FOO'],
611+
env: { FOO: 'BAZ' }
612+
};
613+
const debugConfig = await initializeDebugConfig(config);
614+
await dc.hitBreakpoint(debugConfig, getBreakpointLocation(FILE, BREAKPOINT_LINE));
615+
await assertLocalVariableValue('v', '"BAZ"');
616+
617+
await dc.continueRequest({ threadId: 1 }); // continue until completion for cleanup.
618+
});
619+
});
620+
555621
suite('launch', () => {
556622
test('should run program to the end', async () => {
557623
const PROGRAM = path.join(DATA_ROOT, 'baseTest');

‎test/integration/goDebugConfiguration.test.ts‎

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ suite('Debug Environment Variable Merge Test', () => {
4545
});
4646

4747
interface Input {
48+
debugAdapter?: 'dlv-dap' | 'legacy';
4849
env?: { [key: string]: any };
4950
envFile?: string | string[];
5051
toolsEnv?: { [key: string]: any };
@@ -57,7 +58,8 @@ suite('Debug Environment Variable Merge Test', () => {
5758
name: 'Launch',
5859
request: 'launch',
5960
env: input.env,
60-
envFile: input.envFile
61+
envFile: input.envFile,
62+
debugAdapter: input.debugAdapter
6163
});
6264

6365
const actual = config.env;
@@ -68,21 +70,39 @@ suite('Debug Environment Variable Merge Test', () => {
6870
runTest({}, {});
6971
});
7072

71-
test('toolsEnvVars is propagated', () => {
73+
test('toolsEnvVars is propagated (legacy)', () => {
74+
const debugAdapter = 'legacy';
7275
const toolsEnv = {
7376
GOPATH: '/gopath',
7477
GOOS: 'valueFromToolsEnv'
7578
};
7679

7780
runTest(
78-
{ toolsEnv },
81+
{
82+
debugAdapter,
83+
toolsEnv
84+
},
7985
{
8086
GOPATH: '/gopath',
8187
GOOS: 'valueFromToolsEnv'
8288
}
8389
);
8490
});
8591

92+
test('toolsEnvVars is not propagated', () => {
93+
const toolsEnv = {
94+
GOPATH: '/gopath',
95+
GOOS: 'valueFromToolsEnv'
96+
};
97+
98+
runTest(
99+
{
100+
toolsEnv
101+
},
102+
{}
103+
);
104+
});
105+
86106
test('preserves settings from launchArgs.env', () => {
87107
const env = { GOPATH: 'valueFromEnv', GOOS: 'valueFromEnv2' };
88108
runTest(
@@ -114,16 +134,17 @@ suite('Debug Environment Variable Merge Test', () => {
114134
);
115135
});
116136

117-
test('launchArgs.env overwrites toolsEnvVar', () => {
137+
test('launchArgs.env overwrites toolsEnvVar (legacy)', () => {
118138
const toolsEnv = {
119139
GOPATH: '/gopath',
120140
SOMEVAR1: 'valueFromToolsEnvVar1',
121141
SOMEVAR2: 'valueFromToolsEnvVar2'
122142
};
123143

144+
const debugAdapter = 'legacy';
124145
const env = { SOMEVAR1: 'valueFromEnv' };
125146
runTest(
126-
{ env, toolsEnv },
147+
{ debugAdapter, env, toolsEnv },
127148
{
128149
GOPATH: '/gopath',
129150
SOMEVAR1: 'valueFromEnv',
@@ -132,7 +153,23 @@ suite('Debug Environment Variable Merge Test', () => {
132153
);
133154
});
134155

135-
test('launchArgs.envFile overwrites toolsEnvVar', () => {
156+
test('launchArgs.env is respected, toolsEnvVar is ignored (dlv-dap)', () => {
157+
const toolsEnv = {
158+
GOPATH: '/gopath',
159+
SOMEVAR1: 'valueFromToolsEnvVar1',
160+
SOMEVAR2: 'valueFromToolsEnvVar2'
161+
};
162+
163+
const env = { SOMEVAR1: 'valueFromEnv' };
164+
runTest(
165+
{ env, toolsEnv },
166+
{
167+
SOMEVAR1: 'valueFromEnv'
168+
}
169+
);
170+
});
171+
172+
test('launchArgs.envFile overwrites toolsEnvVar (legacy)', () => {
136173
const toolsEnv = {
137174
GOPATH: '/gopath',
138175
SOMEVAR1: 'valueFromToolsEnvVar1',
@@ -141,15 +178,34 @@ suite('Debug Environment Variable Merge Test', () => {
141178
const envFile = path.join(tmpDir, 'env');
142179
fs.writeFileSync(envFile, ['SOMEVAR2=valueFromEnvFile2'].join('\n'));
143180

181+
const debugAdapter = 'legacy';
144182
runTest(
145-
{ toolsEnv, envFile },
183+
{ debugAdapter, toolsEnv, envFile },
146184
{
147185
GOPATH: '/gopath',
148186
SOMEVAR1: 'valueFromToolsEnvVar1',
149187
SOMEVAR2: 'valueFromEnvFile2'
150188
}
151189
);
152190
});
191+
192+
test('launchArgs.envFile is repected, and toolsEnvVar is ignored (dlv-dap)', () => {
193+
const toolsEnv = {
194+
GOPATH: '/gopath',
195+
SOMEVAR1: 'valueFromToolsEnvVar1',
196+
SOMEVAR2: 'valueFromToolsEnvVar2'
197+
};
198+
const envFile = path.join(tmpDir, 'env');
199+
fs.writeFileSync(envFile, ['SOMEVAR2=valueFromEnvFile2'].join('\n'));
200+
201+
const debugAdapter = 'dlv-dap';
202+
runTest(
203+
{ debugAdapter, toolsEnv, envFile },
204+
{
205+
SOMEVAR2: 'valueFromEnvFile2'
206+
}
207+
);
208+
});
153209
});
154210

155211
suite('Debug Configuration Merge User Settings', () => {

‎test/testdata/envTest/main.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package main
2+
3+
import "os"
4+
5+
func main() {
6+
if len(os.Args) != 2 {
7+
os.Exit(1)
8+
}
9+
v := os.Getenv(os.Args[1])
10+
println(v)
11+
}

0 commit comments

Comments
 (0)