Split sentry-go into multiple submodules #156
Comments
|
@rur wrote in #75 (comment):
|
|
@leighmcculloch wrote in #75 (comment):
|
|
@leighmcculloch wrote in #75 (comment):
|
|
@leighmcculloch wrote in #75 (comment):
|
|
@rhcarvalho wrote in #79 (comment):
|
|
@rhcarvalho wrote in #79 (comment):
|
This idea sounds simple but has known drawbacks observed in the Go community at large. I've run into a few situations in the last months in which projects would not build after some dependency decided to split up into multiple modules per repository. The Go Wiki outlines the caveats of maintaining multi-module repositories: https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories. If we do split, then we have to be very careful with the initial splitting to avoid breaking existing users. We would also have to keep maintaining several Releasing Special care would have to be given whenever we add a new module, if that would be the status quo by then, one module per package, one module per integration. |
Short answerEngineering teams should review and audit packages that are dependencies of their binaries, package That is a different list than what Go Modules track. Long answerWe need to distinguish between Package Dependencies and Module Dependencies.
Packages
$ go list -f '{{join .Imports "\n"}}' github.com/getsentry/sentry-go | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}'An example program that uses $ go list -f '{{join .Imports "\n"}}' github.com/getsentry/sentry-go/example/basic | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}'
github.com/getsentry/sentry-goModulesGo Modules do know about the direct and indirect dependencies of all packages in a module, those being stored in the The When building a program, package Let's say you have a program, package package main
import (
"fmt"
"net/http"
"github.com/getsentry/sentry-go"
sentrygin "github.com/getsentry/sentry-go/gin"
"github.com/gin-gonic/gin"
)
func main() {
// ...
}Here is the module example.com/gin-server
go 1.13
require (
github.com/getsentry/sentry-go v0.4.0
github.com/gin-gonic/gin v1.5.0
github.com/go-playground/universal-translator v0.17.0 // indirect
github.com/golang/protobuf v1.3.3 // indirect
github.com/json-iterator/go v1.1.9 // indirect
github.com/leodido/go-urn v1.2.0 // indirect
github.com/mattn/go-isatty v0.0.12 // indirect
golang.org/x/sys v0.0.0-20200124204421-9fbb57f87de9 // indirect
gopkg.in/go-playground/validator.v9 v9.31.0 // indirect
gopkg.in/yaml.v2 v2.2.8 // indirect
) At the time of writing this, the latest release of https://github.com/getsentry/sentry-go/blob/v0.4.0/go.mod#L9 What is the version of Where is the exact code that needs review? $ go list -deps | sort | xargs go list -f '{{.Dir}}'
$GOROOT/src/bufio
$GOROOT/src/bytes
$GOROOT/src/compress/flate
$GOROOT/src/compress/gzip
$GOROOT/src/container/list
$GOROOT/src/context
$GOROOT/src/crypto
$GOROOT/src/crypto/aes
$GOROOT/src/crypto/cipher
$GOROOT/src/crypto/des
$GOROOT/src/crypto/dsa
$GOROOT/src/crypto/ecdsa
$GOROOT/src/crypto/ed25519
$GOROOT/src/crypto/ed25519/internal/edwards25519
$GOROOT/src/crypto/elliptic
$GOROOT/src/crypto/hmac
$GOROOT/src/crypto/internal/randutil
$GOROOT/src/crypto/internal/subtle
$GOROOT/src/crypto/md5
$GOROOT/src/crypto/rand
$GOROOT/src/crypto/rc4
$GOROOT/src/crypto/rsa
$GOROOT/src/crypto/sha1
$GOROOT/src/crypto/sha256
$GOROOT/src/crypto/sha512
$GOROOT/src/crypto/subtle
$GOROOT/src/crypto/tls
$GOROOT/src/crypto/x509
$GOROOT/src/crypto/x509/pkix
$GOROOT/src/encoding
$GOROOT/src/encoding/asn1
$GOROOT/src/encoding/base64
$GOROOT/src/encoding/binary
$GOROOT/src/encoding/gob
$GOROOT/src/encoding/hex
$GOROOT/src/encoding/json
$GOROOT/src/encoding/pem
$GOROOT/src/encoding/xml
$GOROOT/src/errors
/home/rodolfo/example/gin-server
$GOROOT/src/fmt
$GOPATH/pkg/mod/github.com/getsentry/sentry-go@v0.4.0
$GOPATH/pkg/mod/github.com/getsentry/sentry-go@v0.4.0/gin
$GOPATH/pkg/mod/github.com/gin-contrib/sse@v0.1.0
$GOPATH/pkg/mod/github.com/gin-gonic/gin@v1.5.0
$GOPATH/pkg/mod/github.com/gin-gonic/gin@v1.5.0/binding
$GOPATH/pkg/mod/github.com/gin-gonic/gin@v1.5.0/internal/json
$GOPATH/pkg/mod/github.com/gin-gonic/gin@v1.5.0/render
$GOPATH/pkg/mod/github.com/golang/protobuf@v1.3.3/proto
$GOPATH/pkg/mod/github.com/go-playground/locales@v0.13.0
$GOPATH/pkg/mod/github.com/go-playground/locales@v0.13.0/currency
$GOPATH/pkg/mod/github.com/go-playground/universal-translator@v0.17.0
$GOPATH/pkg/mod/github.com/leodido/go-urn@v1.2.0
$GOPATH/pkg/mod/github.com/mattn/go-isatty@v0.0.12
$GOPATH/pkg/mod/github.com/ugorji/go/codec@v1.1.7
$GOROOT/src/go/ast
$GOROOT/src/go/build
$GOROOT/src/go/doc
$GOPATH/pkg/mod/golang.org/x/sys@v0.0.0-20200124204421-9fbb57f87de9/unix
$GOROOT/src/go/parser
$GOPATH/pkg/mod/gopkg.in/go-playground/validator.v9@v9.31.0
$GOPATH/pkg/mod/gopkg.in/yaml.v2@v2.2.8
$GOROOT/src/go/scanner
$GOROOT/src/go/token
$GOROOT/src/hash
$GOROOT/src/hash/crc32
$GOROOT/src/html
$GOROOT/src/html/template
$GOROOT/src/internal/bytealg
$GOROOT/src/internal/cpu
$GOROOT/src/internal/fmtsort
$GOROOT/src/internal/goroot
$GOROOT/src/internal/goversion
$GOROOT/src/internal/lazyregexp
$GOROOT/src/internal/nettrace
$GOROOT/src/internal/oserror
$GOROOT/src/internal/poll
$GOROOT/src/internal/race
$GOROOT/src/internal/reflectlite
$GOROOT/src/internal/singleflight
$GOROOT/src/internal/syscall/unix
$GOROOT/src/internal/testlog
$GOROOT/src/io
$GOROOT/src/io/ioutil
$GOROOT/src/log
$GOROOT/src/math
$GOROOT/src/math/big
$GOROOT/src/math/bits
$GOROOT/src/math/rand
$GOROOT/src/mime
$GOROOT/src/mime/multipart
$GOROOT/src/mime/quotedprintable
$GOROOT/src/net
$GOROOT/src/net/http
$GOROOT/src/net/http/httptrace
$GOROOT/src/net/http/httputil
$GOROOT/src/net/http/internal
$GOROOT/src/net/rpc
$GOROOT/src/net/textproto
$GOROOT/src/net/url
$GOROOT/src/os
$GOROOT/src/os/exec
$GOROOT/src/path
$GOROOT/src/path/filepath
$GOROOT/src/reflect
$GOROOT/src/regexp
$GOROOT/src/regexp/syntax
$GOROOT/src/runtime
$GOROOT/src/runtime/cgo
$GOROOT/src/runtime/internal/atomic
$GOROOT/src/runtime/internal/math
$GOROOT/src/runtime/internal/sys
$GOROOT/src/sort
$GOROOT/src/strconv
$GOROOT/src/strings
$GOROOT/src/sync
$GOROOT/src/sync/atomic
$GOROOT/src/syscall
$GOROOT/src/text/template
$GOROOT/src/text/template/parse
$GOROOT/src/time
$GOROOT/src/unicode
$GOROOT/src/unicode/utf16
$GOROOT/src/unicode/utf8
$GOROOT/src/unsafe
$GOROOT/src/vendor/golang.org/x/crypto/chacha20poly1305
$GOROOT/src/vendor/golang.org/x/crypto/cryptobyte
$GOROOT/src/vendor/golang.org/x/crypto/cryptobyte/asn1
$GOROOT/src/vendor/golang.org/x/crypto/curve25519
$GOROOT/src/vendor/golang.org/x/crypto/hkdf
$GOROOT/src/vendor/golang.org/x/crypto/internal/chacha20
$GOROOT/src/vendor/golang.org/x/crypto/internal/subtle
$GOROOT/src/vendor/golang.org/x/crypto/poly1305
$GOROOT/src/vendor/golang.org/x/net/dns/dnsmessage
$GOROOT/src/vendor/golang.org/x/net/http2/hpack
$GOROOT/src/vendor/golang.org/x/net/http/httpguts
$GOROOT/src/vendor/golang.org/x/net/http/httpproxy
$GOROOT/src/vendor/golang.org/x/net/idna
$GOROOT/src/vendor/golang.org/x/sys/cpu
$GOROOT/src/vendor/golang.org/x/text/secure/bidirule
$GOROOT/src/vendor/golang.org/x/text/transform
$GOROOT/src/vendor/golang.org/x/text/unicode/bidi
$GOROOT/src/vendor/golang.org/x/text/unicode/norm(output edited to use $GOROOT and $GOPATH instead of full paths in my system) Please note that the code that needs auditing/review is independent of whatever packages appear in the |
This is a great point. I think the go build process only includes into the final build the packages actually imported in the dependency tree and not all packages in a module. However, this doesn't capture the full picture of how this impacts importers of your module. Importing a module still brings that modules complete dependency graph into the dependency resolution process. Anyone using dependencies imported will be limited by the minimum versions defined in sentry-go or any transitive dependency through sentry-go.
This is not always the case. Go.mod files only contain direct dependencies, and indirect dependencies that are not captured in a transitive dependencies go.mod file. If a transitive dependency has a go.mod file, its dependencies will not be captured in the applications go.mod file. If a transitive dependency adds a go.mod file the A large dependency graph still causes importers pain. Multiple modules would reduce that pain in a backwards compatible way. However, I agree, there is care required and a maintenance cost for you folks. |
This is true and unfortunate. Specifically the limitation pushes people to use newer versions. As far as I know, we haven't heard anyone complain about that, though. I imagine it would be worse if we forced people to use old versions. Example for future reference: $ go get -u github.com/smartystreets/goconvey@1.5.0 github.com/getsentry/sentry-go@v0.4.0
go: finding github.com/smartystreets/goconvey 1.5.0
go: finding github.com/gopherjs/gopherjs latest
go: finding golang.org/x/tools latest
go get: inconsistent versions:
github.com/getsentry/sentry-go@v0.4.0 requires github.com/smartystreets/goconvey@v1.6.4 (not github.com/smartystreets/goconvey@v1.5.0 from github.com/smartystreets/goconvey@1.5.0)The example above shows that a downstream module cannot depend on both
We're on the same page. I'll keep this open for investigation. Is a nice to have for v1.0.0. In fact, this issue boils down to how to deal with integrations and examples, those are the ones bringing in extraneous dependencies. Notes
|
|
Adding a point of reference here... I run into golang/go#29935, which describes a similar problem that surfaced in Some highlights:
This is how the dependency tree in If one follows issues and goes down the rabbit hole, one finds several reports of broken state and fire. I think removing the dependencies of integrations from Unfortunately testing the changes precisely is hard, as the whole process relies on public tags on GitHub among other steps that are hard to reproduce, and even if we did do with a fork or sample repo, there is no guarantee all would go right for |
|
And one more similar issue golang/go#30685, Highlights:
|
|
Initially we may follow the same strategy as described in golang/go#28136 (comment). The downside being that importers of an integration (or any "submodule" for that matter) would see lines like It remains an open discussion how to version the submodules:
|
|
I was wondering what situation we'd be in in a world where https://golang.github.io/dep/docs/the-solver.html#activated-constraints
If I understand the above correctly, if |
|
With the recent release of Go 1.14 and plans for 1.15, I learned about golang/go#36460, If my understanding of the proposal is right, a future version of Go Modules could reduce the need to break a repository into multiple modules. The discussion in golang/go#37175 supports that. Some quotes:
|
|
One more issue discussing splitting a repository (golang.org/x/tools) into multiple modules: golang/go#27858 (comments express resistance to having multiple go.mod files) |
|
The Google Cloud SDK is an example of a repo that has split into many modules. I couldn't find any discussion around it though. |
|
|
@Gunni do you have some specific concern I can help you with? Adding to An upcoming change to Go modules, which might land in Go 1.16, will likely remove the need to split the repo into multiple modules, see golang/go#36460. |
|
Unfortunately I can't offer a solution to this problem, but I wanted to drop a note about a workaround. My main issue with the large dependency list is not concern over the size of the binary, but that in my organization I have to get the open source licenses reviewed and approved for all transitive dependencies. GPL/LGPL are problematic. Starting from
Those are the only 3 LGPLv3 licensed libraries in the entire graph. The rest are all generally permissive licenses (MIT, BSD, Apache, etc). As a workaround, I added this to my This forces Go to use a newer version instead, |
|
@peplin thanks for bringing this up. Splitting the SDK into multiple modules is something in our radar but the change is more complex. Note that the only packages that go into your build are those actually imported by your Go packages or their transitive package (not module) dependencies. You can list all recursively imported dependencies of a package with As an example, here are the dependencies of a basic example (from this repo) using sentry-go. Note that there are no dependencies outside of the Go standard library: $ cd example/basic
$ go list -f '{{join .Deps "\n"}}'
bufio
bytes
compress/flate
compress/gzip
container/list
context
crypto
crypto/aes
crypto/cipher
crypto/des
crypto/dsa
crypto/ecdsa
crypto/ed25519
crypto/ed25519/internal/edwards25519
crypto/elliptic
crypto/hmac
crypto/internal/randutil
crypto/internal/subtle
crypto/md5
crypto/rand
crypto/rc4
crypto/rsa
crypto/sha1
crypto/sha256
crypto/sha512
crypto/subtle
crypto/tls
crypto/x509
crypto/x509/internal/macos
crypto/x509/pkix
encoding
encoding/asn1
encoding/base64
encoding/binary
encoding/hex
encoding/json
encoding/pem
errors
fmt
github.com/getsentry/sentry-go
go/ast
go/build
go/doc
go/parser
go/scanner
go/token
hash
hash/crc32
internal/bytealg
internal/cpu
internal/fmtsort
internal/goroot
internal/goversion
internal/lazyregexp
internal/nettrace
internal/oserror
internal/poll
internal/race
internal/reflectlite
internal/singleflight
internal/syscall/execenv
internal/syscall/unix
internal/testlog
internal/unsafeheader
io
io/ioutil
log
math
math/big
math/bits
math/rand
mime
mime/multipart
mime/quotedprintable
net
net/http
net/http/httptrace
net/http/internal
net/textproto
net/url
os
os/exec
path
path/filepath
reflect
regexp
regexp/syntax
runtime
runtime/cgo
runtime/debug
runtime/internal/atomic
runtime/internal/math
runtime/internal/sys
sort
strconv
strings
sync
sync/atomic
syscall
text/template
text/template/parse
time
unicode
unicode/utf16
unicode/utf8
unsafe
vendor/golang.org/x/crypto/chacha20
vendor/golang.org/x/crypto/chacha20poly1305
vendor/golang.org/x/crypto/cryptobyte
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/curve25519
vendor/golang.org/x/crypto/hkdf
vendor/golang.org/x/crypto/internal/subtle
vendor/golang.org/x/crypto/poly1305
vendor/golang.org/x/net/dns/dnsmessage
vendor/golang.org/x/net/http/httpguts
vendor/golang.org/x/net/http/httpproxy
vendor/golang.org/x/net/http2/hpack
vendor/golang.org/x/net/idna
vendor/golang.org/x/net/route
vendor/golang.org/x/sys/cpu
vendor/golang.org/x/text/secure/bidirule
vendor/golang.org/x/text/transform
vendor/golang.org/x/text/unicode/bidi
vendor/golang.org/x/text/unicode/norm |
|
Another problem people are running into is getting confused with dependencies on old/vulnerable versions of integration dependencies, for example, old version of Gin https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMGINGONICGIN-550031. The core SDK (the For the framework integrations like Gin, Echo, etc, we've not bumped our dependencies as per our SDK development philosophy with the intention of not pushing people to upgrade their dependencies and keeping as broad compatibility as possible. Users are ultimately in control of what version of a framework they want to use, and At the moment, one can't use $ go get github.com/getsentry/sentry-go@latest github.com/gin-gonic/gin@v1.3.0
go: github.com/getsentry/sentry-go latest => v0.10.0
go get: inconsistent versions:
github.com/getsentry/sentry-go@v0.10.0 from github.com/getsentry/sentry-go@latest requires github.com/gin-gonic/gin@v1.4.0 (not github.com/gin-gonic/gin@v1.3.0)We could change that dependency version bump policy specifically for Go, but ultimately having separate modules for framework integrations would reduce (to zero) the module dependency list of the top level module. |
|
It is unfortunate that, with the introduction of Go Modules, tools in the Go ecosystem often focus solely on the module dependency tree, ignoring that not all packages in a module are actually part of the build. The See Minimal Version Selection for more details on how the Go tool decides which versions of dependencies are part of the final build list. |
|
This sort of use-case is exactly what we had in mind when we added module graph pruning in Go 1.17. Module graph pruning would allow To activate module graph pruning, you'd need to bump the |
As I understand it, the main purpose of breaking the cloud SDK into multiple modules is so that releases of those modules can happen on independent schedules. (For example, |
Commit generated with this command go mod tidy -go=1.17 -compat=1.15 Using Go version 1.17.5. The idea is to enable consumers of sentry-go to be able to use module graph pruning (Go 1.17+ required), such that dependencies of sentry-go that are otherwise unused by end users can be pruned away. See getsentry#156 (comment)
Commit generated with this command go mod tidy -go=1.17 -compat=1.15 Using Go version 1.17.5. The idea is to enable consumers of sentry-go to be able to use module graph pruning (Go 1.17+ required), such that dependencies of sentry-go that are otherwise unused by end users can be pruned away. See getsentry#156 (comment) Also update CI workflow to check for go.mod tidiness using the same line as used to generate this commit. Update data race tests that were still accidentally being run on Go 1.16 instead of 1.17.
|
Thanks for chiming in @bcmills ❤️ I guess I overlooked the need to declare The next release should enable pruning. Perhaps that addresses most needs and obviates the need to split into multiple submodules (which comes with a higher toll on the development and release processes). |
Commit generated with this command go mod tidy -go=1.17 -compat=1.15 Using Go version 1.17.5. The idea is to enable consumers of sentry-go to be able to use module graph pruning (Go 1.17+ required), such that dependencies of sentry-go that are otherwise unused by end users can be pruned away. See #156 (comment) Also update CI workflow to check for go.mod tidiness using the same line as used to generate this commit. Update data race tests that were still accidentally being run on Go 1.16 instead of 1.17.
go1.17- also can works keep the original dependency version
go1.17- also can works keep the original dependency version
go1.17- also can works keep the original dependency version
cleptric
added
Type: Feature
Type: Breaking
Type: Improvement
Status: Backlog
and removed
Type: Discussion
Version: v1
labels
|
It looks like this is safe to close now as wont-fix/obsolete, with module pruning in place. |
I agree! Closed. |
|
Module pruning helps to keep the go.mod file smaller for importers, but the problem remains because Go does not perform version resolution at a package level but at a module level, so I believe this means anyone importing dependencies that overlap with the large depth graph of sentry-go will still be impacted by it, because sentry-go's mod file will still impact which minimum version is selected. See this rejected proposal for more details: But this issue of the impact on version selection might not be meaningful. It's unclear for me as of yet. |
|
Hi Leigh, Thanks for your comment(s), you always bring up interesting points, much appreciated.
Has that been meaningful in any project you worked on? When weighing the advantages and disadvantages of maintaining multi-module repositories, my experience leads me to lean towards the belief that managing multi-module repos demands a greater investment in customized tooling. On the other hand, single module repositories have made some advancements over the years, especially with the introduction of module pruning. Perhaps there's off-the-shelf tooling that makes releasing and developing multi-module repos a breeze?! Have you used anything you'd recommend? Most recently I've been using |
Not that is actively impacting me. And it's unclear to me if this will meaningfully negatively impact a project, it really only poses a problem if folks want to use an old minor version for some reason, which is not particularly common. If you did want to explore multiple modules, then yup, I think go.work would be the way to go and then keep the release process simple and just always tag all modules regardless of whether they contain any changes so you're still developing as a single unit. But I think closing this issue is a reasonable tradeoff. |
The topic has popped up in a few occasions. Some don't like to see a long list of indirect dependencies in their
go.modfiles.#75 (comment)
#75 (comment)
#79
The text was updated successfully, but these errors were encountered: