-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
@janos could you verify that this is not impacting the way we build our docker images? If it is, then we should address this as well prior to merging this, so that we don't introduce issues during the release scheduled for 17 July. |
It's missing some files. I had the same problem during the repo migration. As a workaround I had to copy these over manually to the vendor dir:
|
There are problems installing linters by gometalinter.v2. This package is deprecated (even so --install command is deprecated earlier), its repo set to read-only and recommends to use github.com/golangci/golangci-lint instead. This looks like a blocker for running |
@janos sure, let's try a more recent linter then! |
The main problem is vendoring cgo dependencies, directories that do not contain any go code, but C files golang/go#26366. We have github.com/karalabe/hid/hidapi and go-ethereum/crypto/secp256k1/libsecp256k1. This packages cannot be vendored with go modules without any additional tooling or scripting, which is very unfortunate. Even on calling Another solution that I like is this one golang/go#26366 (comment), but it requires patches to third party libraries. Adding a blank import to a packages with C code and an empty go file. Is this worth the trouble to use go modules? |
@janos I think it's worth to fix the third party libs with the blank import. I'm sure that Peter wouldn't mind merging these quickly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main README.md also needs an update:
https://github.com/ethersphere/swarm/tree/go-modules#go-environment
https://github.com/ethersphere/swarm/tree/go-modules#vendored-dependencies
Thanks @skylenet, will update README.md, I missed that. Vendoring directories that are not go packages can be done with "_" import, but if there are a lot of this directories it is very ugly, like in the situation with go-ethereum/crypto/secp256k1/libsecp256k1/libsecp256k1. This directory is a copy of the third party C library source tree and in order to keep it vendored, every directory needs to have a go file and each of those trivial go package needs to have a blank import. Adding go files makes maintaining of this C library source tree more difficult and I am not sure that it is a good idea. Another option is to use go modules, but not to vendor packages. I like to have all packages vendored to be able to build offline, but that creates a problem with vendoring cgo dependencies in subpackages. If packages are not vendored and go tool uses packages from its local modules cache (requires fetching them if missing locally), then the whole module source tree is available and there should be no problem with cgo. Would such option be acceptable @skylenet @nonsense? I do not see that go modules will ever vendor cgo dependencies as this is already recognized as bad practice from build perspective https://godoc.org/cmd/cgo golang/go#26366 (comment).
|
@janos I would really like to stick with having the vendored libs inside version control to avoid surprises whenever a third party lib deletes their repo or changes their git history. What do you think about adding a |
@skylenet actually, go modules proxies (such https://proxy.golang.org/) should minimize the risk of library disappearing. Even so, this proxy.golang.org will be de default GOPROXY value in Go 1.13. I do not like so much wrapping go tools if go tools can reverse the action from Makefile, like just calling go mod vendor, in which case an extra attention would be required when committing changes. I am still in favour not to vendor and to rely on public infrastructure and local caches, then to introduce maintenance complexity. |
I don't have an opinion on how we want to maintain our vendored libraries (go-vendor, go modules, or any other tool), but I think it is very important to have a local copy of all the source needed to build Swarm. If we are to maintain a local cache, I am not sure how this is decreasing complexity - it seems the opposite to me, but I am free to be convinced otherwise. I would be against not having a copy of our dependencies and relying on external parties to function, so that we can build Swarm. Offtopic: I had the same disagreement at a previous company, where it was out of my hands, and we opted for fetching dependencies from public infrastructure - the result was production going down 2 times in a span of 6 months because of missing dependencies 🔥 |
@nonsense I see your point about vendoring. Your experience with you previous company, is a very common thing to have if you have only one repository source, but I think that go proxies are a nice improvements on that. By local caches I mean local modules caches that we have on local machines where we do development, not any arbitrary cache that needs maintenance, from where additional redundancy is provided. Go modules proxies keep such caches for public use. Many companies have their own dependency caches (proxies) internally, like nexus, to avoid situations like you experienced. @nonsense If you insist on vendoring, what to you want to have govendor or go modules with custom wrapping, or something else? |
I think this change is nice, but doesn't really solve any critical problems that we have. If there is no option to keep all prerequisite source code for building Swarm within the Swarm repo, and this change comes at that cost, we have to make sure that the problems it addresses are worth it. |
@nonsense Thanks, I am sorry for being so annoying. How would you like to proceed in this change? That is what I wanted to ask. Should we drop go modules based on the problems? You explained nicely your opinion about vendoring, but I asked about your preferred tooling, should we stick with govendor, or wrap go modules? |
1 similar comment
As discussed in a call with @janos . We should proceed with having a If someone accidentally doesn't use the vendor target, we should still notice it on PR's, because you'll see that the C files will be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Well done! 🎉
Thanks @skylenet. |
Maybe @acud or @holisticode could have a look at this. I think this should be merged "soon"™ |
* 'master' of github.com:ethersphere/swarm: chunk, storage: chunk.Store multiple chunk Put (ethersphere#1681) storage: fix pyramid chunker and hasherstore possible deadlocks (ethersphere#1679) pss: Use distance to determine single guaranteed recipient (ethersphere#1672) storage: fix possible hasherstore deadlock on waitC channel (ethersphere#1674) network: Add adaptive capabilities message (ethersphere#1619) p2p/protocols, p2p/testing; conditional propagation of context (ethersphere#1648) api/http: remove unnecessary conversion (ethersphere#1673) storage: fix LazyChunkReader.join potential deadlock (ethersphere#1670) HTTP API support for pinning contents (ethersphere#1658) pot: Add Distance methods with tests (ethersphere#1621) README.md: Update Vendored Dependencies section (ethersphere#1667) network, p2p, vendor: move vendored p2p/testing under swarm (ethersphere#1647) build, vendor: use go modules for vendoring (ethersphere#1532)
This PR uses
go mod
command for vendoring instead govendor. Go mod is the official command to manage dependencies.If swarm codebase is within your $GOPATH/src, please set GO111MODULE=on environment variable when working with modules in Swarm. With Go 1.13 that would not be necessary https://tip.golang.org/doc/go1.13#modules.
Because of cgo dependencies
go mod vendor
is wrapped in Makefile rulevendor
to additionally copy c libraries from packages that forked them.vendor
directory is reconstructed by themake vendor
command, but there are some differences. Repositories that are complete modules (there are no submodules in the same repository) can be specified only as one module. With govendor, we have cases where packages that are in the same repository are vendored with different git commit hashes. This is probably a mistake and we have some different "subpackages" for the same "toplevel" package.Also some, of the packages were not specified in
vendor/vendor.json
file but copied to vendor directory. Docker is one example, which contained an older subpackage in vendor.json, but actually other docker packages require newer code.Inconsistencies:
github.com/syndtr/goleveldb
github.com/syndtr/goleveldb/leveldb c3a204f8e96543bb0cc090385c001078f184fc46
github.com/syndtr/goleveldb/leveldb/cache b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/comparer b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/errors b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/filter b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/iterator b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/journal b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/memdb b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/opt b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/storage b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/table b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
github.com/syndtr/goleveldb/leveldb/util b001fa50d6b27f3f0bb175a87d0cb55426d0a0ae
All subpackages use the same version, and toplevel different. Significant changes are in c3a204f8e96543bb0cc090385c001078f184fc46 so, the whole module references this hash.
github.com/opentracing/opentracing-go
github.com/opentracing/opentracing-go 25a84ff92183e2f8ac018ba1db54f8a07b3c0e04
github.com/opentracing/opentracing-go/ext bd9c3193394760d98b2fa6ebb2291f0cd1d06a7d
github.com/opentracing/opentracing-go/log bd9c3193394760d98b2fa6ebb2291f0cd1d06a7d
Selected version 25a84ff92183e2f8ac018ba1db54f8a07b3c0e04 as the latest of them.
github.com/elastic/gosigar
github.com/elastic/gosigar 37f05ff46ffa7a825d1b24cf2b62d4a4c1a9d2e8
github.com/elastic/gosigar/sys/windows a3814ce5008e612a0c6d027608b54e1d0d9a5613
Selected version 37f05ff46ffa7a825d1b24cf2b62d4a4c1a9d2e8 as the latest of them.
github.com/influxdata/influxdb
github.com/influxdata/influxdb/client a55dd0f50edd14c9c798d3564189eb4f53914309
github.com/influxdata/influxdb/models b36b9f109f2da91c8941679caf5356e08eee0b2b
github.com/influxdata/influxdb/pkg/escape 01288bdb0883a01cac999326bd34421b29acaec8
All packages have different hashes, selected 01288bdb0883a01cac999326bd34421b29acaec8 as the latest of them.
github.com/naoina/toml
github.com/naoina/toml 9fafd69674167c06933b1787ae235618431ce87f
github.com/naoina/toml/ast eb52202f758b98ac5b1a8eb26f36455205d688f0
Selected 9fafd69674167c06933b1787ae235618431ce87f as the latest of them.
golang.org/x/... packages
Go mod vendor/tidy commands always resolved to the same versions, even manually specified. So, I have left them as that.
Cleanup
After updating go.mod file
go mod tidy
should be called to cleanup it. It removes unneeded requirements based on dependency graph, so some of packages that are referenced by govendor are cleaned up from go.mod bygo mod tidy
.Versioning
I am suggesting that we use tagged versions in the future, especially for github.com/ethereum/go-ethereum.
Linting
Tool gopkg.in/alecthomas/gometalinter.v2 is deprecated and does not play well with modules. I have replaced it with github.com/golangci/golangci-lint, as suggested. It has updated code for linting and our codebase fails with it. Some of the checks are disabled until they are fixed, maybe in an additional PR, or this one.
fixes: #1433