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

accounts/abi/bind: stop using goimports in the binding generator #17768

Merged
merged 5 commits into from
Oct 5, 2018
Merged

accounts/abi/bind: stop using goimports in the binding generator #17768

merged 5 commits into from
Oct 5, 2018

Conversation

jeremyschlatter
Copy link
Contributor

In projects with large GOPATHs, goimports can take a long time to run.

goimports has also not been updated for Go 1.11 modules:

golang/go#27287
golang/go#27865
golang/go#24661

Instead of using goimports to insert import statements, this commit generates imports directly in the Go template.

In projects with large GOPATHs, goimports can take a long time to run.

goimports has also not been updated for Go 1.11 modules:

golang/go#27287
golang/go#27865
golang/go#24661

Instead of using goimports to insert import statements, this commit
generates imports directly in the Go template.
@jeremyschlatter
Copy link
Contributor Author

I’m very open to feedback here. Let me know what you think.

I created this PR after running into two different pain points with bind.Bind that both stemmed from goimports:

  1. I used to have a very large GOPATH, and bind.Bind took on the order of seconds, because goimports was scanning the whole GOPATH. I worked around that by creating a new GOPATH for my project and bind.Bind now takes on the order of milliseconds.
  2. I recently tried to convert my project to use modules, but bind.Bind no longer works if I do this, because goimports can’t find imports in modules yet. This is currently blocking me from switching my project to modules — I haven’t yet found a workaround that I’m happy with.

Note that I already have a reasonable workaround for (1), and (2) will eventually be patched upstream, so I don’t think this is critical to solve. But it seemed like a bit of a smell to me that this bit of code caused pain points twice in my project, so I decided to propose this change.

Notable downsides to this approach include increased maintenance cost (needing to manage these hard-coded imports), and occasionally generating code that imports packages it does not need. The latter could be solved by adding logic to only insert imports when we know they are needed, though I suspect that would not be worth the increased complexity.

I got the idea for doing imports this way from the Go protobuf implementation, which does a very similar thing: generator and example output.

The code now also doesn’t go through gofmt, but I could add that back in.

@fjl
Copy link
Contributor

fjl commented Sep 28, 2018

I'm very happy about this change. Please add gofmt and delete goimports from vendor/ to make this PR perfect :).

goimports used to do this formatting. To keep the generated code
in gofmt style, we now need to format it here ourselves.
bind.Bind now works in symlinked environments.
@jeremyschlatter
Copy link
Contributor Author

Done. goimports was still used in two places in bind_test.go. I removed them here and here.

@jeremyschlatter
Copy link
Contributor Author

Thanks for the review @fjl

Anything I can do to help get this merged? I'm itching to try out Go 1.11 modules in my project :)

@fjl fjl changed the title accounts/abi/bind: drop dependency on goimports for Go bindings accounts/abi/bind: stop using goimports in the binding generator Oct 5, 2018
@fjl fjl merged commit 5ed3960 into ethereum:master Oct 5, 2018
@karalabe karalabe added this to the 1.8.17 milestone Oct 8, 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

Successfully merging this pull request may close these issues.

3 participants