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

Make tools re-compiling warnings scoped by toolsGopath #1589

Merged
merged 3 commits into from
Mar 28, 2019
Merged

Make tools re-compiling warnings scoped by toolsGopath #1589

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e92637b
Make tools re-compiling warnings scoped by toolsGopath
FiloSottile Mar 22, 2018
5045ded
Apply pascal case to interface name
FiloSottile Mar 23, 2018
d9fce63
Minor fixes
Mar 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 30 additions & 21 deletions src/goMain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,36 +66,45 @@ export function activate(ctx: vscode.ExtensionContext): void {
let useLangServer = vscode.workspace.getConfiguration('go')['useLanguageServer'];
setGlobalState(ctx.globalState);

updateGoPathGoRootFromConfig().then(() => {
updateGoPathGoRootFromConfig().then(async () => {
const updateToolsCmdText = 'Update tools';
const prevGoroot = ctx.globalState.get('goroot');
const currentGoroot = process.env['GOROOT'];
if (prevGoroot !== currentGoroot && prevGoroot) {
vscode.window.showInformationMessage('Your goroot is different than before, a few Go tools may need recompiling', updateToolsCmdText).then(selected => {
interface GoInfo {
goroot: string;
version: string;
}
const toolsGoInfo: { [id: string]: GoInfo; } = ctx.globalState.get('toolsGoInfo') || {};
const toolsGopath = getToolsGopath() || getCurrentGoPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

In Go versions less than 1.8, it is possible that gopath will not be set, so toolsGopath here would be null or undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

@ramya-rao-a is this still a concern given that 1.12 is out and we dont test for 1.8 anymore

Copy link
Member

Choose a reason for hiding this comment

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

Only thing that comes to mind is the future when modules have taken over and GOPATH becomes obsolete. It's a larger issue though and I don't think it needs to hold up this PR.

if (!toolsGoInfo[toolsGopath]) {
toolsGoInfo[toolsGopath] = { goroot: null, version: null };
}
const prevGoroot = toolsGoInfo[toolsGopath].goroot;
vladbarosan marked this conversation as resolved.
Show resolved Hide resolved
const currentGoroot: string = process.env['GOROOT'];
if (prevGoroot && prevGoroot !== currentGoroot) {
vscode.window.showInformationMessage(`Your current goroot (${currentGoroot}) is different than before (${prevGoroot}), a few Go tools may need recompiling`, updateToolsCmdText).then(selected => {
if (selected === updateToolsCmdText) {
installAllTools(true);
}
});
} else {
getGoVersion().then(currentVersion => {
if (currentVersion) {
const prevVersion = ctx.globalState.get('goVersion');
const currVersionString = `${currentVersion.major}.${currentVersion.minor}`;

if (prevVersion !== currVersionString) {
if (prevVersion) {
vscode.window.showInformationMessage('Your Go version is different than before, few Go tools may need re-compiling', updateToolsCmdText).then(selected => {
if (selected === updateToolsCmdText) {
installAllTools(true);
}
});
}
ctx.globalState.update('goVersion', currVersionString);
const currentVersion = await getGoVersion();
if (currentVersion) {
const prevVersion = toolsGoInfo[toolsGopath].version;
Copy link
Contributor

Choose a reason for hiding this comment

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

When this update goes out, prevVersion will be null for the first time, and users will get the prompt for recompiling.
We need a null check for prevVersion just like we do for prevGoroot

Copy link
Contributor

Choose a reason for hiding this comment

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

yess, good catch

Copy link
Contributor

@vladbarosan vladbarosan Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we do, but just that is nested

Copy link
Contributor

@vladbarosan vladbarosan Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

actually i think its fine how it is ? because we show the warning only if prev version exists

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I missed the nested check. looks good

const currVersionString = `${currentVersion.major}.${currentVersion.minor}`;

if (prevVersion !== currVersionString) {
if (prevVersion) {
vscode.window.showInformationMessage('Your Go version is different than before, few Go tools may need re-compiling', updateToolsCmdText).then(selected => {
if (selected === updateToolsCmdText) {
installAllTools(true);
}
});
}
toolsGoInfo[toolsGopath].version = currVersionString;
}
});
}
}
ctx.globalState.update('goroot', currentGoroot);
toolsGoInfo[toolsGopath].goroot = currentGoroot;
ctx.globalState.update('toolsGoInfo', toolsGoInfo);

offerToInstallTools();
if (checkLanguageServer()) {
Expand Down