kevincobain2000 / gobrew

Go version manager, written in Go. Super simple tool to install and manage Go versions. Install go without root. Gobrew doesn't require shell rehash.
https://medium.com/web-developer/go-version-manager-gobrew-c8750157dfe6
MIT License
374 stars 26 forks source link

gobrew install and use do not give an error print when an incorrect version is entered #196

Closed TafThorne closed 3 months ago

TafThorne commented 3 months ago

When using gobrew manually it is possible to make a simple error and not spot that gobrew is not in fact installing or configuring anything as it prints info lines (often rendered in nice green text) about an error in downloading and extracting a version. If the failure to download cannot be detected and printed as an error level log, the Extract failed message should be.

tt@pwcl-07688180:~
$ gobrew use go1.23.4
==> [Info] Downloading version: go1.23.4
==> [Info] Downloading from: https://go.dev/dl/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Downloading to: /home/tt/.gobrew/downloads
==> [Info] Extracting from: /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Extracting to: /home/tt/.gobrew/versions/go1.23.4
==> [Info] Extract failed: open /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz: no such file or directory

The above example currently fails and to the casual observer might be expected to start working in the next year when Go version 1.23 starts to be published.

I however made a mistake in adding the word go to the start of the version e.g. gobrew use go1.22.5, which caused Goberew to actually do nothing at all, only I did not spot this as everything was green [info] commands. This was further hidden as my left behind go version was 1.25.7 which helpfully downloaded the missing go tools based on my go.mod and most things mostly worked from that point on. Not everything though and it was rather confusing to work out why I had the correct version of go installed and working but not the correct version of go installed and work.

An error exit code is returned in gobew 1.10.8 which is helpful to scripting at least.

TafThorne commented 3 months ago

I think this line is the easiest place to correct it: https://github.com/kevincobain2000/gobrew/blob/50870b93bd22ba5b80360798c75a8dc0a8dcc128/helpers.go#L368

I would suggest things get changed to:

    if err != nil {
        // clean up dir
        gb.cleanVersionDir(version)
        colorErrorln("==> [Error] Extract failed:", err)
        os.Exit(1)
    }
TafThorne commented 3 months ago

What I am not sure of is why something like gobrew use madeupjunk is not causing an error in the download helper. If I go to

$ gobrew use madeupjunk
==> [Info] Downloading version: madeupjunk
==> [Info] Downloading from: https://go.dev/dl/gomadeupjunk.linux-amd64.tar.gz
==> [Info] Downloading to: /home/thomasth/.gobrew/downloads
==> [Info] Extracting from: /home/thomasth/.gobrew/downloads/gomadeupjunk.linux-amd64.tar.gz
==> [Info] Extracting to: /home/thomasth/.gobrew/versions/madeupjunk
==> [Info] Extract failed: open /home/thomasth/.gobrew/downloads/gomadeupjunk.linux-amd64.tar.gz: no such file or directory

When I visit https://go.dev/dl/gomadeupjunk.linux-amd64.tar.gz in a browser I get a 404 error which looks like it should get caught and return an earlier exit in the helper that would print an error message. I still think printing the extract failure as an Info level is wrong, but there seems to be an extra problem with handling failed download too.

kevincobain2000 commented 3 months ago

Fair enough. Checking….

Will get back to you in a day or two. Having a bad day today.

kevincobain2000 commented 3 months ago

I think better would be to do a ping -H to the URL first, and if header is 404 then die with a message. This will happen before gobrew starts creating dirs etc.

kevincobain2000 commented 3 months ago

To be specific

$ gobrew use go1.23.4
==> [Info] Pinging: go1.23.4
==> [Error] Status: 404
exit code 1

==> [Info] Downloading version: go1.23.4
==> [Info] Downloading from: https://go.dev/dl/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Downloading to: /home/tt/.gobrew/downloads
==> [Info] Extracting from: /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Extracting to: /home/tt/.gobrew/versions/go1.23.4
==> [Info] Extract failed: open /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz: no such file or director
juev commented 3 months ago

An interesting point that was missed. In my opinion, you should not check ping, but initially download the file, and only if this is successful, delete it and prepare new directories. In this case, if the user specifies a non-existent version, this will not affect the operation of the entire system.

IMO

kevincobain2000 commented 3 months ago

If going by @juev the output will look like below:

$ gobrew use go1.23.4
==> [Info] Downloading version: go1.23.4
==> [Info] Downloading from: https://go.dev/dl/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Downloading to: /home/tt/.gobrew/downloads
==> [Error] Downloading failed: check if the URL exists <-- here
exit code 1
==> [Info] Extracting from: /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz
==> [Info] Extracting to: /home/tt/.gobrew/versions/go1.23.4
==> [Info] Extract failed: open /home/tt/.gobrew/downloads/gogo1.23.4.linux-amd64.tar.gz: no such file or director
juev commented 3 months ago

If we were unable to download, we must complete the process immediately. I immediately see that we couldn’t download it, but we’re trying to do something.

kevincobain2000 commented 3 months ago

Sorry that’s what I meant by exit code and left the lines later as reference which won’t be printed

kevincobain2000 commented 3 months ago

Anyways. I won’t be working on this PR. I am quite busy with life these days.

juev commented 3 months ago

I fixed the behavior.

Added a check for the version existence and also fixed the error output colors.