Skip to content

Commit

Permalink
src/goDebugConfiguration: massage launch config for debug/test
Browse files Browse the repository at this point in the history 
When the current build directory is resolved to a path
different from the program's directory due to soft/symbolic links,
the go command can be confused and complain that the absolute
path in program is outside the module of the current directory.

This CL avoids such problem by making dlv always use a relative
path as the build target for build/test mode (and auto for testing).
Before starting the debug session, we massage the launch config:

  program: /foo/bar.go -> program: ./bar.go, __buildDir: /foo
  program: /foo/bar -> program: ., __buildDir: /foo/bar

Previously we find the package directory just before spawning
dlv dap and spawn the dlv dap process from the package directory.
With this CL, we find the package directory when resolving
the debug configuration before debug session.

This introduces __buildDir attribute which is internal.
(There is an ongoing work to introduce 'buildDir' to dlv DAP
so we use internal attribute to make it clear.)

Also, this made the resolveDebugConfigurationWithSubstitutedVariables
accept relative paths without workspace root. Just the behavior
is undefined. (The motivation of change in this part is
the testing. We have 'output' property or some others, that
are like relative path. I guess most users wouldn't care much.
Delve is cool with relative paths so we do our best to
resolve that wrt the workspace root.
Otherwise, just let delve decide.

Updates golang#1713

Change-Id: I434c43131b27d9c58058450c502e1b30c58ea690
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/344790
Trust: Hyang-Ah Hana Kim <[email protected]>
Trust: Suzy Mueller <[email protected]>
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>
  • Loading branch information
@hyangah
hyangah committed Aug 27, 2021
1 parent 289839c commit aa3293c
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 48 deletions.
50 changes: 35 additions & 15 deletions src/goDebugConfiguration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import path = require('path');
import vscode = require('vscode');
import { getGoConfig } from './config';
import { parseProgramArgSync } from './goDebugFactory';
import { toolExecutionEnvironment } from './goEnv';
import {
declinedToolInstall,
Expand Down Expand Up @@ -243,8 +244,6 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr

const dlvToolPath = getBinPath(debugAdapter);
if (!path.isAbsolute(dlvToolPath)) {
const tool = getTool(debugAdapter);

// If user has not already declined to install this tool,
// prompt for it. Otherwise continue and have the lack of
// dlv binary be caught later.
Expand Down Expand Up @@ -382,28 +381,49 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr
const entriesWithRelativePaths = ['cwd', 'output', 'program'].filter(
(attr) => debugConfiguration[attr] && !path.isAbsolute(debugConfiguration[attr])
);
if (debugConfiguration['debugAdapter'] === 'dlv-dap' && entriesWithRelativePaths.length > 0) {
const workspaceRoot = folder?.uri.fsPath;
if (!workspaceRoot) {
this.showWarning(
'relativePathsWithoutWorkspaceFolder',
'Relative paths without a workspace folder for `cwd`, `program`, or `output` are not allowed.'
);
return null;
if (debugConfiguration['debugAdapter'] === 'dlv-dap') {
// relative paths -> absolute paths
if (entriesWithRelativePaths.length > 0) {
const workspaceRoot = folder?.uri.fsPath;
if (workspaceRoot) {
entriesWithRelativePaths.forEach((attr) => {
debugConfiguration[attr] = path.join(workspaceRoot, debugConfiguration[attr]);
});
} else {
this.showWarning(
'relativePathsWithoutWorkspaceFolder',
'Behavior when using relative paths without a workspace folder for `cwd`, `program`, or `output` is undefined.'
);
}
}
// compute build dir, and translate the dirname in program to a path relative to buildDir.
if (debugConfiguration.request === 'launch') {
const mode = debugConfiguration['mode'] || 'debug';
if (['debug', 'test', 'auto'].includes(mode)) {
// Massage config to build the target from the package directory
// with a relative path. (https://github.com/golang/vscode-go/issues/1713)
try {
const { program, dirname, programIsDirectory } = parseProgramArgSync(debugConfiguration);
if (dirname) {
debugConfiguration['__buildDir'] = dirname;
debugConfiguration['program'] = programIsDirectory
? '.'
: '.' + path.sep + path.relative(dirname, program);
}
} catch (e) {
this.showWarning('invalidProgramArg', `Invalid 'program': ${e}`);
// keep going - just in case dlv knows how to handle this better.
}
}
}
entriesWithRelativePaths.forEach((attr) => {
debugConfiguration[attr] = path.join(workspaceRoot, debugConfiguration[attr]);
});
}

if (debugConfiguration.request === 'attach' && debugConfiguration['mode'] === 'local') {
// processId needs to be an int, but the substituted variables from pickGoProcess and pickProcess
// become a string. Convert any strings to integers.
if (typeof debugConfiguration['processId'] === 'string') {
debugConfiguration['processId'] = parseInt(debugConfiguration['processId'], 10);
}
}

return debugConfiguration;
}

Expand Down
55 changes: 31 additions & 24 deletions src/goDebugFactory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export class GoDebugAdapterDescriptorFactory implements vscode.DebugAdapterDescr
configuration: vscode.DebugConfiguration
): Promise<vscode.ProviderResult<vscode.DebugAdapterDescriptor>> {
const logger = new TimestampedLogger(configuration.trace, this.outputChannel);
logger.debug(`Config: ${JSON.stringify(configuration)}`);
const d = new DelveDAPOutputAdapter(configuration, logger);
return new vscode.DebugAdapterInlineImplementation(d);
}
Expand Down Expand Up @@ -356,13 +357,9 @@ function spawnDlvDapServerProcess(
throw new Error('Cannot find Delve debugger (dlv dap)');
}
let dir = getWorkspaceFolderPath();
if (launchAttachArgs.request === 'launch') {
try {
dir = parseProgramArgSync(launchAttachArgs).dirname;
} catch (err) {
logErr(`Program arg: ${launchAttachArgs.program}\n${err}\n`);
throw err; // rethrow so the caller knows it failed.
}
if (launchAttachArgs.request === 'launch' && launchAttachArgs['__buildDir']) {
// __buildDir is the directory determined during resolving debug config
dir = launchAttachArgs['__buildDir'];
}

const dlvArgs = new Array<string>();
Expand Down Expand Up @@ -408,7 +405,7 @@ function spawnDlvDapServerProcess(

const logDestStream = logDest ? fs.createWriteStream(logDest) : undefined;

logConsole(`Starting: ${dlvPath} ${dlvArgs.join(' ')}\n`);
logConsole(`Starting: ${dlvPath} ${dlvArgs.join(' ')} from ${dir}\n`);

// TODO(hyangah): In module-module workspace mode, the program should be build in the super module directory
// where go.work (gopls.mod) file is present. Where dlv runs determines the build directory currently. Two options:
Expand Down Expand Up @@ -491,27 +488,37 @@ function spawnDlvDapServerProcess(
export function parseProgramArgSync(
launchAttachArgs: vscode.DebugConfiguration
): { program: string; dirname: string; programIsDirectory: boolean } {
const program = launchAttachArgs.program;
let programIsDirectory = false;
// attach request:
// irrelevant
if (launchAttachArgs.request !== 'launch') return;

if (launchAttachArgs.mode === 'replay') {
// Skip program parsing on modes that do not require a program
return { program: '', dirname: '', programIsDirectory: programIsDirectory };
}
const mode = launchAttachArgs.mode || 'debug';
const program = launchAttachArgs.program;

if (!program) {
throw new Error('The program attribute is missing in the debug configuration in launch.json');
}

try {
programIsDirectory = fs.lstatSync(program).isDirectory();
} catch (e) {
// TODO(hyangah): why can't the program be a package name?
throw new Error('The program attribute must point to valid directory, .go file or executable.');
}
if (!programIsDirectory && launchAttachArgs.mode !== 'exec' && path.extname(program) !== '.go') {
throw new Error('The program attribute must be a directory or .go file in debug and test mode');
// debug, test, auto mode in launch request:
// program ends with .go file -> file, otherwise -> programIsDirectory.
// exec mode
// program should be executable.
// other modes:
// not relevant
if (['debug', 'test', 'auto'].includes(mode)) {
// `auto` shouldn't happen other than in testing.
const ext = path.extname(program);
if (ext === '') {
// the last path element doesn't have . or the first char is .
// Treat this like a directory.
return { program, dirname: program, programIsDirectory: true };
}
if (ext === '.go') {
return { program, dirname: path.dirname(program), programIsDirectory: false };
} else {
throw new Error('The program attribute must be a directory or .go file in debug and test mode');
}
}
const dirname = programIsDirectory ? program : path.dirname(program);
return { program, dirname, programIsDirectory };
// Otherwise, let delve handle.
return { program, dirname: '', programIsDirectory: false };
}
42 changes: 41 additions & 1 deletion test/integration/goDebug.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2053,6 +2053,41 @@ const testAll = (ctx: Mocha.Context, isDlvDap: boolean) => {
await new Promise((resolve) => setTimeout(resolve, 2_000));
});
});

suite('substitutePath with symlink', () => {
let realPath: string;
let symlinkPath: string;

suiteSetup(() => {
realPath = copyDirectory('baseTest');
symlinkPath = path.join(tmpDir, 'symlinked');
fs.symlinkSync(realPath, symlinkPath, 'dir');
});
suiteTeardown(() => {
fs.unlinkSync(symlinkPath);
rmdirRecursive(realPath);
});
test('should stop on a breakpoint', async function () {
if (!isDlvDap) this.skip(); // BUG: the legacy adapter fails with 'breakpoint verification mismatch' error.
const FILE = path.join(symlinkPath, 'test.go');
const BREAKPOINT_LINE = 11;
const config = {
name: 'Launch',
type: 'go',
request: 'launch',
mode: 'debug',
program: FILE,
substitutePath: [
{
from: symlinkPath,
to: realPath
}
]
};
const debugConfig = await initializeDebugConfig(config);
await dc.hitBreakpoint(debugConfig, getBreakpointLocation(FILE, BREAKPOINT_LINE));
});
});
});

let testNumber = 0;
Expand Down Expand Up @@ -2082,7 +2117,12 @@ const testAll = (ctx: Mocha.Context, isDlvDap: boolean) => {
}
testNumber++;

const debugConfig = await debugConfigProvider.resolveDebugConfiguration(undefined, config);
let debugConfig = await debugConfigProvider.resolveDebugConfiguration(undefined, config);
debugConfig = await debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(
undefined,
debugConfig
);

if (isDlvDap) {
dlvDapAdapter = await DelveDAPDebugAdapterOnSocket.create(debugConfig);
const port = await dlvDapAdapter.serve();
Expand Down
56 changes: 48 additions & 8 deletions test/integration/goDebugConfiguration.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,23 +452,50 @@ suite('Debug Configuration Converts Relative Paths', () => {
};
}

test('resolve relative paths with workspace root in dlv-dap mode', () => {
test('resolve relative paths with workspace root in dlv-dap mode, exec mode does not set __buildDir', () => {
const config = debugConfig('dlv-dap');
config.mode = 'exec';
config.program = path.join('foo', 'bar.exe');
const workspaceFolder = {
uri: vscode.Uri.file(os.tmpdir()),
name: 'test',
index: 0
};
const { program, cwd, output } = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(
const { program, cwd, __buildDir } = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(
workspaceFolder,
config
);
assert.deepStrictEqual(
{ program, cwd, output },
{ program, cwd, __buildDir },
{
program: path.join(os.tmpdir(), 'foo', 'bar.exe'),
cwd: os.tmpdir(),
__buildDir: undefined
}
);
});

test('program and __buildDir are updated while resolving debug configuration in dlv-dap mode', () => {
const config = debugConfig('dlv-dap');
config.program = path.join('foo', 'bar', 'pkg');
const workspaceFolder = {
uri: vscode.Uri.file(os.tmpdir()),
name: 'test',
index: 0
};
const {
program,
cwd,
output,
__buildDir
} = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(workspaceFolder, config);
assert.deepStrictEqual(
{ program, cwd, output, __buildDir },
{
program: path.join(os.tmpdir(), 'foo/bar.go'),
program: '.',
cwd: os.tmpdir(),
output: path.join(os.tmpdir(), 'debug')
output: path.join(os.tmpdir(), 'debug'),
__buildDir: path.join(os.tmpdir(), 'foo', 'bar', 'pkg')
}
);
});
Expand Down Expand Up @@ -498,10 +525,23 @@ suite('Debug Configuration Converts Relative Paths', () => {
);
});

test('disallow relative paths with no workspace root', () => {
test('relative paths with no workspace root are not expanded', () => {
const config = debugConfig('dlv-dap');
const got = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(undefined, config);
assert.strictEqual(got, null);
const {
program,
cwd,
output,
__buildDir
} = debugConfigProvider.resolveDebugConfigurationWithSubstitutedVariables(undefined, config);
assert.deepStrictEqual(
{ program, cwd, output, __buildDir },
{
program: '.' + path.sep + 'bar.go',
cwd: '.',
output: 'debug',
__buildDir: 'foo'
}
);
});

test('do not affect relative paths (workspace) in legacy mode', () => {
Expand Down

0 comments on commit aa3293c

Please sign in to comment.