-
Notifications
You must be signed in to change notification settings - Fork 646
Make tools re-compiling warnings scoped by toolsGopath #1589
Conversation
I signed and emailed the Google / Microsoft CLA. I think you'll need to tick that box manually. |
@ramya-rao-a ping. I think you'll need to toggle CLA state manually. |
@FiloSottile We usually dont need to do anything from our side. Did you click on the link "sign now" above and sign there? |
@ramya-rao-a Microsoft has a custom CLA for Google employees, I can't use the online system when contributing as part of my job. But I already signed the non-standard CLA and emailed it to the relevant Microsoft address (I think cla-automation@) and I'm told that now you'll have to check it manually on your side. |
:) There is no button/link here for me to manually do anything in this area, I'll check with a few folks and get back to you. #949 was a PR from another person from Google. I didnt have to manually check anything then @stamblerre Can you share what you had to do for the cla? |
I think something might have indeed happened behind the scenes in #949 because the bot marked it as "cla-already-signed" instead of just signed. |
@FiloSottile Our internal team for Open Source has reached out to the Google legal team which now has agreed that Google employees should use the normal CLA approach. Can you recheck your internal guidance in this matter? |
goroot: string; | ||
version: string; | ||
} | ||
const toolsGopath = getToolsGopath() || getCurrentGoPath(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This would be a super helpful PR to land. I've been having to continuously patch every new release with this to be able to work with multiple workspaces. Are there plans to push this forward? |
@njgraf512 I was just waiting @FiloSottile to get back on my comments. Feel free to jump aboard and address them if you got some time. |
When switching between workspaces with different goroot and toolsGopath settings, the editor is suggesting that re-compiling tools is required because the GOROOT state is global and not toolsGopath-aware. This makes the global context a map keyed on toolsGopath.
@ramya-rao-a @jhendrixMSFT made requested changes. please comment if there is anything else. |
Tested, everything seems fine.
|
@ramya-rao-a since you requested changes initially , all good on your side ? |
ctx.globalState.update('goVersion', currVersionString); | ||
const currentVersion = await getGoVersion(); | ||
if (currentVersion) { | ||
const prevVersion = toolsGoInfo[toolsGopath].version; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess, good catch
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Thanks everyone for picking this up. ✨ I kept meaning to get back to it but never managed to. |
This feature is out in the latest release(0.10.0) of the Go extension. |
When switching between workspaces with different goroot and toolsGopath
settings, the editor is suggesting that re-compiling tools is required
because the GOROOT state is global and not toolsGopath-aware.
This makes the global context a map keyed on toolsGopath.