search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.55k stars 17.61k forks source link

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

Closed kevinburke closed 7 years ago

kevinburke commented 7 years ago

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)
bradfitz commented 7 years ago

@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 commented 7 years ago

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

judepereira commented 7 years ago

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

kevinburke commented 7 years ago

Sorry - by goodish do you mean godash?

On Sun, Aug 6, 2017 at 10:57 Jude Pereira notifications@github.com wrote:

@bradfitz https://github.com/bradfitz I'm on the path of importing the code from goodish, just about what's required. Am I on the right path?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/21216#issuecomment-320522071, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOSI0zbA0qvPYQekaJONRnkF2TCsMlrks5sVf6ZgaJpZM4Ona4Z .

--

-- Kevin Burke 925.271.7005 | kev.inburke.com

judepereira commented 7 years ago

Yes, godash.

judepereira commented 7 years ago

Yes, I am aware. I thought of importing the parsing methods from it.

A cleaner way would be to just print the ChangeInfo object from the gerrit package, without doing some extra stuff that cmd/cl wants to do.

On 07-Aug-2017, at 04:06, Andrew Bonventre notifications@github.com wrote:

We’ve removed godash — https://go-review.googlesource.com/c/50654

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

bradfitz commented 7 years ago

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 commented 7 years ago

What would the output be like then? Also, what's the endpoint for change lists? The gerrit code review endpoint? It wouldn't be console friendly.

Not quite sure what use it'd be of then.

On 07-Aug-2017, at 10:03, Brad Fitzpatrick notifications@github.com wrote:

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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

bradfitz commented 7 years ago

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 commented 7 years ago

Got it. Let me take a look at maintner. Will let you know how it goes. Thanks!

judepereira commented 7 years ago

@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 commented 7 years ago

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

kevinburke commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

kevinburke commented 7 years ago

@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 commented 7 years ago

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

judepereira commented 7 years ago

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

kevinburke commented 7 years ago

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 commented 7 years ago

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

judepereira commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Thanks a lot @andybons! That worked very well.

gopherbot commented 7 years ago

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

judepereira commented 7 years ago

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

judepereira commented 7 years ago

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

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

andybons commented 7 years ago

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 commented 7 years ago

(I'm on vacation and don't have very strong feelings - I can look into this more next Monday if you like)

On Mon, Sep 18, 2017 at 21:37 Andrew Bonventre notifications@github.com wrote:

  • 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 https://github.com/kevinburke what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/21216#issuecomment-330347874, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOSI5oFkJRuC40h-LAQsvi-4WlSf7Eiks5sjtRvgaJpZM4Ona4Z .

--

-- Kevin Burke 925.271.7005 | kev.inburke.com

gopherbot commented 7 years ago

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

judepereira commented 7 years ago

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.