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

x/build/cmd/cl: build broken #21216

Closed
kevinburke opened this issue Jul 29, 2017 · 32 comments
Closed

x/build/cmd/cl: build broken #21216

kevinburke opened this issue Jul 29, 2017 · 32 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@kevinburke
Copy link
Contributor

x/build/godash was removed recently, but x/build/cmd/cl depended on that package, and is now broken. The right answer is to rewrite it using godata or (maybe) remove it, if no one's using that tool anymore.

https://travis-ci.org/kevinburke/build/jobs/258917130

$ go install ./cmd/cl/...
cmd/cl/cl.go:95:2: cannot find package "golang.org/x/build/godash" in any of:
	/Users/kevin/src/golang.org/x/build/vendor/golang.org/x/build/godash (vendor tree)
	/Users/kevin/go/src/golang.org/x/build/godash (from $GOROOT)
	/Users/kevin/src/golang.org/x/build/godash (from $GOPATH)
@gopherbot gopherbot added this to the Unreleased milestone Jul 29, 2017
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jul 29, 2017
@bradfitz
Copy link
Contributor

@andybons, I think @rsc still uses this tool.

Probably what you should do is take the devapp logic that's converting *maintner.Corpus -> your data structure for the devapp HTML template and instead move that into a Go package that x/build/cmd/cl can use to render the same data in text format from a CLI.

Or, better: make cmd/cl just be an HTTP client that hits https://dev.golang.org/foo.txt and returns the same data in text format. Then cmd/cl won't have the maintner start-up latency.

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 29, 2017
@judepereira
Copy link

I'd like to take a shot at this. Have it working partially for now. Will keep you guys updated.

@judepereira
Copy link

judepereira commented Aug 6, 2017

@bradfitz I'm on the path of importing the required code (bits and pieces only) from godash. Am I on the right path?

@kevinburke
Copy link
Contributor Author

kevinburke commented Aug 6, 2017 via email

@judepereira
Copy link

Yes, godash.

@judepereira
Copy link

judepereira commented Aug 7, 2017 via email

@bradfitz
Copy link
Contributor

bradfitz commented Aug 7, 2017

My first comment above contains two possible designs. I think the latter one is better.

Neither of them involve making Gerrit or GitHub API calls.

@judepereira
Copy link

judepereira commented Aug 7, 2017 via email

@bradfitz
Copy link
Contributor

bradfitz commented Aug 7, 2017

The intent of this bug is to change the cl tool's backend only (to maintner, probably via dev.golang.org or gRPC calls to https://maintner.golang.org), and not change its CLI UI or output at all.

@judepereira
Copy link

judepereira commented Aug 7, 2017 via email

@judepereira
Copy link

@bradfitz I got around to using the gRPC calls to maintner. However, most of the relevant information is not sent back as part of the api spec. Should I expose the stuff that's required for build/cmd/cl in build/maintner/maintnerd?

I see that all the data that I required for cl is already present and available in maintnerd's api.go, but just not being sent down.

@judepereira
Copy link

Any updates here? I'm really looking forward to fixing this issue.

@kevinburke
Copy link
Contributor Author

hey Jude - really sorry about this. Instead of reverse engineering the GRPC calls, it's probably better to expose a new API on the Corpus with the data that you need, see the ForeachXX API's that already exist. The data probably already exists in the protobuf files on disk, we just need to expose it properly.

@judepereira
Copy link

Hey Kevin,
Do you mean this here?

As of what I understand, using the Corpus would make cmd/cl take 500 MB worth of memory (described here)

I think the gRPC way is slightly more efficient here - it would get fully processed data (and not use a ton of memory).

Or am I missing something?

@judepereira
Copy link

What do you guys think? Should I go the gRPC way or the Corpus way?

@kevinburke
Copy link
Contributor Author

@judepereira sorry about this - for the moment we'd rather focus on downloading the Corpus and adding API's on it, I think. The good news is once you have it downloaded you don't need to do it again and you can run other tasks off of that.

@judepereira
Copy link

Thanks @kevinburke. I'll start work on it immediately. Should have something working early next week.

@judepereira
Copy link

@kevinburke What's the importance of the reviewer column? Is it still being used?

@kevinburke
Copy link
Contributor Author

Reviewer column in Gerrit is similar to the "Assign X" field in Github - it designates the person responsible for giving a Code-Review +2 to the PR.

If there's any way to break this up into chunks - I'm happy to review smaller parts to avoid any confusion.

@judepereira
Copy link

Thanks, got it. Going to implement something similar to what it was in godash earlier.

@judepereira
Copy link

I've hit a roadblock, which I've been trying to find a way around for the past three days. The reviewer information in the mutation log is of the form "Brad Fitzpatrick 5065@62eb7196-b449-3ce5-99f1-c037f21e1705".

There seems to be no way to resolve a Gerrit UID (5065 in the example) to a person's real email address.

The best that I can do is to link it via a user's name (using a name to email map - where I get the actual email via the author line, and then use it to resolve a reviewer's email address).

Is there a better way to do this? Or am I missing something obvious here? None of the reviewer lines contain the reviewer's actual email address.

This is the only thing that's remaining. Everything else works well with the Corpus.

@judepereira
Copy link

I can use this API to download and cache the Gerrit account IDs to email addresses, and call it whenever I find a new account ID which is not mapped.

What do you think?

@judepereira
Copy link

Strangely, I'm not able to use that API - although I see that I'm a part of the Registered Users group on go-review.googlesource.com, when I hit the list groups API (via the gerrit.NewClient()), I don't see those groups.

If I hit the GetAccountInfo API with self, it shows me my account info (account ID, email address and name).

Is the Gerrit list groups API broken?

@andybons
Copy link
Member

andybons commented Sep 5, 2017

So it’s not broken, but limited. There is no programmatic way to get access to Registered Users via the /groups/ endpoint.

You can try using /accounts/ search (possibly with is:active to trim it down) to get all users.

@judepereira
Copy link

Thanks a lot @andybons! That worked very well.

@gopherbot
Copy link

Change https://golang.org/cl/61970 mentions this issue: x/build/gerrit: add support for querying accounts in Gerrit

@judepereira
Copy link

As a pre-requisite for this issue to be fixed, I've added QueryAccounts in the the gerrit client. Please review when feasible - https://go-review.googlesource.com/c/build/+/61970

As this is ready, I should be able to get cmd/cl to be working as it was before :)

gopherbot pushed a commit to golang/build that referenced this issue Sep 7, 2017
Added support for querying accounts in Gerrit. This is a pre-requisite
for golang/go#21216

Change-Id: Ic6776ddf18a23e347d0eb7edf91a934d2feb01c9
Reviewed-on: https://go-review.googlesource.com/61970
Reviewed-by: Kevin Burke <kev@inburke.com>
@judepereira
Copy link

Okay, I've managed to get this working. Here's how I've solved the resolving of Gerrit email addresses to actual email addresses:

  • On the very first run, fetch all the accounts information mapping and serialise it to a file - stored alongside golang-maintner (it's called golang-gerrit/accounts.bin)
  • Now, for subsequent runs, deserialise this map, and use it
  • Let's say that a new user comes in - this entry will not be present in the Gerrit map. Make a new call to fetch all the accounts info once again, and serialise the mapping back on disk
  • If for whatever reason a Gerrit email address could not be mapped to an actual email address, then a default entry will be set, which is the same as the Gerrit email address (so far, this has happened for 10 open CLs)

Does that sound good? If yes, I'll have a commit available for review this week.

@andybons
Copy link
Member

  • On the very first run, fetch all the accounts information mapping and serialise it to a file - stored alongside golang-maintner (it's called golang-gerrit/accounts.bin)

I would call it golang-build-cmd-cl or something maybe less specific but enough that we know where the binary that's writing to it is located.

I have no other concerns but @kevinburke what do you think?

@kevinburke
Copy link
Contributor Author

kevinburke commented Sep 18, 2017 via email

@gopherbot
Copy link

Change https://golang.org/cl/65770 mentions this issue: cmd/cl: remove broken dependency on godash

@judepereira
Copy link

I've got it working, as it was before, except that the following is a todo for me:

  1. Output as JSON
  2. Support for the do-not-review flag
  3. Support for postpone to next Go release

Please review when feasible. This is my first major commit, so I expect a lot of updates to be made. I'm open to all of them. Thanks.

@golang golang locked and limited conversation to collaborators Oct 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants