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 CLI git hash #1101

Merged
merged 14 commits into from
Jul 13, 2021
Merged

Add CLI git hash #1101

merged 14 commits into from
Jul 13, 2021

Conversation

hu55a1n1
Copy link
Member

Closes: #1094

Description

Export a env var (i.e. HERMES_VERSION) from build.rs and use it as the CLI's version string.
The newer version string is a valid semver string of the form -> <pkg-version>+<git-branch>-<git commit hash>. There's also an optional suffix -dirty which is added if the git repo is dirty. e.g. -> 0.4.0+cli-git-hash-760270a8-dirty


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@romac
Copy link
Member

romac commented Jun 17, 2021

Very nice, thanks @hu55a1n1!

One request: is there a way we can ensure that binaries built on CI and uploaded to the GitHub release show just the version without the +branch-commit[-dirty] suffix?

@hu55a1n1
Copy link
Member Author

One request: is there a way we can ensure that binaries built on CI and uploaded to the GitHub release show just the version without the +branch-commit[-dirty] suffix?

Thanks for reviewing the PR @romac!
I have been thinking about how to achieve that. Do you know if there's an env variable that the CI sets or some other way that I can use to figure out if the build script is running on the CI?

Otherwise, is it an option to change the regex in acceptance.rs to version meta?

fn start_no_args() {
    // ...
    let mut runner = RUNNER.clone();
    let mut cmd = runner.capture_stdout().run();
    cmd.stdout().expect_regex(
        format!(
            "^[^ ]*{} {}", // <-------------------------- remove the trailing `$`?
            env!("CARGO_PKG_NAME"),
            env!("CARGO_PKG_VERSION")
        )
        .as_str(),
    );
    // ...
}

relayer-cli/build.rs Outdated Show resolved Hide resolved
@romac
Copy link
Member

romac commented Jun 17, 2021

One request: is there a way we can ensure that binaries built on CI and uploaded to the GitHub release show just the version without the +branch-commit[-dirty] suffix?

Thanks for reviewing the PR @romac!
I have been thinking about how to achieve that. Do you know if there's an env variable that the CI sets or some other way that I can use to figure out if the build script is running on the CI?

Otherwise, is it an option to change the regex in acceptance.rs to version meta?

fn start_no_args() {
    // ...
    let mut runner = RUNNER.clone();
    let mut cmd = runner.capture_stdout().run();
    cmd.stdout().expect_regex(
        format!(
            "^[^ ]*{} {}", // <-------------------------- remove the trailing `$`?
            env!("CARGO_PKG_NAME"),
            env!("CARGO_PKG_VERSION")
        )
        .as_str(),
    );
    // ...
}

Yes, I actually did that in d3c5360, which has fixed the test.

But now the CI is still broken because Rust 1.53 was just released, and comes with new clippy lints. I think we can disregard these failures for now. I will fix them in a separate PR.

@romac
Copy link
Member

romac commented Jun 17, 2021

Do you know if there's an env variable that the CI sets or some other way that I can use to figure out if the build script is running on the CI?

Looks like there's a bunch of them, not sure which one we should use. Perhaps we can check for GITHUB_JOB == "create-release"?

@romac
Copy link
Member

romac commented Jun 17, 2021

But now the CI is still broken because Rust 1.53 was just released, and comes with new clippy lints. I think we can disregard these failures for now. I will fix them in a separate PR.

Should be good now, merged #1102 and merged master here.

@hu55a1n1
Copy link
Member Author

I just noticed that the new version doesn't appear for help subcommands like hermes tx help. The reason is that there's a blanket impl<C> Command for Help<C> where C: Command, and due to the way gumdrop deals with help, this would mean that we would have to write quite a lot of impls for helps manually instead of deriving them.
I think this was the reason the build script overwrites the CARGO_PKG_NAME instead of trying to change Command impls. I think that is the best solution to modify the version as well, i.e. overwrite CARGO_PKG_VERSION with a valid semver.
If there is no objection, I would like to implement the git hash support in a similar fashion. 🙂

@hu55a1n1
Copy link
Member Author

Linking #590 here, because it describes the problem in more detail. Although here, the chances of breaking cargo workflow are quite low since we're not overwriting the version for CI builds.

@romac
Copy link
Member

romac commented Jun 24, 2021

@hu55a1n1 What's the status on this PR?

@romac romac self-assigned this Jun 27, 2021
@adizere
Copy link
Member

adizere commented Jun 29, 2021

Tagged with "backburner" until Shoaib returns from holidays to look at this again.

@hu55a1n1
Copy link
Member Author

hu55a1n1 commented Jul 5, 2021

@hu55a1n1 What's the status on this PR?

The acceptance test for the CLI was failing because it didn't escape regular expression meta characters in the CARGO_PKG_VERSION since this wasn't expected before. I think it is fixed now. 👍

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Nice work, this is going to be very useful!

@adizere adizere merged commit 8613229 into master Jul 13, 2021
@adizere adizere deleted the hu55a1n1/cli-git-hash branch July 13, 2021 07:48
hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Export HERMES_VERSION env var from build.rs

* Override CLI version to use exported HERMES_VERSION

* Fix CLI integration test which checks the version number

* Move git stuff into it's own module

* Don't add version meta for CI release builds

* Fix git dirty command

* Overwrite CARGO_PKG_VERSION instead of setting HERMES_VERSION

* Minor refactoring

* Fix check for GITHUB_JOB

* Escape regex meta for acceptance test

* Add regex as dev-dep

* Changelog

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Adi Seredinschi <adi@informal.systems>
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.

Add git commit hash to CLI help & error output
3 participants