version: use git hash instead of file blob hash for Version.Build #3987
Conversation
codeaucafe
changed the title
codeaucafe
force-pushed
the
codeaucafe/3986/fix-build-git-commit-hash
branch
2 times, most recently
from
3031118 to
ce85e17
Compare
|
@aarzilli I made changes. I realized the issue was the fact that we were trying to set a struct's field with linker flags doesn't work. Linker flags only work with package-level fields golang/go#26042 (comment) |
| } | ||
| } | ||
|
|
||
| // If we didn't find vcs.revision, try the old key for backward compatibility |
There was a problem hiding this comment.
leaving as backwards compatibility fall back
No we are not setting a struct field with a linker flag. We are setting |
|
sorry, I completely misread that when I was reading and debugging/testing it, let me remove and properly review, as well as look further into |
codeaucafe
force-pushed
the
codeaucafe/3986/fix-build-git-commit-hash
branch
from
ce85e17 to
3faaebd
Compare
|
@aarzilli should be good to review now. From my further understanding, I believe the fix should simply be the additions I made in I also updated issue #3986 accordingly too |
codeaucafe
force-pushed
the
codeaucafe/3986/fix-build-git-commit-hash
branch
from
3faaebd to
6ad547c
Compare
codeaucafe
changed the title
codeaucafe
force-pushed
the
codeaucafe/3986/fix-build-git-commit-hash
branch
2 times, most recently
from
f1a8e4f to
45d6689
Compare
The build ID `dlv version`command is the blob hash of file`pkg/version/version.go`. However, generally build identifier semantics are the built/installed binaries git commit hash. Updates buildInfoFixBuild to use the git commit has for Version.Build. Go build info's git revision hash key is `vcs.revision`. So, I added logic to search for that and attach its value to the version.Build field. If nothing is found and haven't return its value, then we try searching for `gitrevision` which maintains backwards compatibility. Fixes go-delve#3986
codeaucafe
force-pushed
the
codeaucafe/3986/fix-build-git-commit-hash
branch
from
45d6689 to
13cc389
Compare
|
|
||
| func buildInfoFixBuild(v *Version) { | ||
| // Return if v.Build already set, but not if it is Git ident expand file blob hash | ||
| if !strings.HasPrefix(v.Build, "$Id: ") && !strings.HasSuffix(v.Build, " $") { |
There was a problem hiding this comment.
technically don't need this IF the $Id$ is removed but I think its best to keep as fallback/backwards compatibility
pkg/version/version.go
Outdated
| var DelveVersion = Version{ | ||
| Major: "1", Minor: "24", Patch: "2", Metadata: "", | ||
| Build: "$Id$", | ||
| Major: VersionMajor, |
3 participants
Description
Fixes #3986
Problem
Prior to this change, the Delve debugger's version information (via$Id$ . Upon reviewing the code a handful of times I wonder if this is intended, given we search for gitrevision key and try to write its value to Version.Build. From this logic in
dlv versioncommand) displays the blob hash ofpkg/version/version.goas the a Git ident identifierbuildInfoFixBuildI would assume we would want the latest commit hash dlv is built with and not the file blob hash ofpkg/version/version.go. That, and many users (myself included) originally though Build should/would represent the commit hash delve was installed/built with (thus the reason for my going down this rabbit hole surmising why its done this way)If this should in fact be the built git hash then
buildInfoFixBuildfunc infixbuild.goneeds to be updated to search for the git hash invcs.revisionkey's value rather than the current implementation searching forgitrevision.Solution
Add settings search for
vcs.revisionand if it doesn't find it, we fall back to searching forgitrevisionto maintain backwards compatibility (I couldn't determine where that was used previously, but maybe a linker flag from awhile ago?)Note
If we do want to instead keep the Build Id as the blob hash of
pkg/verison/version.gothen I'm happy to close this PR, but the semantics of Build generally mean the commit hash the binary was installed/built with.