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: improve jj version output #3638

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

thoughtpolice
Copy link
Member

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:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@@ -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)?;
Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a 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.

cli/src/commands/version.rs Outdated Show resolved Hide resolved
cli/src/commands/version.rs Outdated Show resolved Hide resolved
cli/src/commands/version.rs Outdated Show resolved Hide resolved
@thoughtpolice thoughtpolice force-pushed the aseipp/push-norxmmmnvvqp branch 3 times, most recently from 5c64353 to 3e45560 Compare May 14, 2024 00:29
@@ -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: {}",
Copy link
Contributor

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.

Copy link
Member Author

@thoughtpolice thoughtpolice May 15, 2024

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.
@thoughtpolice thoughtpolice force-pushed the aseipp/push-norxmmmnvvqp branch from 3e45560 to c38fe35 Compare June 12, 2024 20:49
PhilipMetzger added a commit that referenced this pull request Jul 24, 2024
PhilipMetzger added a commit that referenced this pull request Jul 24, 2024
PhilipMetzger added a commit that referenced this pull request Jul 24, 2024
PhilipMetzger added a commit that referenced this pull request Jul 24, 2024
PhilipMetzger added a commit that referenced this pull request Jul 24, 2024
@PhilipMetzger
Copy link
Contributor

Any intent to land this? As our version output has now been incorrect for some releases.

@PhilipMetzger
Copy link
Contributor

Friendly ping that this still should land at some point.

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.

Release binaries show the git hash in their version output.
4 participants