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

Do not panic on genji version when compiled in GOPATH mode #262

Merged
merged 2 commits into from
Oct 17, 2020
Merged

Do not panic on genji version when compiled in GOPATH mode #262

merged 2 commits into from
Oct 17, 2020

Conversation

tdemin
Copy link
Contributor

@tdemin tdemin commented Oct 15, 2020

The genji version command relies on the versions of modules compiled in as debug info. Those are unavailable when compiling Genji with go get github.com/genjidb/genji/cmd/shell (in GOPATH mode). This makes genji version display a message saying the version is not available if genji has been built with go get.

Sample output:

genji on master [!?] via v1.15.2 > GO111MODULE=off go run github.com/genjidb/genji/cmd/genji version
Using z.Allocator with starting ref: 8005000000000000
version not available in GOPATH mode

The genji version command relies on the versions of modules compiled in
as debug info. Those are unavailable when compiling Genji with go get.
This makes genji version display a message saying the version is not
available if genji has been built with go get.
@tdemin
Copy link
Contributor Author

tdemin commented Oct 15, 2020

I was thinking of returning an additional error code with cli.NewExitError("...", 1), but this would add more visual noise (it's already 2 lines of output, and this would add the 3rd one with an exit code).

@tdemin
Copy link
Contributor Author

tdemin commented Oct 15, 2020

Related issue: #261

@tdemin tdemin changed the title Do not panic when compiled in GOPATH mode Do not panic on genji version when compiled in GOPATH mode Oct 16, 2020
cliVersion = info.Main.Version

if !ok {
fmt.Println("version not available in GOPATH mode")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should suggest running go get with modules enabled? Something like

fmt.Println("version not available in GOPATH mode; use \"go get\" with Go modules enabled")

@tie
Copy link
Contributor

tie commented Oct 17, 2020

@tdemin thanks for this change, nice catch! I’ve added a minor suggestion for the GOPATH mode message.

@tdemin
Copy link
Contributor Author

tdemin commented Oct 17, 2020

@tie sure thing, just added a notice on using modules.

@codecov-io
Copy link

codecov-io commented Oct 17, 2020

Codecov Report

Merging #262 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #262   +/-   ##
=======================================
  Coverage   61.32%   61.32%           
=======================================
  Files          68       68           
  Lines        6345     6345           
=======================================
  Hits         3891     3891           
  Misses       1938     1938           
  Partials      516      516           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 375ed7a...7848504. Read the comment docs.

Copy link
Collaborator

@asdine asdine left a comment

Choose a reason for hiding this comment

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

Thanks @tdemin !

@asdine asdine merged commit ce8c2f0 into chaisql:master Oct 17, 2020
@asdine asdine linked an issue Oct 17, 2020 that may be closed by this pull request
@asdine asdine mentioned this pull request Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic on genji version
4 participants