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

chore(deps): update min go build version #8423

Merged
merged 3 commits into from
Feb 7, 2023
Merged

chore(deps): update min go build version #8423

merged 3 commits into from
Feb 7, 2023

Conversation

joshua-goldstein
Copy link
Contributor

@joshua-goldstein joshua-goldstein commented Nov 10, 2022

Problem

Currently the minimum version of Go required to build Dgraph is 1.12. This can be found at the top of the go.mod file. In order to bring in features like generics, we would have to bump up the minimum required version to 1.20. This PR shows the difference after changing 1.12 to 1.18 and running go mod tidy. Note that we already build Dgraph using Go 1.18, so bumping up to 1.19 is not a huge leap and will allow us to bring in many new features in the future.

Why are there now two require blocks?

This PR actually is not making any substantive changes to Dgraph's dependencies. The huge apparent increase in indirect dependencies is actually just Go showing us transitive dependencies that already existed. For more info see here. This is simply a change in how dependencies are described in the go.mod, not in the actual dependencies themselves. The first require block lists all direct dependencies, and the second require block lists all secondary (or tertiary, etc.) dependencies. These dependencies already existed - the go.mod just makes that more explicit.

@all-seeing-code
Copy link
Contributor

In terms of testing this, what else do you think we should do here other than CI/CD pipeline?

@skrdgraph skrdgraph marked this pull request as ready for review November 12, 2022 09:45
@skrdgraph
Copy link
Contributor

I want the tests to run, so I am opening this PR for a review. I will mark it back to draft right after. I think it will be interesting to see the results.

In terms of testing:

  • this is quite a huge change, we don't have coverage on all areas
  • we may* have to do this package by package (from how I see it)
  • these changes may also have performance impacts*, im not sure how - it may be good to merge LDBC PR for a baseline.

(I would be curious to see if this fixes up some long pending CVEs as well)

If you guys think of another ideas here, I would be interested to be part of those discussions - this is an interesting & a massive change.

@skrdgraph skrdgraph marked this pull request as draft November 12, 2022 09:55
@coveralls
Copy link

coveralls commented Nov 12, 2022

Coverage Status

Coverage: 67.256% (+0.1%) from 67.12% when pulling c164e43 on joshua/min-go into 695354d on main.

@skrdgraph
Copy link
Contributor

So the tests pass, I think this is not having a wide impact with API changes as we expected it to. I still feel like we should think about some points I raised in my last comment.

@joshua-goldstein
Copy link
Contributor Author

joshua-goldstein commented Nov 13, 2022

@skrdgraph @matthewmcneely I think there might be a misunderstanding about what is happening here (I was also confused). Note that all of the lines from go.mod that are "removed" are actually added again verbatim. Taking a closer look at the new go.mod file, one can see that there are now two separate "require" sections, one for direct dependencies and one for indirect dependencies. So our previous dependencies have not changed at all (version pins are still the same).

As for all of the "new" indirect dependencies, I stumbled upon the release notes for Go 1.17. Quoting from there:

If a module specifies go 1.17 or higher in its go.mod file, its go.mod file now contains an explicit require directive for every module that provides a transitively-imported package. (In previous versions, the go.mod file typically only included explicit requirements for directly-imported packages.)

and also

Because the number of explicit requirements may be substantially larger in an expanded Go 1.17 go.mod file, the newly-added requirements on indirect dependencies in a go 1.17 module are maintained in a separate require block from the block containing direct dependencies.

So basically this change is not actually changing our dependencies. It is simply showing us all of the transitive indirect dependencies that already existed. I will do some more digging but I think this change is less scary than it looks. It also makes more sense now since Go has strict backward compatibility guarantees, so requiring a higher Go version shouldn't cause any breaking changes or change our dependencies.

Side Note

Also one might wonder, why would Dgraph already have "indirect" dependencies in the go.mod, if they were not transitive? I think the reason is that Go also marks dependencies on packages that do not have a go.mod file as "indirect" (not a very good word in this case). E.g. if you check out https://github.com/microsoft/go-winio/tree/v0.4.15 you will see that there is no go.mod file at this version, so our dependency on it is already marked as "indirect".

@matthewmcneely
Copy link
Member

@joshua-goldstein Did you go mod tidy following the change of the Go version? IIRC, that's when Go tooling would automagically upgrade a package if the current version was incompatible with the newly defined version. But as you indicated, since backward-compatibility is guaranteed in Go, that situation would probably only ever be encountered if one was moving backward, say from 1.18 to 1.16.

Also agree that the indirect word is misleading.

@joshua-goldstein
Copy link
Contributor Author

@joshua-goldstein Did you go mod tidy following the change of the Go version? IIRC, that's when Go tooling would automagically upgrade a package if the current version was incompatible with the newly defined version. But as you indicated, since backward-compatibility is guaranteed in Go, that situation would probably only ever be encountered if one was moving backward, say from 1.18 to 1.16.

Also agree that the indirect word is misleading.

Yes, I did run a go mod tidy after the version upgrade (just double checked it again). Any reason you see not to bump up the version to 1.19? It would be nice to eventually incorporate generics, and there were some nice runtime additions in Go 1.19 like a soft memory limit. This could be very helpful for Badger (which we would also have to bump up).

I imagine this should be part of a major release since it would require dependencies using Dgraph and Badger to upgrade their minimum Go versions as well.

@matthewmcneely
Copy link
Member

I imagine this should be part of a major release since it would require dependencies using Dgraph and Badger to upgrade their minimum Go versions as well.

Agree with the 1.19 upgrade. I tend to like to wait for a couple of minor versions to be baked before upgrading... and Go is at 1.19.3, so yeah.

mangalaman93
mangalaman93 previously approved these changes Feb 2, 2023
@mangalaman93 mangalaman93 merged commit 41d4b99 into main Feb 7, 2023
@mangalaman93 mangalaman93 deleted the joshua/min-go branch February 7, 2023 17:27
github.com/hashicorp/vault/api v1.0.4
github.com/minio/minio-go/v6 v6.0.55
github.com/mitchellh/panicwrap v1.0.0
github.com/paulmach/go.geojson v0.0.0-20170327170536-40612a87147b
github.com/pierrec/lz4 v2.6.0+incompatible // indirect
github.com/pkg/errors v0.9.1
github.com/pkg/profile v1.2.1
github.com/prometheus/client_golang v1.11.1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we should upgrade this to the latest 1.14 which will include new go metrics around memory allocation and maybe GC. Minimum version in the release notes is golang 1.17 per https://github.com/prometheus/client_golang/releases so I think this will be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damonfeldman I opened a PR for this here. Looks like this bumps up some of our other dependencies, in particular the grpc library. We should be able to bring this in but we will want to make sure that there are no breakages.

all-seeing-code pushed a commit that referenced this pull request Feb 8, 2023
## Problem

Currently the minimum version of Go required to build Dgraph is 1.12.
This can be found at the top of the `go.mod` file. In order to bring in
features like generics, we would have to bump up the minimum required
version to 1.19. This PR shows the difference after changing 1.12 to
1.18 and running `go mod tidy`. Note that we already build Dgraph using
Go 1.18, so bumping up to 1.19 is not a huge leap and will allow us to
bring in many new features in the future.

## Why are there now two require blocks?

This PR actually is not making any substantive changes to Dgraph's
dependencies. The huge apparent increase in indirect dependencies is
actually just Go showing us transitive dependencies that already
existed. For more info see
[here](#8423 (comment)).
This is simply a change in how dependencies are described in the go.mod,
not in the actual dependencies themselves. The first require block lists
all direct dependencies, and the second require block lists all
secondary (or tertiary, etc.) dependencies. _These dependencies already
existed_ - the go.mod just makes that more explicit.
all-seeing-code pushed a commit that referenced this pull request Feb 8, 2023
## Problem

Currently the minimum version of Go required to build Dgraph is 1.12.
This can be found at the top of the `go.mod` file. In order to bring in
features like generics, we would have to bump up the minimum required
version to 1.19. This PR shows the difference after changing 1.12 to
1.18 and running `go mod tidy`. Note that we already build Dgraph using
Go 1.18, so bumping up to 1.19 is not a huge leap and will allow us to
bring in many new features in the future.

## Why are there now two require blocks?

This PR actually is not making any substantive changes to Dgraph's
dependencies. The huge apparent increase in indirect dependencies is
actually just Go showing us transitive dependencies that already
existed. For more info see
[here](#8423 (comment)).
This is simply a change in how dependencies are described in the go.mod,
not in the actual dependencies themselves. The first require block lists
all direct dependencies, and the second require block lists all
secondary (or tertiary, etc.) dependencies. _These dependencies already
existed_ - the go.mod just makes that more explicit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants