Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

Fixes #2122 tools are also found at GOBIN path#3001

Merged
ramya-rao-a merged 4 commits intomicrosoft:masterfrom
marcel-basel:tools-missing-when-gobin-2122
Jan 28, 2020
Merged

Fixes #2122 tools are also found at GOBIN path#3001
ramya-rao-a merged 4 commits intomicrosoft:masterfrom
marcel-basel:tools-missing-when-gobin-2122

Conversation

@marcel-basel
Copy link
Copy Markdown
Contributor

If go.toolsGopath setting is set, goInstallTools.installTools was already setting GOBIN env to ''. With no go.toolsGopath set but GOBIN defined the tools are installed in GOBIN path already. The log message had to be corrected (Installing x tools at ...).

GOBIN was added to the list of places where the extension looks for the tools

@msftclas
Copy link
Copy Markdown

msftclas commented Jan 25, 2020

CLA assistant check
All CLA requirements met.

Comment thread src/goPath.ts Outdated
}

const binname = alternateTool && !path.isAbsolute(alternateTool) ? alternateTool : toolName;
const env = Object.assign({}, process.env);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for making a copy of the env variables?
Since we are only reading the env var values, process.env can be directly used.

Comment thread src/goPath.ts Outdated

const binname = alternateTool && !path.isAbsolute(alternateTool) ? alternateTool : toolName;
const env = Object.assign({}, process.env);
if (env['GOBIN'] != undefined){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if check can be removed. If no GOBIN is set, then getBinPathFromEnvVar() will return null anyway.

Comment thread src/goPath.ts Outdated
const binname = alternateTool && !path.isAbsolute(alternateTool) ? alternateTool : toolName;
const env = Object.assign({}, process.env);
if (env['GOBIN'] != undefined){
const path = getBinPathFromEnvVar(binname, env['GOBIN'], false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to pathFromGoBin to match the patterns of pathFromGoRoot and pathFromPath names used later on

@marcel-basel
Copy link
Copy Markdown
Contributor Author

Commited the requested changes

Comment thread src/goInstallTools.ts Outdated
}bin`;
let binpath = toolsGopath + path.sep + "bin";
if(envForTools['GOBIN'] != undefined && envForTools['GOBIN'] != ''){
binpath = "GOBIN path " + envForTools['GOBIN']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GOBIN is set, then we can essentially skip the code path

} else {
toolsGopath = getCurrentGoPath();
outputChannel.appendLine(`go.toolsGopath setting is not set. Using GOPATH ${toolsGopath}`);
}
if (toolsGopath) {
const paths = toolsGopath.split(path.delimiter);
toolsGopath = paths[0];
envForTools['GOPATH'] = toolsGopath;
} else {
const msg = 'Cannot install Go tools. Set either go.gopath or go.toolsGopath in settings.';
vscode.window.showInformationMessage(msg, 'Open User Settings', 'Open Workspace Settings').then((selected) => {
switch (selected) {
case 'Open User Settings':
vscode.commands.executeCommand('workbench.action.openGlobalSettings');
break;
case 'Open Workspace Settings':
vscode.commands.executeCommand('workbench.action.openWorkspaceSettings');
break;
}
});
return;
}
right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have to set the current go path envForTools['GOPATH'] = toolsGopath; since envForTools is passed as options when go get child process is created. GOBIN is only setting the path where the executable should be installed. Right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm fair point

@ramya-rao-a
Copy link
Copy Markdown
Contributor

Also, there are some linting errors that need to be fixed.

Screen Shot 2020-01-27 at 4 59 15 PM

Run npm run lint to find the above errors
Run npm run fix-lint to fix the automatically fixable linting errors

@ramya-rao-a ramya-rao-a force-pushed the tools-missing-when-gobin-2122 branch from 1f5fa4f to 83e3360 Compare January 28, 2020 22:38
@ramya-rao-a ramya-rao-a merged commit fbb180d into microsoft:master Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants