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

cli: add cgo compiler, os and arch info to version output #5766

Merged
merged 1 commit into from
Mar 31, 2016

Conversation

dt
Copy link
Member

@dt dt commented Mar 31, 2016

also hide the dep versions unless requested with --all since they are usually captured by the tag
thanks to glock, and thus just clutter up submitted issue reports.

$ ./cockroach version
Build Tag:   beta-20160331-dirty
Build Time:  2016/03/31 14:42:04
Platform:  darwin amd64
Go Version:  go1.6
C Compiler:  4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.29)

This change is Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


cli/cli.go, line 46 [r1] (raw file):
Can you add another space before the %s so the fields are aligned?


util/build.go, line 25 [r1] (raw file):
This seems to return something less useful for gcc. Try building with build/builder.sh make build and then build/builder.sh ./cockroach version.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Mar 31, 2016

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


cli/flags.go, line 480 [r1] (raw file):
put an extra newline above this to separate


cli/cliflags/names.go, line 21 [r1] (raw file):
"all" is really ambiguous. How about "deps"?


util/build.proto, line 25 [r1] (raw file):
can you rename this field to go_version and remove the json tag? this is just confusing.


util/build.proto, line 29 [r1] (raw file):
we use snake_case in protos


util/build.proto, line 30 [r1] (raw file):
why bother with the tag?


Comments from the review on Reviewable.io

@dt dt force-pushed the versioninfo branch 3 times, most recently from 937d952 to 9cb1e88 Compare March 31, 2016 15:36
@dt
Copy link
Member Author

dt commented Mar 31, 2016

Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions.


cli/cli.go, line 46 [r1] (raw file):
Done.


cli/flags.go, line 480 [r1] (raw file):
Done.


cli/cliflags/names.go, line 21 [r1] (raw file):
Done.


util/build.go, line 25 [r1] (raw file):
Done.


util/build.proto, line 25 [r1] (raw file):
Done.


util/build.proto, line 29 [r1] (raw file):
Done.


util/build.proto, line 30 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

:lgtm:


Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


util/build.go, line 25 [r2] (raw file):
Technically, I believe the hash mark (#) is supposed to appear in the first column. So you should de-indent this to line up with the const char*.


util/build.go, line 30 [r2] (raw file):
"unknown " __VERSION__?


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Mar 31, 2016

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


cli/flags.go, line 79 [r2] (raw file):
s/all //


Comments from the review on Reviewable.io

also hide the dep versions unless requested with --all since they are usually captured by the tag
thanks to glock, and thus just clutter up submitted issue reports.

```
$ ./cockroach version
Build Tag:   beta-20160331-dirty
Build Time:  2016/03/31 14:42:04
Platform:  darwin amd64
Go Version:  go1.6
C Compiler:  4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.29)
```
@dt
Copy link
Member Author

dt commented Mar 31, 2016

Review status: 5 of 9 files reviewed at latest revision, 3 unresolved discussions.


util/build.go, line 25 [r2] (raw file):
Done.


util/build.go, line 30 [r2] (raw file):
I think if it somehow isn't gcc or clang, we can't actually count on __VERSION__ being defined (i.e. looks like MS VS doesn't define it).


Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

Review status: 3 of 9 files reviewed at latest revision, 2 unresolved discussions.


util/build.go, line 30 [r2] (raw file):
Ah, good point. Carry on.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Mar 31, 2016

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


util/build.go, line 30 [r2] (raw file):
Perhaps we should log something better than "unknown"? "unknown: not clang or gcc".


util/build.proto, line 25 [r3] (raw file):
did this break the UI? sigh


Comments from the review on Reviewable.io

@dt
Copy link
Member Author

dt commented Mar 31, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


util/build.proto, line 25 [r3] (raw file):
yeah, not sure if the UI acutally uses it, but it certainly made the test unhappy.


Comments from the review on Reviewable.io

@dt dt merged commit a5215bf into cockroachdb:master Mar 31, 2016
@dt dt deleted the versioninfo branch April 7, 2016 17:07
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.

4 participants