tmthrgd / go-bindata

A small utility which generates Go code from any file. Useful for embedding binary data in a Go program. [Obsolete].
https://golang.org/issue/35950
Other
72 stars 4 forks source link

Revendor all deps with the dep tool #3

Closed ryboe closed 7 years ago

ryboe commented 7 years ago

Thanks so much for taking over go-bindata! 🎉 🎉 🎉 I have several PRs I want to submit. The first is to vendor all the missing deps using the dep tool. This tool is only in alpha, but it's the best Go vendoring tool there is. It will be merged into the go tool sometime around 1.10. Future dep updates can be done like this.

dep ensure -v -update

Edit manifest.json if you want to specify version constraints.

In any case, because of the large code change (the gddo package was last updated in March 2015), this should be tested thoroughly.

tmthrgd commented 7 years ago

You're most welcome, and thank you for your interest. Feel free to submit away!

With 7adbaa1 the only non-standard library dependency used is github.com/golang/gddo/httputil/header in httpasset. That was vendored with govendor in 397fcce157e9e8612255cee51f10db95a25848b3.

There are some non-standard library dependencies used in the test suite, but I definitely do not want to vendor those as that's just pure bloat.

As for govendor vs dep, I'm going to remain on the fence with this for now. (I can't say I'm a huge fan of vendoring in general — it feels like the wrong solution to the wrong problem 💁‍♂️). Ultimately, I'm not prepared to switch over to dep until golang/dep#276 is resolved. Once the manifest format is stable then I'm happy to revisit this.

If you would like to add a new commit that excludes the test suite dependencies and is updated for 7adbaa1, I'll happily leave this open and revisit during the go1.9 cycle which is when the manifest format should be stable.

Edit: also see this comment:

I would encourage no one to move to dep until we at least close out #276 :smile: manifest and lock will be changing structure, names, formats, and we won't provide an upgrade path.

We're pushing as hard as we can on getting that done, but for now, experimentation and feedback is what we really need to get there.

ryboe commented 7 years ago

Fair points. I would add these points, though. 1) I don't think the unstable manifest and lock files are a big deal. It's pretty easy to delete them both, delete vendor/, and rerun dep init if the deps are up-to-date. Running dep init again would produce new manifest.toml and lock.toml files with minimal code churn. 2) I do think it's a good idea to vendor the test deps. IMHO contributors should be able the clone the repo and start running tests immediately. It's kind of annoying to have to figure out which deps are required (let alone the versions) and fetch them. The alternative is to push to a branch and wait forever for Travis to run it, before finding out you have a syntax error. 3) Static analysis tools like staticcheck can't be run because they can't build without the missing deps. The followup PRs are fixes for bugs caught by static analysis tools. Instead I'll leave them as open issues and let you decide what you want to do.

tmthrgd commented 7 years ago

With respect to point two, just run: go get -u -t github.com/tmthrgd/go-bindata. I assume you weren't aware of the -t flag. From the docs:

The -t flag instructs get to also download the packages required to build the tests for the specified packages.

I'm not really too worried about the test dependencies becoming out of date. Most people will never run the test suite, that's more for me and any contributors. I think it's easy enough for me to update the test suite in the event of any breaking changes.

Just doing a quick once over of the imports of the test suite:

With respect to the first point, I'm really not interested in being stuck managing any churn around a tool that is known to be unstable. I don't see any issue sticking with govendor for now, it doesn't effect package users in any meaningful way at all. Like I said, when the manifest format becomes stable I will certainly revisit this.

Again, thank you for your interest in this project and my efforts. 🙂

ryboe commented 7 years ago

All fair points. I didn't know about the -t flag. That's a great tip. Thanks! 😄