enhancement(cli): Show Git version and target triple in `vector --version` output (#1044) #1044

ghost · 2019-10-17T14:38:12Z

Closes #951

This PR tries to implement the same logic for version info as rustc and cargo use (see this comment).

The table below summarizes output version for development and release versions (based on version in Cargo.toml), where builds either from the Git repository (locally / from CI) or from an archive (without .git directory in the source tree).

	master (`x.y.z-dev`)	release (`x.y.z`)
Local build	`0.6.0-dev (g816041c 2019-10-17)`	`0.6.0 (g816041c 2019-10-17)`
CI build	`0.6.0-nightly (g816041c 2019-10-17)`	`0.6.0 (g816041c 2019-10-17)`
Archive build	`0.6.0-dev`	`0.6.0`

Initially it is implemented using the information provided by the built crate, which provides the result of git describe --tags as built_info::GIT_VERSION, so that some unnecessary logic happens in runtime.

Ideally, it would be better to create a custom build.rs file and put this logic there, that's why I marked the PR as draft for now.

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

LucioFranco · 2019-10-17T14:43:48Z

src/main.rs

+    } else {
+        built_info::PKG_VERSION.into()
+    };
+    let commit_hash = built_info::GIT_VERSION.and_then(|v| v.split('-').last());


really I think this is the big thing holding us too built, it pulls down libgit2 and uses that. I wonder if we could just use Command + git?

Using Command + git would be less portable (for example, git command is not available on most Windows machines). I agree that libgit2 increases the build time, but it doesn't increase the final executable size because it is used only during the build.

I think we can assume that all users compiling from source should have git and if it doesn't exist we can just skip this and place a place holder. I don't think we will ever have the case of someone building a release and not having git.

I think we should separate the discussion about potentially moving away from built. Personally, I'm in favor of sticking with it (at least for the time being), but I don't think that decision needs to be made as part of this PR.

I disagree. I do not want to assume anything, and it is possible git is not installed. Again, I prefer to optimize for the UX not Lucio's developer happiness. 😄

Not sure what UX we lose by not using built? It's completely a release thing? All we would be avoiding is having to compile libgit to fetch a commit sha. Happy to discuss this further in another issue.

binarylogic · 2019-10-17T14:50:18Z

src/main.rs

+fn get_version() -> String {
+    let pkg_version = if built_info::CI_PLATFORM.is_some() && built_info::PKG_VERSION.contains('-')
+    {
+        built_info::PKG_VERSION.replace("-dev", "-nightly")


This gets into versioning questions:

Are we planning on representing the next version with the -dev suffix? If so, how do we know which version that is? For example, is the next version 0.6.0 or 1.0.0?

Or, should we forgo guessing the next version and allow the package version to represent the last released version? We could also use the result of git describe --tags here. I believe that is built_info::GIT_VERSION.

We started with this discussion but punted on a conclusion since it involved figuring out how we'll release point releases.

In my ideal world, we use the version of the next release + -dev. But since that requires knowing the future, I think my second choice would be most recent release + -dev?

I think that if we go with "the next version in Cargo.toml" approach, there might even be no need to use -dev suffix, it could be just the next version.

Rust sets version in master to the next released version, without any suffixes (link)

Clang does the same, as clang --version from master shows the next version to be released

Then the question is it an actual release or not could be decided in CI by looking at presence of the corresponding Git tag. For example, if the version in Cargo.toml is 0.6.0 and there is no Git tag v0.6.0, then the version output by vector --version would be vector 0.6.0-dev (g816041c 2019-10-17) and if there is tag v0.6.0 then vector --version would return vector 0.6.0 (g816041c 2019-10-17).

The case when pre-release version is distributed as an archive is rare and I think that there is no need to officially distribute source archives of master at all, it should be enough to distribute only release archives.

The question about whether to bump the version before release or not is tougher, but I lean more to the "next version" approach. If we show the previous version with some suffix, then the users could expect it to be compatible with the previous release, for example if they see the version string starting from 0.5.0, they could assume that they need to look at the documentation for version 0.5.0, which might not be correct.

However, the guessing of the next version is not that hard problem because it could happen not immediately after the next release, but only after the first new change is merged to master.

Example 1

we release 0.6.0

then we improve docs (a PR merged to master)

after the merge we increase the version in Cargo.toml to 0.6.1 and make a new release

then we wait for the next PR ready to be merged

Example 2

0.6.1 is released

new yet unstable sink is merged

we are not ready to release, but increase the version in Cargo.toml to 0.7.0 because there are already enough changes for the next major 0.y.z release

continue merging other PRs

release 0.7.0

Also, I see no need to actually release all versions that appear in Cargo.toml. If in the previous example we did even more changes after merging the new sink and think that we are ready to release 1.0.0, there seems to be nothing bad with bumping the version in Cargo.toml again up to 1.0.0 and having 1.0.0 release after 0.6.1.

Well said, and I agree. There is nothing stopping us from bumping up to a major version when it warrants it. My vote is to adopt this strategy. Ref #1010.

binarylogic · 2019-10-17T14:51:39Z

Nice work! Once we figure out my above comment we can update this to reflect our future versioning strategy.

lukesteensen · 2019-10-17T15:08:44Z

src/main.rs

+fn get_version() -> String {
+    let pkg_version = if built_info::CI_PLATFORM.is_some() && built_info::PKG_VERSION.contains('-')
+    {
+        built_info::PKG_VERSION.replace("-dev", "-nightly")


In my ideal world, we use the version of the next release + -dev. But since that requires knowing the future, I think my second choice would be most recent release + -dev?

lukesteensen · 2019-10-17T15:10:04Z

src/main.rs

+    } else {
+        built_info::PKG_VERSION.into()
+    };
+    let commit_hash = built_info::GIT_VERSION.and_then(|v| v.split('-').last());


I think we should separate the discussion about potentially moving away from built. Personally, I'm in favor of sticking with it (at least for the time being), but I don't think that decision needs to be made as part of this PR.

lukesteensen · 2019-10-17T15:12:59Z

src/main.rs

+    };
+    let commit_hash = built_info::GIT_VERSION.and_then(|v| v.split('-').last());
+    let built_date = chrono::DateTime::parse_from_rfc2822(built_info::BUILT_TIME_UTC)
+        .unwrap()


It doesn't really matter, but built does provide a helper to do this parsing: https://docs.rs/built/0.3.2/src/built/util.rs.html#68-72

But using it would require adding built as a runtime dependency, while now it is used only as a build-time dependency.

That is a good point! Probably not worth it.

binarylogic · 2019-10-20T02:50:29Z

However, the guessing of the next version is not that hard problem because it could happen not immediately after the next release, but only after the first new change is merged to master.

@a-rodin I'm happy to rollback fe26627. I'm a little concerned that we'll reliably bump the version as changes happen, but I'm happy to give it a shot and see how it goes.

ghost · 2019-10-21T10:13:09Z

@binarylogic

I'm a little concerned that we'll reliably bump the version as changes happen

Could we solve this by adding an additional check to CI that would fail the build if there are additional changes since last tag, but version is not not updated?

lukesteensen

Just to summarize, I think there are two things being discussed here:

The code changes in this PR, which I think are good and generally non-controversial
The question of when in our development/release process we should bump the version number in Cargo.toml

The leading proposal for (2) seems to be that we bump to the next potential release version as we make the changes that justify that version bump. For example:

Release version = "0.1.0" (git tag v0.1.0)
Merge small change to docs
Bump to version = "0.1.1" (no git tag)
Merge large breaking change
Bump to version = "0.2.0" (no git tag)
Merge other small changes
Release version = "0.2.0" (git tag v0.2.0)

I think this generally seems good and would like to clarify a few things:

Do we include a -dev/-nightly suffix in the Cargo.toml version or is that something that's inferred by the lack of git tag?
Should the version bump happen as part of the normal changes, before them, or after them? I see some slight downsides to bundling them into the same change (e.g. reverts get funky, coordinating multiple open PRs, etc), but before and after both seem like they could be easy to forget and slightly awkward.

ghost · 2019-10-23T12:31:51Z

Do we include a -dev/-nightly suffix in the Cargo.toml version or is that something that's inferred by the lack of git tag?

I prefer inferring the suffix from lack of Git tag. I think it makes history of Cargo.toml changes less noisy and also I find it nice to have current master always ready to be released without additional changes, which could be useful if we ever decide to do schedule-based releases.

Should the version bump happen as part of the normal changes, before them, or after them? I see some slight downsides to bundling them into the same change (e.g. reverts get funky, coordinating multiple open PRs, etc), but before and after both seem like they could be easy to forget and slightly awkward.

I think what we definitely need to do here is to add a CI test that would fail if there is no Git tag but the version is not updated yet. It would allow us to not forget to bump the version in any case. I don't have a strong opinion about whether bumping the versions is better to be done before merging/after merging/inside PRs, but I like "after merging" slightly more because it generates clean commit history like

* Improve documentation for A (#1001)
* Bump version to 0.x.y
* Add more tests for B (#1002)
* Add feature C (#999)
* Bump version to 0.z.0

or even

* Improve documentation for A (#1001)
* Add more tests for B (#1002)
* Bump version to 0.x.y
* Add feature C (#999)
* Bump version to 0.z.0

in case if PRs for A and B were merged at the same time.

binarylogic · 2019-10-23T13:24:54Z

It'd be pretty easy to add a CI check on master that would compare the changes between the last tag and HEAD to determine if a version bump is necessary. And assuming we follow conventional commits properly it could determine if we need a patch, minor, or major bump. This would remove the "remember" aspect of this process.

What do you think?

ghost · 2019-10-24T08:42:42Z

And assuming we follow conventional commits properly it could determine if we need a patch, minor, or major bump.

I like the idea of using conventional commits for this. The CI check can infer minimal necessary version bump and fail if the current version bump is less than the minimal required. However, I think the maximal version bump don't have to be constrained by CI to allow doing larger version increments for external reasons. It can be useful when we decide to release 1.0 or if some of the commits end up not following conventional commits strictly enough.

binarylogic · 2019-10-24T13:16:26Z

Agree 👍. I've opened #1083 to represent that.

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

ghost · 2019-11-04T13:48:29Z

I've updated the version displaying to match the new strategy and show the target platform (#1110).

Currently the displayed versions should looks like this:

Build type	Version
Local build from Git	`0.6.0 (g816041c x86_64-unknown-linux-musl 2019-11-04)`
Local build from archive	`0.6.0 (x86_64-unknown-linux-musl)`
CI nightly build	`0.6.0-nightly (g816041c x86_64-unknown-linux-gnu 2019-11-04)`
CI release build	`0.6.0 (g816041c x86_64-unknown-linux-gnu 2019-11-04)`

Nightly versions suffixes are appended when Vector was compiled with special nightly feature, as was proposed by @LucioFranco. Adding the nightly feature to nightly builds in CI is not implemented yet.

binarylogic · 2019-11-04T14:51:49Z

Looks great!

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Alexander Rodin added 2 commits October 17, 2019 13:34

Show Git version using built_info crate

7e144e6

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Append "nightly" suffix for CI builds of "dev" versions

816041c

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

ghost requested review from LucioFranco, binarylogic and lukesteensen and removed request for binarylogic and LucioFranco October 17, 2019 14:39

LucioFranco reviewed Oct 17, 2019

View reviewed changes

binarylogic reviewed Oct 17, 2019

View reviewed changes

lukesteensen approved these changes Oct 17, 2019

View reviewed changes

binarylogic self-requested a review October 20, 2019 02:47

binarylogic approved these changes Oct 20, 2019

View reviewed changes

binarylogic assigned lukesteensen Oct 21, 2019

lukesteensen reviewed Oct 22, 2019

View reviewed changes

binarylogic mentioned this pull request Oct 24, 2019

Add CI check to enforce new versioning strategy #1083

Closed

Alexander Rodin added 2 commits November 4, 2019 12:31

Merge branch 'master' into git-version

06b37d7

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Support new versioning strategy and add target to version

a1cb43e

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

ghost marked this pull request as ready for review November 4, 2019 13:48

Merge branch 'master' into git-version

1212750

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Alexander Rodin added 4 commits November 4, 2019 19:20

Update nightly build script

e98db58

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Generalize regexp for extracting version from Cargo.toml

f1c9043

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Determine whether the build is nightly using Git branch

7e407a4

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

Remove unused "use" from build.rs

e993fbc

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>

ghost changed the title ~~enhancement(cli): Show Git version~~ enhancement(cli): Show Git version and target triple in vector --version output (#1044) Nov 4, 2019

ghost merged commit a0a5bee into master Nov 4, 2019

ghost mentioned this pull request Nov 5, 2019

Display the target triple in Vector's version #1110

Closed

ghost deleted the git-version branch November 20, 2019 16:51

binarylogic mentioned this pull request Dec 16, 2019

Please update vector binary to v0.5.0 on release archives and docker images #1296

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(cli): Show Git version and target triple in `vector --version` output (#1044) #1044

enhancement(cli): Show Git version and target triple in `vector --version` output (#1044) #1044

ghost commented Oct 17, 2019 •

edited by ghost

LucioFranco Oct 17, 2019

ghost Oct 17, 2019 •

edited by ghost

LucioFranco Oct 17, 2019

lukesteensen Oct 17, 2019

binarylogic Oct 17, 2019 •

edited

LucioFranco Oct 17, 2019

binarylogic Oct 17, 2019

lukesteensen Oct 17, 2019

ghost Oct 17, 2019

ghost Oct 17, 2019 •

edited by ghost

binarylogic Oct 17, 2019 •

edited

binarylogic commented Oct 17, 2019

lukesteensen Oct 17, 2019

lukesteensen Oct 17, 2019

lukesteensen Oct 17, 2019

ghost Oct 18, 2019

lukesteensen Oct 21, 2019

binarylogic commented Oct 20, 2019

ghost commented Oct 21, 2019 •

edited by ghost

lukesteensen left a comment

ghost commented Oct 23, 2019

binarylogic commented Oct 23, 2019

ghost commented Oct 24, 2019 •

edited by ghost

binarylogic commented Oct 24, 2019 •

edited

ghost commented Nov 4, 2019 •

edited by ghost

binarylogic commented Nov 4, 2019

enhancement(cli): Show Git version and target triple in vector --version output (#1044) #1044

enhancement(cli): Show Git version and target triple in vector --version output (#1044) #1044

Conversation

ghost commented Oct 17, 2019 • edited by ghost

Choose a reason for hiding this comment

ghost Oct 17, 2019 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binarylogic Oct 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 17, 2019 • edited by ghost

Choose a reason for hiding this comment

binarylogic Oct 17, 2019 • edited

Choose a reason for hiding this comment

binarylogic commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binarylogic commented Oct 20, 2019

ghost commented Oct 21, 2019 • edited by ghost

lukesteensen left a comment

Choose a reason for hiding this comment

ghost commented Oct 23, 2019

binarylogic commented Oct 23, 2019

ghost commented Oct 24, 2019 • edited by ghost

binarylogic commented Oct 24, 2019 • edited

ghost commented Nov 4, 2019 • edited by ghost

binarylogic commented Nov 4, 2019

enhancement(cli): Show Git version and target triple in `vector --version` output (#1044) #1044

enhancement(cli): Show Git version and target triple in `vector --version` output (#1044) #1044

ghost commented Oct 17, 2019 •

edited by ghost

ghost Oct 17, 2019 •

edited by ghost

binarylogic Oct 17, 2019 •

edited

ghost Oct 17, 2019 •

edited by ghost

binarylogic Oct 17, 2019 •

edited

ghost commented Oct 21, 2019 •

edited by ghost

ghost commented Oct 24, 2019 •

edited by ghost

binarylogic commented Oct 24, 2019 •

edited

ghost commented Nov 4, 2019 •

edited by ghost