Move repo dependency management from go dep to modules #1634
Assignees
Comments
This was referenced Aug 22, 2019
leighmcculloch
added a commit
that referenced
this issue
Aug 22, 2019
Remove the `source` parameter for the `github.com/asaskevich/govalidator` dependency in the dep Gopkg.* files. This change is being made to simplify the `go.mod` file that will be generated when we move to Modules (#1634). The `source` parameter for the `github.com/asaskevich/govalidator` dependency is pointing to the same repository as the dependency name, just with a `.git` extension. GitHub treats the two repository URLs the same and it was probably an error that we set the source to that value. The presence of the `source` parameter causes `go mod init` when converting from go dep to go modules to think the `source` is pointing to a fork which adds an extra unnecessary `replace` directive to the the `go.mod` file that is generated. This change is expected to be a no-op on existing functionality and on the dependencies that get pulled in during a build as it should cause no changes to the dependencies that `dep` pulls in.
This was referenced Aug 23, 2019
Merged
leighmcculloch
added a commit
that referenced
this issue
Aug 26, 2019
…1642) Make @bartekn's fork of the `throttled` dependency the primary dependency for that dependency instead of being just a custom source, which is now hosted at `github.com/stellar/throttled`. This is part of the initiative to move to Modules (#1634) and is a small change to simplify our Gopkg.toml/lock files to make that transition possible and simpler. Using the fork seems to be confusing the go tooling in go1.12 and go1.13rc1, with both behaving a little differently. There is an issue open golang/go#33795 about it. The issue seems to be limited to replacing modules that don't have a `go.mod`, which is the case with the `throttled` module. Since this fork is the only fork of a dependency we use and it is simple package, it is straight forward for us to switch to using the fork fully in name rather than to try and make it work consistently with both versions, and we did that in bartekn/throttled#2. See that issue for more details about why we felt like this is the right move. For context, in modules a `replace` directive is how we tell the go toolchain that we want to use a fork of a module. However, a replace statement only applies when go commands are executed inside this repo as the main module and so importers of our repo won't be using the fork. This doesn't really matter for `throttled` because we only use it in `internal` and `main` modules of `horizon`, but because of this the go toolchain does care about the original source since importers of our repo will get the original source, not the replacement. The revision of `stellar/throttled` that we are importing did change. You can see the diff here: stellar/throttled@c99eef3...89d7581
This was referenced Aug 26, 2019
Closed
leighmcculloch
added a commit
that referenced
this issue
Aug 27, 2019
…to v1.4.0 (#1652) Upgrade dependency `github.com/go-sql-driver/mysql` from `2e00b5cd7039` (Dec 24, 2016), which is a couple commits after `v1.3.0`, to `v1.4.0` (Jun 3rd, 2018). This is part of making the transition to Modules (#1634). The `mysql` module is used directly by this repositories `support/db` package. The `mysql` module is also used indirectly via the `github.com/jmoiron/sqlx` module which we have pinned to `v1.2.0`. We use the `sqlx` package in `services/ticker`, `services/horizon` and `support/db`. The `sqlx` package's tests have a dependency on `mysql` `v1.4.0` and even though it is only their tests, Modules carries that minimum requirement to our project, even though we are not dependent on the tests. ``` +-----------------------------+ | +------+ +-----------+ | +----------->+ sqlx +<-----+ sqlx.test | | | v1.2.0 | +------+ +-----------+ | | +-----------------------------+ +-----+------+ | | stellar/go | | +-----+------+ | | | | +-------+ | +----------->+ mysql +<----------+ +-------+ v1.4.0 ``` This isn't ideal for us because we don't particularly care about dependencies of our dependencies tests, but the perspective of the Go team has been that a dependencies test dependencies are a signal to compatability of the module in general. e.g. If the sqlx devs say that to test sqlx they need at least mysql v1.4.0, there's a good chance sqlx is only compatible with v1.4.0+. I considered two approaches to resolve this. 1. Upgrade `mysql` to `v1.4.0`, which is this change. 2. Downgrade `sqlx` two commits to drop it's `go.mod` file. The last two commits on the version we have are only the addition of a `go.mod` file. Without it we can pin the version of `mysql` that we have today. This isn't a good long term plan though, as sooner or later we're going to need to be bumped. I've gone with this change because it better embraces the best practices from the perspective of the core Go developers and it will probably cause us less problems long term. The diff in the mysql dependency is 69 commits, 38 changed files, 4,928 additions, 797 deletions, 12 new features, 12 bug fixes, and numerous other changes. It's not insignificant and is probably not possible for one of us to review and be able to meaningful identify if upgrading would be harmful. The best thing we can do is rely on our tests here.
This was referenced Aug 28, 2019
leighmcculloch
added a commit
that referenced
this issue
Aug 29, 2019
Update `ginkgo` and `gomega` to newer versions. The transition to Modules (#1634) requires some modules to be updated because Modules requires the same minor versions to be used across test dependencies of our dependencies. There are some dependencies we have imported that require these newer versions of these two modules. This is a low risk change since these modules are not used in production, only in tests.
7 tasks
leighmcculloch
added a commit
that referenced
this issue
Aug 30, 2019
…d2 (#1679) Update `github.com/rcrowley/go-metrics` for transition to Modules (#1634). From: rcrowley/go-metrics@a5cfc242a56b To: rcrowley/go-metrics@51425a2415d2 Diff: rcrowley/go-metrics@a5cfc24...51425a2 (41 commits) To make the transition to Modules possible. Some dependencies at their current version have incompatible relationships with other dependencies we have. Dep was more tolerant to incompatible versions than Modules. Making this change ahead of the PR that switches us to Modules makes that PR a functional no-op. The reason Modules is updating `go-metrics` is because the monorepo is also dependent on `github.com/ethereum/go-ethereum` which is in turn, via it's tests, dependent on the `exp` sub-package within `go-metrics`. The sub-package was added in rcrowley/go-metrics@51425a2. The `go` tool in this situation will automatically upgrade `go-metrics` to it's latest revision since it can see the `exp` is available there. This PR is however upgrading to the first or oldest revision that contains the sub-package we need to satisfy Modules. We could upgrade to the latest revision but the changes in the smaller diff are much less significant and pose a smaller risk than the changes that are in the diff (rcrowley/go-metrics@a5cfc24...cac0b30 144 commits) to take us all the way to latest. More details about how this was discovered can be found here: golang/go#33771 (comment)
This was referenced Aug 30, 2019
leighmcculloch
added a commit
that referenced
this issue
Aug 30, 2019
Update `gopkg.in/yaml.v2` for transition to Modules (#1634). From: `gopkg.in/yaml.v2@7ad95dd0798a` To: `gopkg.in/[email protected]` Diff: go-yaml/yaml@7ad95dd...v2.2.2 (96 commits) ### Goal and scope To make the transition to Modules possible. Some dependencies at their current version have incompatible relationships with other dependencies we have. Dep was more tolerant to incompatible versions than Modules. Making this change ahead of the PR that switches us to Modules makes that PR a functional no-op. The reason Modules is updating `yaml.v2` is because the monorepo is also dependent on `github.com/onsi/gomega` which had to be upgraded due to another dependency requirement. The version of `gomega` we're using is explicitly dependent on at least `v2.2.1` of `yaml.v2` requiring us to update to a newer version. I'm taking the dependency all the way to `v2.2.2` which is the latest patch release. While I don't understand all the changes deeply in the diff, the changes don't appear to be significant. As a bonus these newer versions of `yaml.v2` are released under a more desirable license, Apache License 2.0. The commit we were dependent on previously was released under LGPL 3.
leighmcculloch
added a commit
that referenced
this issue
Sep 3, 2019
Update `golang.org/x/...` packages for transition to Modules (#1634). - `golang.org/x/crypto` golang/crypto@7f87c0f...cc06ce4 (36 commits) - `golang.org/x/net` golang/net@9bc2a33...ba9fcec (516 commits) - `golang.org/x/sys` golang/sys@cc5685c...5da2858 (100 commits) - `golang.org/x/text` golang/text@1cbadb4...v0.3.2 (133 commits) To make the transition to Modules possible. Some dependencies at their current version have incompatible relationships with other dependencies we have. Dep was more tolerant to incompatible versions than Modules. Making this change ahead of the PR that switches us to Modules makes that PR a functional no-op. With some of the other package upgrades I've done I've attempted to be as conservative as possible, however with these `golang.org/x` packages are described as part of the Go project, but just stored separately to the main tree so they can be iterated on outside of Go releases. More can be read about them [here](https://github.com/golang/go/wiki/SubRepositories). We should be able to assume a certain degree of rigor has been maintained. For these packages I've gone with the versions that Modules by default attempted to retrieve. This makes their diffs largely impossible to meaningfully review, however we don't review changes in the Go standard library when new versions of Go are released, so I think these fall into the same bucket. As a side note we should be keeping these repositories up to date more often. They are only guaranteed to maintain compatibility with the last two versions of Go, which is Go 1.11 and Go 1.12. The versions we were importing were from a time prior to those releases and while it's unlikely there were issues there's no guarantee they would behave appropriately for the latest releases.
|
Reopening, because there are a couple follow-ups that were mentioned in the description on #1542:
|
|
Actually, going to close this. I've opened an issue for the first item in that list stellar-deprecated/integration-tests#6. @brahman81 is going to take the second item, and I completed the last item. |
This was referenced Sep 16, 2019
leighmcculloch
added a commit
that referenced
this issue
Sep 16, 2019
Move Go coding conventions from the README.md into the CONTRIBUTING.md file which discusses other requirements for code contributed. I'm in the process of adding a developing document that includes finer details about how to do things like add dependencies, update them, remove them, as part of the previous work switching to Modules (#1634). I noticed during that work that this section is in the projects readme but is primarily listing more things that someone contributing should be thinking about, and not so much someone who is browsing or importing the repository. It makes more sense for this to live in the contributing document.
leighmcculloch
added a commit
that referenced
this issue
Sep 16, 2019
Add a `DEVELOPING.md` capturing some basic instructions on how to develop in this repository, including instructions on how to use Modules to add, update and remove dependencies. To make a clear path for folks to add, update and remove dependencies. While we mostly use the standard Go modules features to manage dependencies they are relatively new in the ecosystem and it is helpful if we anyone adding dependencies knows how to get it right first time without needing to iterate, receive CI or PR feedback, and then do more work. We also use a non-standard [go.list](go.list) file so that we can see what our actual build/tested dependency versions are in PR diffs, according to `go list -m all`. CI will error if any steps haven't been followed for updating dependencies, but as @graydon pointed out when he was our first person doing an upgrade, it's helpful if we can give developers clear instructions so they have the opportunity to get it right and so it's not an unknown they're stepping into. It's worth addressing that there is some overlap of these instructional files, e.g. `README.md`, `DEVELOPING.md` and `CONTRIBUTING.md`, but each has a different target audience and are relevant at different stages. - `README.md`: Folks learning about our code, importing the Go SDK, installing from source. - `DEVELOPING.md`: Developers wanting to make changes to the code. - `CONTRIBUTING.md`: Developers wanting to contribute changes back to this repo. For this reason it would be overwhelming to dump all this information into `README.md`, or to clutter `CONTRIBUTING.md` with details that are no longer relevant to a developer when they are thinking about opening a PR. This is a follow up to #1634.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This repository uses dep to manage it's dependencies and the Go tooling and the community has shifted to use Modules. Modules were automatically enabled in Go 1.11+ when used in repositories outside a GOPATH, and will be always enabled in Go 1.13 that is currently in
rc1as of August 21st.We should move to Modules to remain consistent with the wider Go community, and to make it easier for developers contributing or depending on packages in this repository to use common tooling.
The text was updated successfully, but these errors were encountered: