Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swarm/version: commit version added #1159

Closed
wants to merge 2 commits into from

Conversation

gluk256
Copy link

@gluk256 gluk256 commented Jan 23, 2019

fixes #1133

@nonsense
Copy link
Contributor

I am not sure this will work, we set main.gitCommit when building, not version.GitCommit. I'd like to avoid having to set the same variable twice during compilation if possible.

@@ -41,6 +41,10 @@ var VersionWithMeta = func() string {
return v
}()

// this variable will be assigned if corresponding parameter is passed with install, but not with test
// e.g.: go install -ldflags "-X main.gitCommit=ed1312d01b19e04ef578946226e5d8069d5dfd5a" ./cmd/swarm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is wrong, should say

version.GitCommit

instead of

main.gitCommit

@@ -51,6 +55,7 @@ func ArchiveVersion(gitCommit string) string {
}
if len(gitCommit) >= 8 {
vsn += "-" + gitCommit[:8]
GitCommit = gitCommit[:8]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant. We can just use this function, it also gives the current version, next to the commit, which is not bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only this is redundant, but actually wrong - GitCommit is set during build time, we should not be modifying it.

@@ -59,6 +64,7 @@ func VersionWithCommit(gitCommit string) string {
vsn := Version
if len(gitCommit) >= 8 {
vsn += "-" + gitCommit[:8]
GitCommit = gitCommit[:8]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant. We can just use this function, it also gives the current version, next to the commit, which is not bad.

@nonsense
Copy link
Contributor

@gluk256 I see that you are adding version.GitCommit, which makes sense, as we cannot use main.gitCommit since it is not exported and will result in circular dependency.

In this case, please remove main.gitCommit altogether, and refactor main to use version.GitCommit. This way we will only have to set GitCommit during build time in the version package, and not in the main package.

Copy link
Contributor

@nonsense nonsense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - you can open on ethereum:master and will merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include commit in bzz.hive console output
2 participants