Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When I am removing Go, I want to prefer existing paths #24

Closed
wants to merge 2 commits into from

Conversation

martinkunc
Copy link

@martinkunc martinkunc commented Oct 9, 2019

The script wasnt removing Go for me, because I havent had $GOROOT or it wasnt in ~/.go
I think when we are removing, we could rely on what go env has set. What do you think about following changes ?
Btw, great script :)

@d2s
Copy link

d2s commented Oct 9, 2019

I see various issues with the way the --remove works currently.

As mentioned on the g go version manager's documentation:

"Now, if you want to remove everything, first be sure to backup your projects inside $GOROOT, if any."

Most likely there should (at least) be a confirmation step before rm -rf "$GOROOT" happens, because if people don't remember that their own code is under $GOROOT (as is the convention with Go), they will end up losing their own files.

@canha
Copy link
Owner

canha commented Oct 10, 2019

@martinkunc Thanks for the PR, I'll review it as soon as I can.

@d2s Can you point me to where is the convention about storing code under $GOROOT? In general you shouldn't even touch that variable/folder unless you really need to. In essence, it stores the Golang's install, not people's code - that's in $GOPATH, which the script never touches.

Regardless, I guess it doesn't hurt printing out the full path and asking for another confirmation.

@d2s
Copy link

d2s commented Oct 10, 2019

@canha Probably my misunderstanding, sorry.

At least that is what I had understood while following Go's official documentation (How to Write Go Code - The Go Programming Language) and reading how Google etc. use Go.

mkdir $GOPATH/src/github.com/user/hello

If you're using a source control system, now would be a good time to initialize a repository, add the files, and commit your first change. Again, this step is optional: you do not need to use source control to write Go code.

$ cd $GOPATH/src/github.com/user/hello
$ git init
Initialized empty Git repository in /home/user/go/src/github.com/user/hello/.git/

From a blog post The default GOPATH (from 2016):

"If your GOROOT (the location where you checkout the Go’s source code) is the default GOPATH and if you don’t have a GOPATH set, the tools will reject to use the default GOPATH not to corrupt your GOROOT."

Copy link
Owner

@canha canha left a comment

Choose a reason for hiding this comment

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

Thanks for thinking about this case. What happens if for some reason the go executable is not in the path? I believe the variables will be empty and we aren't doing additional checks. We should probably do something about this.

@majorthorn
Copy link
Contributor

majorthorn commented Oct 14, 2019

The script wasnt removing Go for me, because I havent had $GOROOT or it wasnt in ~/.go
I think when we are removing, we could rely on what go env has set. What do you think about following changes ?
Btw, great script :)

How were you calling go from your terminal if you didnt have a GOROOT?

@canha
Copy link
Owner

canha commented Oct 14, 2019

The script wasnt removing Go for me, because I havent had $GOROOT or it wasnt in ~/.go
I think when we are removing, we could rely on what go env has set. What do you think about following changes ?
Btw, great script :)

How were you calling go from your terminal if you didnt have a GOROOT?

Because you only need go to be in your path to use it. Actually, we should probably stop adding the GOROOT variable as part of the script because it's not really needed, but I'll create an issue for it after we close this.

@martinkunc
Copy link
Author

What about a check like this ?
This should prefer GOROOT if set (to allow set it externally) while removal.
Then it should try run go env GOROOT if exists.
I think that is most default setup - not using GOROOT, but have go configured with default -> go env GOROOT shows /usr/local/go.

I guess some future improvement would be to try to use /usr/local/go over ~/.go when possible.
What are your thoughts ?

rm -rf "$GOROOT"
# Try $GOROOT var, then if go is in PATH, execute go env GOROOT to get configured value, otherwise will be empty
GOROOT=$( [ ! -z "$GOROOT" ] && echo \"$GOROOT\" || ( command -v go > /dev/null && go env GOROOT) )
GOPATH=$(go env GOPATH)
Copy link
Owner

Choose a reason for hiding this comment

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

If $GOROOT is still empty at this point (line 49), what do you think about just showing an error and exiting the script immediately?

Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks in testing.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, sorry unfortunatelly notifications from GH seems to be slow for me.
Can you explain more how it breaks @majorthorn ? Would you like to cover the case when GOROOT is not set ? Because this script is setting it on install. Or some other case when someone used script just for uninstallation ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@canha
Copy link
Owner

canha commented Oct 14, 2019

I guess some future improvement would be to try to use /usr/local/go over ~/.go when possible.
What are your thoughts ?

The problem with that is that it requires root privileges, which is something we'd like to avoid.

@majorthorn
Copy link
Contributor

majorthorn commented Oct 14, 2019

I guess some future improvement would be to try to use /usr/local/go over ~/.go when possible.
What are your thoughts ?

The problem with that is that it requires root privileges, which is something we'd like to avoid.

Keeping with this line of thinking

Would you be willing add a check on the script to see if the script is running as Root and Changing its install path based on that? The only issue we would run into with that is figuring out how we would tell the --remove to remove the correct directory. Nevermind, I think that would add a level of complexity that is outside the scope of this PR and probably should be discussed in an Issue.

@majorthorn
Copy link
Contributor

majorthorn commented May 7, 2020

This is 7 months old. Suggesting we close this pr

@canha canha closed this May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants