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

x/tools/go/packages: expose Deps field of a package #38953

Closed
aykevl opened this issue May 8, 2020 · 12 comments
Closed

x/tools/go/packages: expose Deps field of a package #38953

aykevl opened this issue May 8, 2020 · 12 comments

Comments

@aykevl
Copy link

@aykevl aykevl commented May 8, 2020

I have been working on Go module support in TinyGo, here: tinygo-org/tinygo#941

For getting the list of packages, I'm currently using the x/tools/go/packages package. Unfortunately it doesn't provide a way to get the full list of (recursive) dependencies of a package, in particular the runtime package (which appears to be implicitly added by the Go toolchain). Therefore I need to run the packages query twice: once to get the dependencies of the main package, and once to get the dependencies of the runtime package.

What version of Go are you using (go version)?

$ go version
go version go1.14.1 windows/amd64

(but this is not related to any particular Go version)

Does this issue reproduce with the latest release?

I'm right now using the tools module at golang/tools@b320d3a but I can't find a Deps field on https://godoc.org/golang.org/x/tools/go/packages#Package either.

What did you do?

Right now I'm using Package.Imports and obtaining a list of packages by going through all imports recursively. Unfortunately this doesn't include the runtime package, unless it is explicitly imported.

What did you expect to see?

I was hoping for a field like this:

    Deps map[string]*Package

And perhaps a way to specify that I want this field to be filled (unfortunately NeedDeps is already in use, perhaps use NeedRecursiveDeps instead?).

What did you see instead?

No way to get the packages in the Deps field returned by go list -json.

@gopherbot gopherbot added this to the Unreleased milestone May 8, 2020
@gopherbot gopherbot added the Tools label May 8, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 8, 2020

/cc @matloob

@heschi
Copy link
Contributor

@heschi heschi commented May 8, 2020

What do you mean when you say that packages implicitly depend on the runtime package? Naturally everything implicitly depends on the runtime for memory allocation and such, but those functions don't come in by a normal package dependency. Nothing should depend on the runtime package unless they explicitly import it, afaik.

@aykevl
Copy link
Author

@aykevl aykevl commented May 8, 2020

Naturally everything implicitly depends on the runtime for memory allocation and such, but those functions don't come in by a normal package dependency.

I'm building a compiler, not a regular Go tool. Therefore I need to know all dependencies, including the runtime (which is an implicit dependency of all packages). I use it to get the list of files I need to compile to produce an executable.

For example, if have the following Go file:

package main

func main() {
	println("hello world!")
}

I get the following JSON for go list -json -deps hello.go (irrelevant fields removed):

{
        "Name": "main",
        "GoFiles": [
                "hello.go"
        ],
        "Deps": [
                "internal/bytealg",
                "internal/cpu",
                "runtime",
                "runtime/internal/atomic",
                "runtime/internal/math",
                "runtime/internal/sys",
                "unsafe"
        ]
}

Note that while this package has no explicit imports, it still depends on the runtime (and all packages the runtime imports).

All these other packages are also given as output of the go list command, but there is no way to get them using the x/tools/go/packages package. That is what this feature request is about: exposing all packages (as *packages.Package object), not just those that are imported as an explicit dependency.

@heschi
Copy link
Contributor

@heschi heschi commented May 8, 2020

I see. That seems like more of a build system/implementation detail than information about the package, so I don't know if it's a fit for go/packages. But that's more of a question for @matloob.

@matloob
Copy link
Contributor

@matloob matloob commented May 11, 2020

I think this might be too low-level for go/packages, but on the other hand, I think this solidly falls within the use cases of go/packages.

Can your tool always add package runtime as an argument to go/packages and then determine the set of Deps by collecting the transitive set of Imports and adding on runtime's Imports? If you can or there's a similar path to getting the information you need, we should go that route.

@aykevl
Copy link
Author

@aykevl aykevl commented May 11, 2020

I tried adding both import paths (the package to build and the runtime package), and while that works in many cases it fails for getting dependencies for single Go files.

This works:

packages.Load(cfg, "runtime", "some/package/path")

This doesn't:

packages.Load(cfg, "runtime", "some/file.go")

So it looks like there are two workarounds: either do two queries (what I'm doing now), or call go list directly (which it seems I'm not supposed to do).

@matloob
Copy link
Contributor

@matloob matloob commented May 15, 2020

I see. Is doing two queries a burden (performance- or other-wise) for your tool? That's the right way to do it. Unfortunately because of how go list works you can't mix sources ad-hoc packages with normal packages, and the go/packages query interface is the same as go lists, with some additions.

@aykevl
Copy link
Author

@aykevl aykevl commented May 15, 2020

Is doing two queries a burden (performance- or other-wise) for your tool?

I haven't measured it but it's probably not a problem. Consistency might be a problem however (when edits are ongoing during the query). Also, it feels a bit unclean to me to run the query twice for what - in my view - should be one query.

That's the right way to do it.

That seems like a strange statement to me. The go list command contains all the information needed, but the packages package doesn't expose it. Is leaving out the Deps field intentional?

@matloob
Copy link
Contributor

@matloob matloob commented May 18, 2020

Adding @dominikh @ianthehat @mvdan for their opinions.

go/packages isn't meant to expose all the information that go list provides. There are a number of things that it leaves out. While it's meant to be used for all kinds of go tooling, its primary focus is tools that operate on syntax and type information, so it presents the information needed to interact with go/ast and go/types. I don't know that we made an intentional decision that the Deps field definitely doesn't belong in go/packages, but it wasn't necessary for the sorts of tools go/packages was created to support so we never felt the need to put it in.

Since the data in the Deps field isn't unique, but can be determined by piecing together Imports, my intuition is that we should keep things the way they are. (Though it's unfortunate that "runtime" can't be added to the query because of ad-hoc packages). On the other hand if there are other go/packages users that want the information in Deps, I think it would be reasonable to add it.

By the way, there's a discussion here: #38445 about adding other fields to the Packages struct. It might be worth examining how they fit in.

@mvdan
Copy link
Member

@mvdan mvdan commented May 19, 2020

Given that runtime is the only special case here, and that we can agree that go list runtime should be very fast, I also lean towards not adding Deps.

@aykevl do you intend to support build systems other than go list, such as bazel? Because if not, you could just call go list -json and get all of its information directly. See #38234 for some starting points.

@aykevl
Copy link
Author

@aykevl aykevl commented May 19, 2020

Okay, understood. I agree that my 'tool' (actually a Go compiler) isn't exactly what the packages package was intended to support. The Deps field may therefore be a rather unusual requirement.

@aykevl do you intend to support build systems other than go list, such as bazel? Because if not, you could just call go list -json and get all of its information directly. See #38234 for some starting points.

Not right now, but directly calling go list -json may indeed be the better option due to my somewhat unusual needs. Right now I'm using x/go/packages and it works so I don't feel like changing it, but will keep it in mind when requirements change.

@matloob
Copy link
Contributor

@matloob matloob commented Jun 5, 2020

Okay. I'll close this issue if that's okay.

@dominikh dominikh closed this May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants