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

Add commit hash to version info, if present #957

Merged
merged 3 commits into from Nov 14, 2021
Merged

Add commit hash to version info, if present #957

merged 3 commits into from Nov 14, 2021

Conversation

AloeareV
Copy link
Contributor

@AloeareV AloeareV commented Nov 2, 2021

Fixes #922. Right now it doesn't detect release versions, and just blindly throws on the commit hash in all cases that one exists. Reverts to old (current) behavior if built in an environment with no git repo.

@@ -66,7 +66,7 @@ FLAGS:
-V, --version Prints version information
",
env!("CARGO_PKG_NAME"),
env!("CARGO_PKG_VERSION"),
env!("GIT_HASH"),
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see us use version-hash in both places, i.e. v0.4.0-abcdef

Comment on lines 6 to 9
.map(|x| String::from_utf8(x.stdout).ok())
.ok()
.flatten()
.unwrap_or(String::from(env!("CARGO_PKG_VERSION")));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.map(|x| String::from_utf8(x.stdout).ok())
.ok()
.flatten()
.unwrap_or(String::from(env!("CARGO_PKG_VERSION")));
.and_then(|output| String::from_utf8((output.stdout))
.unwrap_or_default();

use std::process::Command;
fn main() {
let git_hash = Command::new("git")
.args(&["describe", "--dirty"])
Copy link
Member

Choose a reason for hiding this comment

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

I think we also want to use "rev-parse", "--short", "HEAD" so we only get the SHA

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought @pickfire 's comment here:

#922 (comment)

made sense. It seems like dirty-ness and distance from release could be really useful info. Is there a constraint somewhere else that makes such a string difficult to work with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that for my first draft, and then changed to describe --dirty after reading @pickfire's comment on the issue. Would be no problem to switch back, if we're not concerned about version including knowledge of dirty state.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's okay, but describe always uses the latest available tag. We'll have to be careful because if the code is built after a new tag is accidentally added then the version info will be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worthwhile to use a specific process/tool to enforce that the version can't use any tag except the actual release tag?

Copy link
Member

Choose a reason for hiding this comment

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

So this actually happened in the latest release (it builds as v0.5 on latest master because the release points to a commit before I rebased against master). I'd still rather see we only pull the SHA and use the crate version.

Co-authored-by: Ivan Tham <pickfire@riseup.net>
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.

Insufficient information in --version to distinguish between release and development instances
4 participants