Fixed version info be set when go install #991

vasser · 2023-01-13T17:15:02Z

Resolves #928

In Go 1.18+ debug.ReadBuildInfo contains revision so we can read it from there when we are installing git-bug with go install.

In this PR we are utilizing the following attributes of debug.BuildInfo:

vcs.revision - get commit hash
vcs.modified - identifies whether the state of project is clean
vcs.time - in discussion

This change introduces new version schema:

v0.8.0 - release
v0.8.0-dev-xyxyxy - non release, at commit xyxyxy
v0.8.0-dev-xyxyxy-dirty - same as above, but also dirty
dev-xyxyxy - we don't know the tag, at commit xyxyxy
dev-xyxyxy-dirty - same but also dirty

smoyer64

I'd looked into how to best accomplish this a couple weeks ago and agree that the new debug.BuildInfo type is the way to go (pun intended) since the project is now >= Go v1.18. I'm not sure that we shouldn't still rely on the Make file for LAST_TAG and EXACT_TAG ... if there's no tag on HEAD, one common alternative would be to use the commit. I also like think we should be outputting a version that includes the dirty flag. Perhaps this library can be some inspiration - https://pkg.go.dev/github.com/carlmjohnson/versioninfo.

The Makefile also has a problem that we don't currently manage well (though it hasn't broken a release build yet) ... both git commands that used to be in the Makefile only use the last tag found on the commit. If there's more than one tag we're going to get whatever it contains (hopefully some sort of semantic version!)

smoyer64 · 2023-01-13T18:20:23Z

commands/root.go

+		}
+	}
+
+	cmd := exec.Command("git", "describe", "--tags", "--abbrev=0")


I'm not sure we want to run this every time a git-bug command is executed and, since we're already using go-git using exec might be a bit dangerous. Theoretically, git-bug is run inside a git repository and the executable should be available but perhaps not?

The bigger issue is that this would return tag information for the git repository where git-bug is run rather than tag information for the git-bug repository. I think I'd continue to ask the Makefile for this information.

@smoyer64 is correct, we want to bake into the binary which code was compiled.

@smoyer64 @MichaelMure thank you for the detailed response, I see what you say.

I think I was too fast, opening this PR.

Here are my suggestions for now, would like to validate that with you, before I move forward:

Use Makefile in the first place (revert all the changes I did)

If GitLastTag, GitCommit and GitExactTag are not defined - try to extract info from ReadBuildInfo (if that's possible and Go is 1.18+)

If by the end we do not have required info - return some sort of better defined version, as now it's -dev-

There is one more thing wort mentioning: ReadBuildInfo does return only vcs.revision, that is commit, and we cannot get tag from there.

Does that sound okay for you?

It seems that:

vcs.revision is the equivalent of GIT_COMMIT

vcs.modified tells if the original repo was dirty, which could be used to add a -dirty suffix on the version name

We are indeed missing the tag, which is quite annoying.

So ... what to do ... We could use those two variables when they are available, which I assume would at least partially fix #928. Ideally golang would also expose the tag, but I don't even see an issue in their tracker for that so it's not going to happen anytime soon.

But yeah, the tag extraction in the makefile need to stay for now.

So we would have different version schema:

v0.8.0 <-- release

v0.8.0-dev-xyxyxy <-- non release, at commit xyxyxy

v0.8.0-dev-xyxyxy-dirty <-- same but also dirty

dev-xyxyxy <-- we don't know the tag

dev-xyxyxy-dirty <-- same but also dirty

There's also vcs.time ... after I wrote the above I was also coming to the conclusion that we might want a different format. Fortunately, debug.BuildInfo.Main.Version will either contain the tag or the word "devel". The real problem (for me) is that I'll never install a release package, and I'd love to know the "base" release for what I'm working on!

In the examples above, the first three would have to be tagged AND would be produced by the Makefile right?

Related (with links to a lot more):

runtime/debug: document BuildInfo.Main.Version == "(devel)" golang/go#29228 (comment) (and whole issue)

cmd/go: stamp the pseudo-version in builds generated by go build golang/go#50603

Note that you can still use linker flags with go install - would users do that if so instructed?

Good reading about the build info and it's caveats.

Basically, the case when we can retrieve a tag from debug.BuildInfo.Main.Version will work only when we have a release, right?

There's also vcs.time

We can use time from the build info as well, but will this more useful info for the use?
In this case, it sounds better to use linker flags as you suggest - it will be more useful for those who really need full release. But I should admit that will have to research that, since have never used linker flags

In the examples above, the first three would have to be tagged AND would be produced by the Makefile right?

Yes, exactly. Updating PR description with that info again

For now, updated PR: reverted back changes in Makefile. The only change there, is --dirty flag. And added changes to return version per new schema described above

MichaelMure · 2023-01-18T17:30:40Z

Makefile

@@ -1,7 +1,7 @@
 all: build

-GIT_COMMIT:=$(shell git rev-list -1 HEAD)
-GIT_LAST_TAG:=$(shell git describe --abbrev=0 --tags)
+GIT_COMMIT:=$(shell git rev-parse --short HEAD)


can you explain those changes?

Oh, that change was unnecessary. I was thinking of returning a short commit hash to not to format it in the root

MichaelMure · 2023-01-18T17:36:15Z

commands/root.go

+
+	info, ok := debug.ReadBuildInfo()
+
+	if !ok {


if you can't read the build infos:

you should not continue reading the returned value, that could crash. Instead, return an error or a valid value.

you should not print anything

MichaelMure · 2023-01-18T17:37:11Z

commands/root.go

@@ -90,3 +107,28 @@ func Execute() {
 		os.Exit(1)
 	}
 }
+
+func getCommitAndDirty() (commit, dirty string) {


looks like dirty should be a boolean

MichaelMure · 2023-01-18T17:38:40Z

commands/root.go

+	// modified status (that is the flag for repository dirty or not)
+	for _, kv := range info.Settings {
+		switch kv.Key {
+		case "vcs.revision":


instead of going over all the values, you could simply try to read: if val, ok := info.Settings["vcs.revision"]; ok { ... }.

info.Settings returns an array of structs, so I can't access required values by key name

MichaelMure · 2023-01-19T10:07:12Z

@vasser I made some changes, mostly style, but also fixed one or two edge cases I found. Thanks for the help :-)

vasser · 2023-01-19T10:10:30Z

Great, happy to help 👍

Fixed version info be set when go install

fd40320

smoyer64 reviewed Jan 13, 2023

View reviewed changes

smoyer64 requested a review from MichaelMure January 13, 2023 18:46

New approach to define the version

83ffed8

MichaelMure reviewed Jan 18, 2023

View reviewed changes

vasser and others added 3 commits January 18, 2023 22:33

address PR review

68dcbab

dirty should be bool

61bfe18

version: code cleanup, fix some edge cases

5238d1d

MichaelMure merged commit 06e8396 into MichaelMure:master Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed version info be set when go install #991

Fixed version info be set when go install #991

vasser commented Jan 13, 2023 •

edited

smoyer64 left a comment •

edited

smoyer64 Jan 13, 2023

MichaelMure Jan 14, 2023

vasser Jan 14, 2023

MichaelMure Jan 14, 2023

MichaelMure Jan 14, 2023

smoyer64 Jan 14, 2023 •

edited

smoyer64 Jan 14, 2023

smoyer64 Jan 14, 2023

vasser Jan 15, 2023

MichaelMure Jan 18, 2023

vasser Jan 18, 2023

MichaelMure Jan 18, 2023

MichaelMure Jan 18, 2023

MichaelMure Jan 18, 2023

vasser Jan 18, 2023

MichaelMure commented Jan 19, 2023

vasser commented Jan 19, 2023

Fixed version info be set when go install #991

Fixed version info be set when go install #991

Conversation

vasser commented Jan 13, 2023 • edited

smoyer64 left a comment • 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

Choose a reason for hiding this comment

smoyer64 Jan 14, 2023 • 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

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

MichaelMure commented Jan 19, 2023

vasser commented Jan 19, 2023

vasser commented Jan 13, 2023 •

edited

smoyer64 left a comment •

edited

smoyer64 Jan 14, 2023 •

edited