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

wire: support loading type-checking with Go modules #10

Closed
zombiezen opened this issue Jun 7, 2018 · 3 comments
Closed

wire: support loading type-checking with Go modules #10

zombiezen opened this issue Jun 7, 2018 · 3 comments
Assignees

Comments

@zombiezen
Copy link
Collaborator

Wire uses golang.org/x/tools/go/loader to get type information, but this does not use Go modules. The current workaround is to run vgo mod -vendor, but this occasionally requires adding more imports temporarily to pin all the dependencies (especially when using awscloud.AWS or gcpcloud.GCP). There isn't a good way to solve this until Go module tooling hits mainstream, but I'm keeping this issue here to track.

@zombiezen zombiezen changed the title wire: support loading type-checking from vgo wire: support loading type-checking with Go modules Jul 19, 2018
@zombiezen
Copy link
Collaborator Author

The proper approach seems to be to use golang.org/x/tools/go/packages, which will be in Go 1.11's standard library. While it's not stable yet, I'll give it a shot to see if there are any issues in integrating with it.

zombiezen referenced this issue in google/go-cloud Jul 19, 2018
We should be instructing users to run stable things, and more
adventurous folks can adapt to use Go modules. I just ran the test suite
locally from a `go get` and it compiled and ran fine.

Updates #78
Updates #208
@zombiezen
Copy link
Collaborator Author

This is blocked on golang/go#26509.

@zombiezen
Copy link
Collaborator Author

No longer blocked on the go/packages issue, but now blocked on golang.org/cl/127258 which allows modifying the list of files before it hits the type checker.

@zombiezen zombiezen self-assigned this Oct 11, 2018
zombiezen referenced this issue in zombiezen/go-cloud Nov 5, 2018
The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
neccessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates #78
Updates google#323
zombiezen referenced this issue in google/go-cloud Nov 6, 2018
The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
necessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates #78
Updates #323
zombiezen referenced this issue in zombiezen/go-cloud Nov 6, 2018
Unfortunately, this does come with a ~4x slowdown to Wire, as it is
now pulling source for all transitively depended packages, but not
trimming comments or function bodies. This is due to limitations with
the ParseFile callback in go/packages.

This comes with a single semantic change: when performing analysis, Wire
will now evaluate everything with the wireinject build tag. I updated
the build tags tests accordingly.

I deleted the vendoring test, as vendoring becomes obsolete with
modules.

go/packages now parses comments by default, so now the generated code
includes comments for non-injector declarations.

Fixes #78
zombiezen referenced this issue in google/go-cloud Nov 7, 2018
Unfortunately, this does come with a ~4x slowdown to Wire, as it is
now pulling source for all transitively depended packages, but not
trimming comments or function bodies. This is due to limitations with
the ParseFile callback in go/packages.

This comes with a single semantic change: when performing analysis, Wire
will now evaluate everything with the wireinject build tag. I updated
the build tags tests accordingly. Prior to this PR, only the packages
directly named by the package patterns would be evaluated with the
wireinject build tag. Dependencies would not have the wireinject build
tag applied. There isn't a way to selectively apply build tags in go/packages,
and there isn't a clear benefit to applying it selectively. Being consistent with
other Go tooling provides greater benefit.

I deleted the vendoring test, as non-top-level vendoring
becomes obsolete with modules.

go/packages now parses comments by default, so now the generated code
includes comments for non-injector declarations.

Fixes #78
zombiezen referenced this issue Nov 28, 2018
We should be instructing users to run stable things, and more
adventurous folks can adapt to use Go modules. I just ran the test suite
locally from a `go get` and it compiled and ran fine.

Updates google/go-cloud#78
Updates google/go-cloud#208
zombiezen referenced this issue Nov 28, 2018
…loud#616)

The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
necessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates google/go-cloud#78
Updates google/go-cloud#323
zombiezen referenced this issue Nov 28, 2018
Unfortunately, this does come with a ~4x slowdown to Wire, as it is
now pulling source for all transitively depended packages, but not
trimming comments or function bodies. This is due to limitations with
the ParseFile callback in go/packages.

This comes with a single semantic change: when performing analysis, Wire
will now evaluate everything with the wireinject build tag. I updated
the build tags tests accordingly. Prior to this PR, only the packages
directly named by the package patterns would be evaluated with the
wireinject build tag. Dependencies would not have the wireinject build
tag applied. There isn't a way to selectively apply build tags in go/packages,
and there isn't a clear benefit to applying it selectively. Being consistent with
other Go tooling provides greater benefit.

I deleted the vendoring test, as non-top-level vendoring
becomes obsolete with modules.

go/packages now parses comments by default, so now the generated code
includes comments for non-injector declarations.

Fixes google/go-cloud#78
@zombiezen zombiezen transferred this issue from google/go-cloud Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant