go/tools: add gopackagesdriver #2858
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 3, 2021
This commit introduces the GOPACKAGESDRIVER for rules_go Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
Those files are generated and will end up in the temporary mod cache, which isn't available later on. Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
This handles some edges cases in which the import path last part is not the package name. Signed-off-by: Steeve Morin <[email protected]>
It's no use, and it can be a significant amount of files, so disable it. Signed-off-by: Steeve Morin <[email protected]>
The indexing by ID was never used except for iterating on all packages. The file index isn't used either since the whole graph is dumped anyway. Signed-off-by: Steeve Morin <[email protected]>
Needed when in a CGo environment for go list to work Signed-off-by: Steeve Morin <[email protected]>
Fetch the stdlib JSON from the deepest target, and cascade it upward so that transitions are applied properly. Also, this enables applying the aspect to a target that depends on a go_binary, such as a cc_binary with proper transition applied. Signed-off-by: Steeve Morin <[email protected]>
This is more correct. Signed-off-by: Steeve Morin <[email protected]>
This ensures that all possible packages are built Signed-off-by: Steeve Morin <[email protected]>
The key types have to be copied and paster so might has well remove the dependency altogether. In the process, rework how the LoadMode is passed. Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
Since the packages driver is meant to be run via `bazel run`, it's simpler to leverage BUILD_WORKSPACE_DIRECTORY. It's one less parameter. Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
|
Made some changes regarding UX, should be simpler now. |
Signed-off-by: Steeve Morin <[email protected]>
Will be easier when debugging. Signed-off-by: Steeve Morin <[email protected]>
Sometimes if the aspects explores nodes that won't have go_ rules, there is no inner stdlib_json to fetch. Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
gopherbot
pushed a commit
to golang/tools
that referenced
this pull request
Apr 6, 2021
Now that https://github.com/bazelbuild/rules_go has a working prototype of a `GOPACKAGESDRIVER`, it may be time to revert that commit. The draft implementation is at bazel-contrib/rules_go#2858. Change-Id: Ia738e8be448d936f8a3b2b421d0a765f94bbff52 GitHub-Last-Rev: 0df6c91 GitHub-Pull-Request: #297 Reviewed-on: https://go-review.googlesource.com/c/tools/+/307169 Reviewed-by: Rebecca Stambler <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]> Trust: Rebecca Stambler <[email protected]> Run-TryBot: Rebecca Stambler <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Go Bot <[email protected]>
|
golang/tools#297 / https://go-review.googlesource.com/c/tools/+/307169 has been merged! |
|
The check failure seems transient and unrelated to the PR: Click to expand! |
Use special flags in order to speed up the bazel query Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
Cleaner than hasattr Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
Signed-off-by: Steeve Morin <[email protected]>
simply print it and exit(1) Signed-off-by: Steeve Morin <[email protected]>
…cher Signed-off-by: Steeve Morin <[email protected]>
Since root packages are now properly computed, walk the package graph from them. This saves on JSON payload size a bit. Signed-off-by: Steeve Morin <[email protected]>
|
@jayconrod great review, I pushed most of the fixes |
|
My understanding is that this will fail if the project is library-only and doesn't contain any binary -- because of the bazel query. Any ideas? |
|
@pierreis it should work with a library too |
|
I am getting the following error -- which disappears as soon as I create a binary target: |
|
It looks like your GOPACKAGESDRIVER_TARGETS or _QUERY isn't matching anything. On the example I did make it match to go_binary, you should tweak it. |
|
Right, I did assume from the instructions that it had to be this query for some reason -- incorrectly it seems. Thanks! |
|
Is this waiting on anything else? |
|
I think this is ready to merge, but we're still sorting out a repo access issue. We'll probably do a minor release with this soon after. |
|
Is there any more work to do regarding error handling? |
|
Maybe it should return |
|
My biggest pain point with the package driver at this moment is that, when using it, I loose all autocompletion or error highlighting as soon as the project fails to compile for any reason. Fixing the issue without error highlighting is much more painful than it should be. |
|
What's the latest status of this PR? It'd be soooo nice to have this merged. Improvements could happen iteratively. |
achew22
approved these changes
Jun 28, 2021
|
@steeve thank you for the hard work on this one. I'm delighted to open the bug report floodgates. |
|
Thank you @steeve! This is amazing. I'll start preparing for a minor release this week or next week. |
|
Great!!! When using gopackagedriver with The workspace changes are caused by the Putting |
|
@yancl I experienced the same thing and realized when copying the . That seemed to also fix it for me :) |
|
@raymondpebble Thanks for your information. The problem disappeared after I upgraded the version of the The |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces an aspect based
GOPACKAGESDRIVERforrules_go.It is rather clunky, but all VSCode +
goplsfeatures work with it:It does not support static usage at the moment (for instance by running it inside an action for codegen tools), but it could with few additions.
Usage
The packages driver fundamentally works from the perspective of one or more targets. In order
for it to function properly, the targets must be specified in the packages driver configuration,
with environment variables.
These are:
GOPACKAGESDRIVER_BAZEL_TARGETS: specifies specific targets,//...works too (although this is not recommended without tag filters). It is possible to set targets to something other thango_rules, as long as thego_nodes are reachable via thedepsattribute in the graph. Such as whengo_binary(linkmode = "c-archive") -> cc_library.GOPACKAGESDRIVER_BAZEL_QUERY: runs abazel queryto select targets before, common one should bekind(go_binary, //...). This should work for most cases.GOPACKAGESDRIVER_BAZEL_TAG_FILTERS: makes use of the--build_tag_filtersoption to only build (filter) targets with certain tags. This is useful for when you want each target to define wether or not it should be part of the packages list. For instance, setGOPACKAGESDRIVER_BAZEL_TARGETS="//..."andGOPACKAGESDRIVER_BAZEL_TAG_FILTERS="completion"to only build targets with thecompletiontag in the whole workspace.In addition, those environment variables are optional:
GOPACKAGESDRIVER_BAZEL: bazel binary, defaults tobazelBUILD_WORKSPACE_DIRECTORY: directory of the bazel workspace (should be auto detected when using a launcher script because it invokesbazel run)1.
goplsYou'll need
gopls >= v0.6.10(released on Apr 13th 2021). If you are using VSCode, it should be automatic.2. Create a launcher script
Create a launcher script, say
tools/gopackagesdriver.sh:3. Sample VSCode configuration
In
.vscode/settings.jsonadd the following:Open VSCode, and after a while the packages should be properly detected after the build is done.
The same principles should apply for vim-go or any editor that uses
gopls.Limitations
Query patterns are ignored and the whole graph is returned each timepatterns are now usedRoot packages detection is probably wrongit should be better nowTechnical details
Aspects
Most of the work is done by one aspect that generates a minimum package JSON entry, and extracts/forwards the Stdlib JSON packages file from the deepest
go_target in the graph (ie once all configurations/transitions are applied).Each JSON file can contain one or more Go packages definition. Which are then loaded, their paths expanded to real absolute paths. Then, the package files are read to get the real package name and its imports list. Imports are then resolved to other packages in the graph.
stdlib packages
Because stdlib packages are not part of the bazel graph, the packages driver will open the
.gofiles of each package and scan its imports to then resolve all packages, including of course stdlib.The whole stdlib definition sits inside one JSON file, which is generated by the
stdliblistbuilder. It executesgo list builtin std runtime/cgoand then transforms its output to match the JSON packages.This list file is fetched from the deepest
go_target in the graph so that all configurations and transitions are applied.