Add Date to Unstable #19522

cbdotguru · 2019-05-02T20:41:23Z

Closes #19517

The second commit is a bit out of scope, let me know if that's a problem. This should work now.

karalabe · 2019-05-02T20:46:41Z

params/version.go

+		return time.Unix(ti, 0).Format("20060102")
+	}
+	return ""
+}


This solution would require the entire git repository to be available during Geth's execution. You need to do this during build time when the source code is available.

Yeah, just noticed this when I was running that it was being pulled during the call. Give me a few minutes...

okey dokey, updated. this solution is a bit more involved but it appears this is what you're looking for. let me know!

cbdotguru · 2019-05-02T22:12:42Z

updated @karalabe

cbdotguru · 2019-05-03T00:02:30Z

CI fails appear to be unrelated.

holiman

Generally this looks good to me. Have not tested it

holiman · 2019-05-03T07:42:26Z

Don't know if this was already broken (I suppose so), but it appears it cannot build if git is not present:

[user@work go-ethereum]$ make
build/env.sh go run build/ci.go install ./cmd/geth
env.go:121: Could not get gitCommit date: 
util.go:78: git tag -l --points-at HEAD: exec: "git": executable file not found in $PATH
exit status 1
make: *** [Makefile:15: geth] Error 1
[user@work go-ethereum]$

It looks like we do want to build anyway, but at least linux/fedora gives a different error than we're expecting:

func RunGit(args ...string) string {
	cmd := exec.Command("git", args...)
	var stdout, stderr bytes.Buffer
	cmd.Stdout, cmd.Stderr = &stdout, &stderr
	if err := cmd.Run(); err == exec.ErrNotFound {
		if !warnedAboutGit {
			log.Println("Warning: can't find 'git' in PATH")
			warnedAboutGit = true
		}
		return ""
	} else if err != nil {
		log.Fatal(strings.Join(cmd.Args, " "), ": ", err, "\n", stderr.String())
	}
	return strings.TrimSpace(stdout.String())
}

holiman · 2019-05-03T07:45:10Z

Ah, on lp_unix.go, the ErrNotFound is actually wrapped:

 	return "", &Error{file, ErrNotFound}

holiman · 2019-05-03T07:53:23Z

This seems to work

diff --git a/internal/build/util.go b/internal/build/util.go
index 319c0f2d3..a28b51895 100644
--- a/internal/build/util.go
+++ b/internal/build/util.go
@@ -68,13 +68,19 @@ func RunGit(args ...string) string {
 	cmd := exec.Command("git", args...)
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout, cmd.Stderr = &stdout, &stderr
-	if err := cmd.Run(); err == exec.ErrNotFound {
-		if !warnedAboutGit {
-			log.Println("Warning: can't find 'git' in PATH")
-			warnedAboutGit = true
+	err := cmd.Run()
+	if err != nil {
+		if e, ok := err.(*exec.Error); ok {
+			if e.Err == exec.ErrNotFound {
+				if !warnedAboutGit {
+					log.Println("Warning: can't find 'git' in PATH")
+					warnedAboutGit = true
+				}
+				return ""
+			}
 		}
-		return ""
-	} else if err != nil {
+	}
+	if err != nil {
 		log.Fatal(strings.Join(cmd.Args, " "), ": ", err, "\n", stderr.String())
 	}
 	return strings.TrimSpace(stdout.String())

I don't know, @karalabe, is this worth fixing now while we're at it?

holiman · 2019-05-03T07:54:57Z

Fwiw, windows also appears to wrap the erorr, so building without git can't work on windows either right now.

holiman · 2019-05-03T07:58:36Z

Also, if there's no git repo present, it will show

INFO [05-03|09:57:25.566] Starting peer-to-peer node               instance=Geth/v1.9.0-unstable-/linux-amd64/go1.11.2

Wouldn't it be nicer to show Geth/v1.9.0-unstable-NA or Geth/v1.9.0-unstable instead of Geth/v1.9.0-unstable-

karalabe

There's also a small catch. The date doesn't work in docker, because our .dockerignore doesn't pass the git repo in. Trying to figure out how best to actually handle this without hacing to forward the entire repo.

karalabe · 2019-05-03T07:33:41Z

cmd/utils/flags.go

-	if len(gitCommit) >= 8 {
-		app.Version += "-" + gitCommit[:8]
-	}
-	app.Usage = usage


You accidentally deleted setting the usage.

karalabe · 2019-05-03T07:34:11Z

internal/build/env.go

-	IsCronJob           bool
+	Name                            string // name of the environment
+	Repo                            string // name of GitHub repo
+	Commit, CommitDate, Branch, Tag string // Git info


I think if you call it Date, it's enough.

karalabe · 2019-05-03T07:36:19Z

internal/build/env.go

@@ -107,6 +114,23 @@ func firstLine(s string) string {
 	return strings.Split(s, "\n")[0]
 }

+func getCommitDate(commit string) string {
+	if commit != "" {


Slight nit, usually it's cleaner code to handle "error" cases first, returning; and only then the main clauses. It saves indentation. i.e.

if commit == "" { return "" } // rest of the code without indent

agreed, will change

karalabe · 2019-05-03T07:40:02Z

internal/build/env.go

@@ -107,6 +114,23 @@ func firstLine(s string) string {
 	return strings.Split(s, "\n")[0]
 }

+func getCommitDate(commit string) string {
+	if commit != "" {
+		out, err := exec.Command("git", "show", "-s", "--format=%ct", commit).CombinedOutput()


We have a utility method called build.RunGit, which does a bit more magic around failures. Please use that.

ok, will do

karalabe · 2019-05-03T07:46:41Z

internal/build/env.go

+		ti, err := strconv.ParseInt(strings.TrimSpace(string(out)), 10, 64)
+		if err != nil {
+			log.Println("Could not convert gitCommit date. Parsed timestamp is: " + err.Error())
+			return ""


I think at this point we should do a log.Fatal. We seem to have the proper repo, but something is very very wrong if we can't interpret it. Safer to break immediately than to silently disable the date feature without anybody noticing.

this makes sense

karalabe · 2019-05-03T07:48:46Z

build/ci.go

}
+

Pls remove this superfluous empty line.

karalabe · 2019-05-03T07:52:31Z

params/version.go

 	vsn := VersionWithMeta
 	if len(gitCommit) >= 8 {
 		vsn += "-" + gitCommit[:8]
 	}
+	if VersionMeta != "stable" {
+		vsn += "-" + gitDate


You need to also check that gitDate is not nil, otherwise you get:

INFO [05-03|10:50:08.272] [...] instance=Geth/v1.9.0-unstable-/linux-amd64/go1.12.3

note, the version has a trailing -

Question regarding this. We do want the date anytime it's not stable, so by nature a v1.9.0-unstable- would be an error. The question is, do we want to add the check here (essentially a bandaid) and drop the extra dash, or no? I think we do want to add here as I think about it because the other apps built, ie Swarm, end up with this extra dash because this process isn't fully built out there.

cbdotguru · 2019-05-03T12:47:52Z

I see that regarding the .dockerignore not passing in .git. Without this though, gitCommit is also not available, so how is this being handled? In CI, appveyor provides an env var that identifies the commit time, but travis does not have this. I initially started to plug in the ci vars available until I realized this inconsistency, then for the sake of consistency, reverted to using the utility across the board. So, we can use ci var for appveyor but travis needs to be figured out in that case. But my main question is, using docker, how is gitCommit fed into the build?

cbdotguru · 2019-05-07T16:46:58Z

This should do @karalabe . I think the git is ok, lmk if not

karalabe · 2019-05-08T10:35:04Z

@holiman We didn't need git until now during builds, because we injected 1-2 fiels from .git into docker and read them directly. That's why it didn't blow up. That said, we should definitely still be able to build both without git and without the git repo. I'll look into how I can fix that error check.

karalabe · 2019-05-08T10:41:19Z

@holiman Ah, I see you already debugged and wrote up the code. That SGTM, checked that it's not a recent Go change, 8 years old. The old code was just borked.

holiman · 2019-05-08T10:52:37Z

@karalabe did you mean to ping @HACKDOM and not me?

karalabe · 2019-05-08T10:54:28Z

@holiman No, I was just explaining what we've been doing with git in the past and now, since you brought up that builds failed.

karalabe

Added some tiny git fixes to make sure it works in docker and in our travis/appveyor builders too. LGTM.

Thanks for putting all that effort and iterations into this PR!

cbdotguru · 2019-05-08T14:57:38Z

ah... I see what you do there. very cool

karalabe · 2019-05-09T07:31:19Z

@HACKDOM Btw, we've been talking with @holiman that another potentially useful thing might be to include in the version whether it was built from a clean commit, or contains dirty code?

I.e. From time to time we get bug reports that something doesn't work, and it turns out that the OP modified Geth and that's the reason. Would be nice to be able to detect those cases, by prepending a -dirty as a version suffix if the git tree was not clean on build. With git now properly set up in this PR, it should be fairly straightforward to check if there are any modifications and signal that too to the binaries.

Would you like to pick such a task up? If not, no problem, I can also do it, just figured you might be interested since it's almost the same modification that you did here :)

cbdotguru mentioned this pull request May 2, 2019

Closes #19517 #19518

Closed

karalabe reviewed May 2, 2019

View reviewed changes

cbdotguru mentioned this pull request May 2, 2019

Add date to unstable version string #19517

Closed

cbdotguru force-pushed the master branch from 2ba2f29 to 798c90d Compare May 2, 2019 22:10

holiman approved these changes May 3, 2019

View reviewed changes

karalabe requested changes May 3, 2019

View reviewed changes

cbdotguru force-pushed the master branch from 61a97e6 to fb2ebcf Compare May 7, 2019 15:44

cbdotguru requested review from fjl and zsfelfoldi as code owners May 7, 2019 15:44

cbdotguru force-pushed the master branch 4 times, most recently from 4eee67d to 78e877b Compare May 7, 2019 16:43

H and others added 2 commits May 8, 2019 13:25

add-date-to unstable

2593ee5

fields-insteadof-split

727a917

karalabe force-pushed the master branch from 4f59952 to 727a917 Compare May 8, 2019 10:26

internal/build: support building with missing git

780b414

karalabe added 2 commits May 8, 2019 14:25

docker: add git history back to support commit date in version

cc4d720

internal/build: use PR commits hashes for PR builds

0bb1e76

karalabe approved these changes May 8, 2019

View reviewed changes

karalabe merged commit be4d74f into ethereum:master May 8, 2019

Add Date to Unstable #19522

Add Date to Unstable #19522

Conversation

cbdotguru commented May 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbdotguru commented May 2, 2019

cbdotguru commented May 3, 2019

holiman left a comment

Choose a reason for hiding this comment

holiman commented May 3, 2019

holiman commented May 3, 2019

holiman commented May 3, 2019

holiman commented May 3, 2019

holiman commented May 3, 2019

karalabe left a 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

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

cbdotguru May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbdotguru commented May 3, 2019 • edited Loading

cbdotguru commented May 7, 2019

karalabe commented May 8, 2019

karalabe commented May 8, 2019

holiman commented May 8, 2019

karalabe commented May 8, 2019

karalabe left a comment

Choose a reason for hiding this comment

cbdotguru commented May 8, 2019

karalabe commented May 9, 2019

cbdotguru May 3, 2019 •

edited

Loading

cbdotguru commented May 3, 2019 •

edited

Loading