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

Comments

Projects
None yet
5 participants
@kevinburke
Contributor

kevinburke commented Jul 29, 2017

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 label Jul 29, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jul 29, 2017

Member

@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.

Member

bradfitz commented Jul 29, 2017

@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.

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 6, 2017

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

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

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira 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?

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

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Aug 6, 2017

Contributor
Contributor

kevinburke commented Aug 6, 2017

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 6, 2017

Yes, godash.

Yes, godash.

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 7, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 7, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 7, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 7, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 7, 2017

judepereira commented Aug 7, 2017

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 12, 2017

@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.

@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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 15, 2017

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

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

@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Aug 15, 2017

Contributor

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.

Contributor

kevinburke commented Aug 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 15, 2017

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?

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 18, 2017

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

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

@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Aug 23, 2017

Contributor

@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.

Contributor

kevinburke commented Aug 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 24, 2017

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

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

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 28, 2017

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

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

@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Aug 28, 2017

Contributor

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.

Contributor

kevinburke commented Aug 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Aug 28, 2017

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

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

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 3, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 3, 2017

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?

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 5, 2017

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?

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

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Sep 5, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 6, 2017

Thanks a lot @andybons! That worked very well.

Thanks a lot @andybons! That worked very well.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 6, 2017

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

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

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 6, 2017

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 :)

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

x/build/gerrit: add support for querying accounts in Gerrit
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

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 17, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Sep 18, 2017

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?

Member

andybons commented Sep 18, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Sep 18, 2017

Contributor
Contributor

kevinburke commented Sep 18, 2017

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 24, 2017

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

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

@judepereira

This comment has been minimized.

Show comment
Hide comment
@judepereira

judepereira Sep 24, 2017

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment