-
Notifications
You must be signed in to change notification settings - Fork 380
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: improve jj version
output
#3638
base: main
Are you sure you want to change the base?
Conversation
630c215
to
6df5da8
Compare
@@ -63,6 +70,7 @@ Report bugs: <https://github.com/martinvonz/jj/issues> | |||
if args.build_info { | |||
writeln!(ui.stdout())?; | |||
writeln!(ui.stdout(), "Target: {}", env!("JJ_CARGO_TARGET"))?; | |||
writeln!(ui.stdout(), "Commit: {}", git_commit)?; |
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.
Do we want to allow redistributors the option of adding another suffix here, so it can be even clearer how it was built?
e.g. my rustc version
gives rustc 1.78.0 (9b00956e5 2024-04-29) (Alpine Linux 1.78.0-r0)
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.
Yes, and I think you should also be able to set some information through the API, too.
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.
LG with minor nit.
5c64353
to
3e45560
Compare
@@ -73,6 +79,11 @@ Report bugs: <https://github.com/martinvonz/jj/issues> | |||
writeln!(ui.stdout())?; | |||
writeln!(ui.stdout(), "Target: {}", env!("JJ_CARGO_TARGET"))?; | |||
writeln!(ui.stdout(), "Commit: {}", git_commit)?; | |||
writeln!( | |||
ui.stdout(), | |||
"Release: {}", |
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.
I'm not sure what the point of this field is. If the numeric version doesn't have a commit suffix, it's a release build. If it does have one, it's not.
Also I originally thought it was referring to whether the binary was built in release mode. It might separately be useful to add a big all caps blinking banner when the binary is built in debug mode.
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.
I guess it's mostly just a thing in case users look at it, since they may not immediately know what build they're using (they may miss finer details like that or easily forget where they sourced their version from.)
Also, I'd like the build to include the actual Cargo features, but that's probably trickier.
This prints the build options used to compile the `jj` binary. Used further in an upcoming diff.
`version` now prints the short 12-byte git hash of the current commit, and this is now prefixed with `g` to make it clear that the following characters are a "g"it hash; we might use a `j` variant in the future, for example. The full commit hash is still given when `--build-info` is provided.
Hopefully this paper trail can help us in the future.
3e45560
to
c38fe35
Compare
Waleed noticed this is #3638.
Waleed noticed this is #3638.
Waleed noticed this is #3638.
Waleed noticed this in #3638.
Waleed noticed this in #3638.
Any intent to land this? As our version output has now been incorrect for some releases. |
Friendly ping that this still should land at some point. |
Spruces up the
version
output to be a little more informative. Adds two new flags for more specific information. The new output was somewhat inspired by the way coreutils tools print output, and other similar tools that have some equivalent of things like--numeric-only
. Best read commit by commit as usual.Fixes #3629.
Checklist
If applicable:
CHANGELOG.md