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

go/tools/gopackagesdriver: add automatic target detection #2932

Merged
merged 24 commits into from Aug 28, 2021
Merged

go/tools/gopackagesdriver: add automatic target detection #2932

merged 24 commits into from Aug 28, 2021

Conversation

steeve
Copy link
Contributor

@steeve steeve commented Aug 3, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR introduces automatic target detection so that no user input
is required after the GOPACKAGESDRIVER setup. This effectively
deprecates the following environment variables:

  • GOPACKAGESDRIVER_BAZEL_TARGETS
  • GOPACKAGESDRIVER_BAZEL_QUERY
  • GOPACKAGESDRIVER_BAZEL_TAG_FILTERS

It works as follows:

  • for importpath queries, it will bazel query a matching go_library
    matching it
  • for file= queries, it will try to find the matching go_library in the
    same package
  • since it supports multiple queries at the same time, it will union those
    queries and query that

Once the go_library targets are found, it will try to build them directly,
which should dramatically speed up compilation times, at the loss of transition
support (which wasn't used as much as I thought it would be).

I may reintroduce it in the future via a user-defined flag (to only build the
part of the graph that needs building).

In any case, toolchain or platforms can be switched with the following
environment variables it configuration transitions are needed:

  • GOPACKAGESDRIVER_BAZEL_FLAGS which will be passed to bazel invocations
  • GOPACKAGESDRIVER_BAZEL_QUERY_FLAGS which will be passed to bazel query
    invocations
  • GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE which specifies the scope for importpath queries (since gopls only issues file= queries, so use if you know what you're doing!)
  • GOPACKAGESDRIVER_BAZEL_BUILD_FLAGS which will be passed to bazel build
    invocations

Finally, the driver will not fail in case of a build failure, and even so
uses --keep_going and will return whatever packages that did build.

Which issues(s) does this PR fix?

It should fix most of the issues from #2858.

Other notes for review
I have added a .vscode folder for folks who work on rules_go with it so that gopls now works there too!

@google-cla google-cla bot added the cla: yes label Aug 3, 2021
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we continue to support GOPACKAGESDRIVER_BAZEL_QUERY etc? This is important for large repos like Uber's Go monorepo. Although it's huge, most users only work on small subset of targets and only need to index their projects and dependencies. It's quite a waste of time for them to wait for hours for package driver to load //...

go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
@steeve
Copy link
Contributor Author

steeve commented Aug 5, 2021

@linzhp I have updated the PR, it shouldn't load the whole //... for file queries now

@steeve
Copy link
Contributor Author

steeve commented Aug 5, 2021

@linzhp I have added GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE which should control the scope when issuing importpath queries. That said, you should not need it for gopls as it issues file= queries, and I have removed the whole universe loading for those.

@steeve
Copy link
Contributor Author

steeve commented Aug 8, 2021

@linzhp I have fixed lots of bugs, can you try this version?

@steeve
Copy link
Contributor Author

steeve commented Aug 8, 2021

Here's a summary of the changes:

  • when called without GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE, return stdlib packages by default (so that vscode doesn't complain)
  • add a compiled sources output group to ensure all files are generated and present in the sandbox
  • enable querying by importpath prefix (github.com/bazelbuild/rules_go/...)
  • fix path for external generated files
  • fix path for external source files

Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works now. Commented on other minor issues.

go/tools/gopackagesdriver/bazel.go Outdated Show resolved Hide resolved
go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
go/tools/gopackagesdriver/main.go Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Aug 17, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Aug 17, 2021
@steeve
Copy link
Contributor Author

steeve commented Aug 17, 2021

@linzhp can you confirm the CLA please?

@google-cla
Copy link

google-cla bot commented Aug 17, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@thinkiny
Copy link

can't wait this merge to master

Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are failing.

It's strange that googlebot thought I didn't have CLA, given I already had several accepted pull requests to this repo.

@googlebot I consent.

go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
go/tools/gopackagesdriver/main.go Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Aug 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@steeve
Copy link
Contributor Author

steeve commented Aug 18, 2021

The test failure is unrelated (race)

@steeve steeve added cla: yes and removed cla: no labels Aug 18, 2021
@google-cla
Copy link

google-cla bot commented Aug 18, 2021

☹️ Sorry, but only Googlers may change the label cla: yes.

@google-cla google-cla bot removed the cla: yes label Aug 18, 2021
@steeve
Copy link
Contributor Author

steeve commented Aug 18, 2021

@linzhp I have added 3512e59 which makes the gopackagesdriver honor the build tags when listing files. Can you try it ?

@steeve
Copy link
Contributor Author

steeve commented Aug 18, 2021

can't wait this merge to master

very soon !

go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
fp, _ := filepath.Rel(b.bazel.WorkspaceRoot(), filename)
filename = fp
}
return fmt.Sprintf(`kind("go_library", same_pkg_direct_rdeps("%s"))`, filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may return empty result when the filename is a test file like foo_test.go, because no go_library would depend on such file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, test files are not done atm, if you have an idea, i'm all ears

go/tools/gopackagesdriver/bazel_json_builder.go Outdated Show resolved Hide resolved
@linzhp
Copy link
Contributor

linzhp commented Aug 18, 2021

I have added 3512e59 which makes the gopackagesdriver honor the build tags when listing files. Can you try it ?

How should I try it? It seems to be a separate feature. Can you put it in a separate pull request?

@linzhp
Copy link
Contributor

linzhp commented Aug 18, 2021

The test failure is unrelated (race)

It failed consistently in recent commits, but not in earlier commits or master. So it may be broken by some recent commit

@google-cla google-cla bot added the cla: no label Aug 18, 2021
@steeve
Copy link
Contributor Author

steeve commented Aug 18, 2021

How should I try it? It seems to be a separate feature. Can you put it in a separate pull request?

Just verify that it correctly works as it did before. It's done behind the scenes. Basically gopls complained because all the go files were listed as part of the package, which would break some symbols.

Just check that the packagedriver continues to work on your repo.

@steeve
Copy link
Contributor Author

steeve commented Aug 18, 2021

@linzhp careful:

leaving a comment that contains only @googlebot I consent. in this pull request.

steeve and others added 24 commits August 21, 2021 15:13
This commit introduces automatic target detection so that no user input
is required after the `GOPACKAGESDRIVER` setup. This effectively
deprecates the following environment variables:
- `GOPACKAGESDRIVER_BAZEL_TARGETS`
- `GOPACKAGESDRIVER_BAZEL_QUERY`
- `GOPACKAGESDRIVER_BAZEL_TAG_FILTERS`

It works as follows:
- for `importpath` queries, it will `bazel query` a matching `go_library`
  matching it
- for `file=` queries, it will try to find the matching `go_library` in the
  same package
- since it supports multiple queries at the same time, it will `union` those
  queries and query that

Once the `go_library` targets are found, it will try to build them directly,
which should dramatically speed up compilation times, at the loss of transition
support (which wasn't used as much as I thought it would be).

I may reintroduce it in the future via a user-defined flag (to only build the
part of the graph that needs building).

In any case, toolchain or platforms can be switched with the following
environment variables it configuration transitions are needed:
- `GOPACKAGESDRIVER_BAZEL_FLAGS` which will be passed to `bazel` invocations
- `GOPACKAGESDRIVER_BAZEL_QUERY_FLAGS` which will be passed to `bazel query`
   invocations
- `GOPACKAGESDRIVER_BAZEL_BUILD_FLAGS` which will be passed to `bazel build`
  invocations

Finally, the driver will not fail in case of a build failure, and even so
uses `--keep_going` and will return whatever packages that did build.

Signed-off-by: Steeve Morin <[email protected]>
This allows to use gopls in the project itself.

Signed-off-by: Steeve Morin <[email protected]>
…eries

When using `--universe_scope=//...`, the whole `//...` is loaded even though
we only use `same_pkg_direct_rdeps`. So remove it and specify the scope in the
query, such as `importpath` ones.

Signed-off-by: Steeve Morin <[email protected]>
Add the `GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE` environment variable so that
bazel queries are limited the given scope. This should save time when doing
`importpath` queries on large monorepos.

Signed-off-by: Steeve Morin <[email protected]>
While at it, fetch and parse the whole bazel info data.
…ault

When fetching the whole graph, default to returning the stdlib packages so that
vscode doesn't complain.
Guard against packages with the same prefix by happending `/` to HasPrefix
and match the exact name if needed.
While it used to work, this is better.
When there is no such go_library, the some function would lead to errors like
ERROR: Evaluation of query failed: argument set is empty, with exit code 7. It
is unclear whether the query has a bug or there is no matching package. If we
don't have the some function, Bazel query will exit normally with empty result.
We can report a better error to gopls for this case.

Co-authored-by: Zhongpeng Lin <[email protected]>
This is done using a brand new build context that is configured using
the regular environment variables.

Signed-off-by: Steeve Morin <[email protected]>
Match a/ab and a/ab/c but not a/abc

Signed-off-by: Steeve Morin <[email protected]>
@google-cla
Copy link

google-cla bot commented Aug 21, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@steeve steeve merged commit 70b8365 into bazel-contrib:master Aug 28, 2021
@steeve steeve deleted the steeve/gopackagesdriver2 branch August 28, 2021 22:59
@thinkiny
Copy link

awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants