-
Notifications
You must be signed in to change notification settings - Fork 617
version: add fallback to fill revision from BuildInfo #1637
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
Conversation
| ) | ||
|
|
||
| func init() { | ||
| if info, ok := debug.ReadBuildInfo(); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check first Revision is empty to avoid extra call to build info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this information gets used when buildx is used as a module? Because in that case it would be showing version from "whatever project used it as a module"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱 ah! I think you're right, ugh.
We could try and read through the modules first, and look for buildx in there, so we don't hit that? Frustratingly, BuildInfo.Main.Version doesn't get populated, so we can't pull from there.
Maybe we should drop this PR, and avoid clever guessing, since using buildx as a module is definitely a valid use case (e.g. in compose, in docker desktop, etc). This PR is mostly just to provide a fallback for #1620 anyways - we should push the reading of buildinfo down into the logic there, instead of trying to make it generic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think vcs.* is only populated at build time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - but I think it's populated by whatever vcs information is in the build.
So, if compose uses buildx as a module, then with this patch, we'd populate Revision with compose's revision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to close this PR, don't think it's worth adding all this extra complex logic just to find a revision.
4b780ef to
6c6e5a9
Compare
crazy-max
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, PTAL @thaJeztah
If Revision is not set by the linker, fallback to load the revision in the same format as generated by ./hack/git-meta. Unfortunately, we have no access to the tags, so we can't have a fallback for the Version field. Signed-off-by: Justin Chadwell <me@jedevc.com>
6c6e5a9 to
0d2a90e
Compare
🛠️ As mentioned in #1620 (comment).
If Revision is not set by the linker, fallback to load the revision in the same format as generated by ./hack/git-meta.
Unfortunately, we have no access to the tags, so we can't have a fallback for the Version field.