Add prototype gopackagesdriver based on `bazel query`. by ribrdb · Pull Request #2835 · bazel-contrib/rules_go

ribrdb · 2021-03-03T21:38:02Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?
Add a prototype implementation of a gopackagesdriver.

Which issues(s) does this PR fix?

Fixes #512

Other notes for review
gopls has recently stopped supporting gopackagesdriver. You need a version earlier than 092357f697aefc45480a378b64b698e063e63da9, or you need to revert that commit and rebuild gopls.

Also, you should make sure your editor excludes the following patterns, or gopls can go crazy scanning them:

*.runfiles/**
bazel-{workspace_name}/**
bazel-out/**

Don't exclude all of bazel-bin, you do want it to notice when generated files get re-built.

jayconrod

Added a few low level comments. I haven't read through pkgconv yet. I hope other maintainers will chime in as well.

My main concern is that the approach based on bazel query isn't going to work for more complicated requests, especially those involving cgo, generated code, and custom rules that are compatible with rules_go. This also won't work for a driver that runs inside an action, where we can't invoke Bazel.

The design doc in Editor and tool integration is more the direction I think this should go. The builder can generate package JSON files, either as part of GoCompilePkg or another action, then the driver would build the appropriate targets (either using an aspect or an output group), absolutize the paths, then hand those JSON objects back to the caller.

jayconrod · 2021-03-09T21:01:39Z

+
+const fileQueryPrefix = "file="
+
+var gorootPattern = regexp.MustCompile(`^bazel-[^/]+/external/go_sdk\b`)


We should try to avoid hard-coding the name go_sdk. People could use different names and have multiple SDKs declared. Is there a way to pass in GOROOT explicitly or infer it some other way? @io_bazel_rules_go//:stdlib at least would have that information.

Maybe you could configure your editor preferences to set GOROOT for gopls, but that seems difficult.
I couldn't figure out any way to infer this. I only get the rule attribute values from bazel query, and I don't see anything useful in @io_bazel_rules_go//:stdlib
aquery also doesn't help, because @io_bazel_rules_go//:stdlib doesn't seem to have any actions

It's reported through a provider, but I don't think there's any way to access that from the command line. This is part of why I think build actions (either the normal actions or actions created by an aspect) should generate most of this information: they have much more direct access to it.

It's fine to add a // TODO(#512) here for now (and for most other comments here that can't be quickly resolved).

jayconrod · 2021-03-09T21:10:15Z

+    # Needed for gopackagesdriver
+    _maybe(
+        http_archive,
+        name = "com_github_bazelbuild_bazel_watcher",


I'm really really leery of adding new dependencies. It adds a great deal of maintenance complexity.

It looks like these packages are used to invoke Bazel commands and to parse bazel query output. Can we do something more minimal with our own package here?

Would copying the protos and bazel.go here be better?
If so, should I put them into //third_party or here?

Copying the pre-generated .pb.go files is good. They can go anywhere under go/tools/gopackagesdriver; nothing else should use them.

ribrdb · 2021-03-11T00:17:09Z

Added a few low level comments. I haven't read through pkgconv yet. I hope other maintainers will chime in as well.

My main concern is that the approach based on bazel query isn't going to work for more complicated requests, especially those involving cgo, generated code, and custom rules that are compatible with rules_go. This also won't work for a driver that runs inside an action, where we can't invoke Bazel.

Yep, 100% agree. I think bazel aquery might be able to work instead of query, but integrating with the builder seems much better. I'm not sure what you mean about running "inside an action" though.

The design doc in Editor and tool integration is more the direction I think this should go. The builder can generate package JSON files, either as part of GoCompilePkg or another action, then the driver would build the appropriate targets (either using an aspect or an output group), absolutize the paths, then hand those JSON objects back to the caller.

I haven't seen this doc before, I'll try to digest it.

sluongng · 2021-03-25T12:21:31Z

Hey folks, how is this PR coming along?

@steeve I saw on Twitter you were also working in this area? Perhaps it could be duplicated effort?

steeve · 2021-03-30T14:23:11Z

Ah yes probably! @ribrdb I'm working on a version that's leveraging aspects and output groups. It was a off work weekend hack, but I got it to the point of it working with x/tools/packages: see https://github.com/znly/rules_go/tree/steeve/packagedriver

jayconrod · 2021-03-30T21:33:23Z

I'm going to step back from reviewing this. Unfortunately I have very little time this year available to work on rules_go and Gazelle, so I'm probably blocking progress at this point. I'm happy to answer high level questions or loop in the gopls folks, but the other folks in @bazelbuild/go-maintainers may have more insights, especially @steeve, who's been working on this lately.

One high level comment: I think aspects or output groups are the only way to do this. Each Go package needs a corresponding action that produces a file containing raw package data. That file should contain everything that could go into packages.Package, including the AST and type information. The gopackagesdriver should be able to build those, drop data that wasn't requested, absolutize paths, then hand those objects up to the client.

Since those objects will contain information that can only be obtained by reading source files (possibly including generated files), they can't be produced from bazel query alone.

It's possible these files could be produced as part of the GoCompilePkg action, since that reads those files anyway. If that's done, the files would need to be part of a separate output group so they aren't returned by default when you bazel build a library. Alternatively (perhaps more efficiently), they can be produced as part of a separate action, created by an aspect.

steeve · 2021-03-31T11:49:50Z

For the record, I have some prototype of a packages driver that's using aspects and output_groups. It's very done in my spare time, and I'm live tweeting it at https://twitter.com/steeve/status/1373247624929759232?s=20 (I don't always push my twitter ramblings on github)

I'll open a PR, @ribrdb, would love your insight on this, too.

As of last night it's able to generate stdlib info, although I'm not sure why, it's not working in gopls yet (even though it reports talking to the driver and no error in the return values).

ribrdb · 2021-03-31T13:41:20Z

@steeve It looks like your aspect is building all the JSON using starlark and then outputting it using write actions.
I tried a similar approach last year and it does not scale. Bazel ends up keeping all of the json files in memory, and on a project with lots of dependencies this easily adds up to bazel becoming unusably slow.
If we use an aspect it needs to ctx.actions.run to generate the .json files. For the sdlib itself I expect we could just run go list

ribrdb · 2021-03-31T14:06:18Z

Oh, reading through twitter it sounds like you've got a lot more that what's currently on your github. I'd be happy to take a look when it's up.

steeve · 2021-04-03T21:07:26Z

@ribrdb it's up at #2858

Add bazel query based gopackagesdriver.

a2e4fe7

ribrdb requested a review from jayconrod as a code owner March 3, 2021 21:38

google-cla Bot added the cla: yes label Mar 3, 2021

jayconrod requested a review from a team March 3, 2021 21:58

jayconrod reviewed Mar 10, 2021

View reviewed changes

ribrdb added 4 commits March 11, 2021 10:04

Address PR comments.

3b9ffd6

Add godoc for package pkgconv.

a814eaf

Update copyright headers.

97a18db

Implement more code review feedback.

c5e76b0

jayconrod requested a review from a team March 30, 2021 21:23

ribrdb closed this May 11, 2021


		const fileQueryPrefix = "file="

		var gorootPattern = regexp.MustCompile(`^bazel-[^/]+/external/go_sdk\b`)

Uh oh!

Conversation

ribrdb commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayconrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jayconrod Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ribrdb Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

jayconrod Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

jayconrod Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ribrdb Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

jayconrod Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ribrdb commented Mar 11, 2021

Uh oh!

sluongng commented Mar 25, 2021

Uh oh!

steeve commented Mar 30, 2021

Uh oh!

jayconrod commented Mar 30, 2021

Uh oh!

steeve commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ribrdb commented Mar 31, 2021

Uh oh!

ribrdb commented Mar 31, 2021

Uh oh!

steeve commented Apr 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ribrdb commented Mar 3, 2021 •

edited

Loading

steeve commented Mar 31, 2021 •

edited

Loading