Skip to content
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

cmd/dist: wrong version computed #9296

Closed
rsc opened this issue Dec 12, 2014 · 13 comments
Closed

cmd/dist: wrong version computed #9296

rsc opened this issue Dec 12, 2014 · 13 comments
Assignees

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Dec 12, 2014

https://go-review.googlesource.com/#/c/1405/2 fixed cmd/api to put up with a runtime.Version() that doesn't begin with 'devel' but is a development branch (not a release-branch). That should never happen. cmd/dist should be fixed not to do that, and the CL should be rolled back so that cmd/api is not tolerating the bug.

@bradfitz @dsymonds

@minux
Copy link
Member

@minux minux commented Dec 12, 2014

the problem is not cmd/dist's. The builder will generate a VERSION file
containing only the sha1 commit hash.
That was necessary because hg clone is so slow on certain builders, and I
switched to hg export, which sped
up the process at least 10x on the slower builders, but without the .hg
directory, cmd/dist can't figure out the
version, so a hard-coded VERSION file was generated by the builder.

Now that we switched to git, we don't need that speed hack anymore.

@minux
Copy link
Member

@minux minux commented Dec 12, 2014

Do we still need to support hg in tools/dashboard/builder? If not, we can
get rid of the VERSION file creation step entirely. (Or we can switch hg
to use clone instead of archive)

My last comment had a typo, it should be "hg archive" not "hg export".

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2014

We can drop hg support.

@minux
Copy link
Member

@minux minux commented Dec 12, 2014

OK. I will do some test on the netbsd-386 builder.
And send a CL.

@bradfitz, could you clear the result for netbsd-386-minux builder
from the dashboard? To debug my changes, I need the dashboard
to give the builder some work to do. Thanks.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2014

We don't have an easy+safe way to clear a result or row or column from the dashboard yet. We should have such a mechanism.

Before we have a tool, I'm reluctant to touch anything by hand.

@minux
Copy link
Member

@minux minux commented Dec 12, 2014

My changes are working now. However it seems Gerrit has disabled upload from unregistered users.

Here is the patch: https://gist.github.com/minux/c2b1c270ac1c7ea1a0cc

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2014

However it seems Gerrit has disabled upload from unregistered users.

Gerrit has never allowed unauthenticated uploads. What do you mean?

But I don't think your patch is quite what we want. We still want to re-use the git repo $GOROOT we place at /goroot in the Linux docker images as a base, and only git pull up from that to our target build hash.

I fear that your patch will cause way too many git clone operations, which weigh in at 50+ MB each, and lots of disk I/O.

@minux
Copy link
Member

@minux minux commented Dec 13, 2014

This is the error I was getting:
$ git push origin HEAD:refs/for/master
fatal: remote error: Upload denied for project 'tools'

I also tried to create a change on the web interface, but I got the same
error for both tools and go.

My patch doesn't change how the git repository is handled. The current
builder will always git clone
a new workspace, and git clone will use hard links for the .git directory
if possible. (git clone is
even faster than hg archive last time i benchmarked....)

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 13, 2014

Oh, I see we've disabled people without +2 from pushing changes, until we fix some Gerrit configuration things about the CLA checks.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 13, 2014

@minux, you should be able to upload again.

@minux
Copy link
Member

@minux minux commented Dec 13, 2014

Thanks very much, @bradfitz, I've mailed golang.org/cl/1510

@dsymonds
Copy link
Member

@dsymonds dsymonds commented Dec 14, 2014

I did intend for the cmd/dist/build.c change to yield "devel ", but @rsc was getting me to make a bunch of changes during its review and I think it slipped aside in the process. I can look into fixing that tomorrow if no-one has already started.

@dsymonds dsymonds self-assigned this Dec 14, 2014
@dsymonds dsymonds added the bug label Dec 14, 2014
@dsymonds
Copy link
Member

@dsymonds dsymonds commented Dec 15, 2014

I went to start fixing cmd/dist, but couldn't see how the code could be omitting "devel". A quick experiment showed that it is working correctly at tip:

$ go version
go version devel +e5ee9a8 Sun Dec 14 23:54:15 2014 +0000 linux/amd64
$ cat > x.go
package main
import "runtime"
func main() { println(runtime.Version()) }
$ go run x.go
devel +e5ee9a8 Sun Dec 14 23:54:15 2014 +0000

So cmd/dist is working fine at tip.

@dsymonds dsymonds closed this Dec 15, 2014
gopherbot pushed a commit to golang/tools that referenced this issue Dec 18, 2014
The builder will no longer generate a VERSION file, so we
can revert https://golang.org/cl/1405 once all builders
have updated.

Fixes golang/go#9296.

Change-Id: Ie51cb06a712157c16b231167f166b31d10ba8667
Signed-off-by: Shenghou Ma <minux@golang.org>
Reviewed-on: https://go-review.googlesource.com/1510
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Chris Manghane <cmang@golang.org>
adg pushed a commit to golang/build that referenced this issue Jan 21, 2015
The builder will no longer generate a VERSION file, so we
can revert https://golang.org/cl/1405 once all builders
have updated.

Fixes golang/go#9296.

Change-Id: Ie51cb06a712157c16b231167f166b31d10ba8667
Signed-off-by: Shenghou Ma <minux@golang.org>
Reviewed-on: https://go-review.googlesource.com/1510
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Chris Manghane <cmang@golang.org>
dsymonds added a commit that referenced this issue Feb 8, 2015
This reverts commit 11d1c05.
See #9296 for details.

Change-Id: I89a36351cb007836662f28a611af5616818b95fe
Reviewed-on: https://go-review.googlesource.com/1536
Reviewed-by: Minux Ma <minux@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.