-
Notifications
You must be signed in to change notification settings - Fork 17
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
Version: Change go_version to based on runtime, add additional #50
base: master
Are you sure you want to change the base?
Conversation
Given that go is statically compiled, I'm not sure why GoVersion is linked at build time, vs using runtime directly. Also, add it some additional runtime variables. While seemingly superfluous, these can add clarity in the rosetta2 environment.
I am not aware of any problem with the new approach. Perhaps the |
Not sure either. This change LGTM. |
@@ -53,7 +56,9 @@ func Version() Info { | |||
Version: version, | |||
Branch: branch, | |||
Revision: revision, | |||
GoVersion: goVersion, |
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.
Should probable be a TODO to delete this global or mark it as deprecated? I understand not wanting to break any scripts still setting it.
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 swapped it runtime.Version(), which seems a little superfluous with calling runtime. but fits nicely with the PrintFull
function. 🤷 Not sure this really gets any use
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.
My point is that the goVersion
variable in the var block is now unused.
|
Given that go is statically compiled, I'm not sure why GoVersion is linked at build time, vs using runtime directly.
Also, add it some additional runtime variables. While seemingly superfluous, these can add clarity in the rosetta2 environment.
@groob @zwass You both might know the history here. Is this a bad idea?