Skip to content

wire: use go/packages for analysis #623

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

Merged
merged 10 commits into from
Nov 7, 2018
Merged

Conversation

zombiezen
Copy link
Contributor

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

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 zombiezen requested a review from vangent November 6, 2018 18:32
@googlebot googlebot added the cla: yes Google CLA has been signed! label Nov 6, 2018
@vangent
Copy link
Contributor

vangent commented Nov 6, 2018

Tests are failing....

@zombiezen
Copy link
Contributor Author

Investigating. This is a new failure to me.

@vangent
Copy link
Contributor

vangent commented Nov 6, 2018

The changes look fine to me, once tests are passing :-).

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

Can you clarify -- what did it do before?

@vangent vangent removed their assignment Nov 6, 2018
@vangent vangent assigned zombiezen and unassigned vangent Nov 6, 2018
@zombiezen
Copy link
Contributor Author

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.

@jba
Copy link
Contributor

jba commented Nov 7, 2018

vendoring becomes obsolete with modules

Aren't top-level vendor directories still supported?

@zombiezen
Copy link
Contributor Author

@jba They are, but the semantics as they pertain to Wire have changed: such a vendored directory used to silently be myproject/vendor/example.com/foo, whereas now it just appears as example.com/foo in the graph. (Maintaining that test was also going to require plumbing the -mod=vendor build flag through the stack, which I was not inclined to do.)

Note to other contributors: please don't merge this yet. I don't have an explanation for the original build failure yet. In the meantime, I have pushed seemingly identical code and the tests passed. :|

@zombiezen zombiezen assigned vangent and unassigned zombiezen Nov 7, 2018
@zombiezen
Copy link
Contributor Author

OK, tests pass AFAICT. The Mac failure is legitimate and has been addressed by calling filepath.EvalSymlinks on the returned directory name. However, I remain unable to reproduce the original Linux failure of:

go [list -e -json -compiled -test=false -export=false -deps=true -tags=wireinject -- example.com/foo]: exit status 1: build example.com/foo: cannot find module for path github.com/google/go-cloud/wire

I am apprehensive that I might be somehow introducing a nondeterministic test. However, I have no plausible hypotheses as to the source of the failure, so I don't know what to log in order to monitor the failure in the future. Possibilities that I've ruled out:

  • Not related to GO111MODULE setting: I have tried both on and off on my local machine.
  • Not related to Travis userspace: I downloaded the Travis Docker image and could not reproduce, and also can't get the failure to happen reliably on Travis.
  • Not related to go.mod quoting: passes either way on Travis
  • Not related to TMPDIR setting: Travis does not set TMPDIR, only TRAVIS_TMPDIR, and Go will use /tmp in this case.
  • GOPATH is being materialized correctly: I checked the file names being created and they align with my expectations.

@vangent, please review, but please do not merge. I would like to do another pass at editing the commit message before it goes in.

@zombiezen zombiezen merged commit 95bbfbd into google:master Nov 7, 2018
@zombiezen zombiezen deleted the wire-modules branch November 7, 2018 22:44
zombiezen added a commit to google/wire that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants