Skip to content
This repository was archived by the owner on Aug 29, 2023. It is now read-only.

Conversation

@galargh
Copy link
Contributor

@galargh galargh commented Feb 7, 2023

Description

This PR upgrades go to 1.20.x. It also introduces automated migration rule for deprecated packages - see math/rand in https://tip.golang.org/doc/go1.20.

My proposal for the automated migration is as follows:

# 1. replace rand.Seed(*) with rand := rand.New(rand.NewSource(*))
#     if math/rand is imported
# 2. replace math/rand import with crypto/rand import
#     if crypto/rand is not imported yet and only Read from rand is called
# 3. add crypto/rand import as crand and replace rand.Read with crand.Read
#     if crypto/rand is not imported yet and methods other than Read from rand are called too

@marten-seemann WDYT? If you're okay with this migration, we can proceed with the release on 2023-02-08.

Testing

Before Merge
After Merge

@galargh galargh marked this pull request as ready for review February 7, 2023 13:44
@galargh galargh requested a review from a team as a code owner February 7, 2023 13:44
@marten-seemann
Copy link
Contributor

I don't think this works. Consider the following code:

func NewStruct() *MyStruct {
     rand.Seed(1337)
     return &MyStruct{}
}

func (m *MyStruct() RandomNumber() int {
     return rand.Int()
}

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

@marten-seemann Yes, that's right. It's the case here for example: https://github.com/libp2p/go-libp2p-pubsub/blob/973fef56e1e6fd6afc568ea3c1c2f654f7754d49/backoff.go#L46 (from description; edit I just noticed uCI is not configured for it).

There, I'd expect the CI to fail and the PR to require manual intervention.

My assumption is that it's not the case in most of our packages and that this would work for the majority of the cases. I'm not aiming at 100% success rate. But that's exactly why I wanted your input on this because you definitely have a better view of the land.

One more thing I wanted to change was to replace rand.Seed with r := rand.Seed rather than rand := rand.Seed because the latter might actually hide some errors.

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

The alternative would be to do nothing which could be a fine starting point. We can see in practice how many repos are affected.

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

Let me try to retrieve how many might be caught in this.

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

Using seed (12):

[
  "filecoin-project/go-address",
  "filecoin-project/go-data-segment",
  "ipfs/go-ds-flatfs",
  "ipfs/go-ipfs-util",
  "ipfs/go-mfs",
  "ipld/go-storethehash",
  "ipni/go-indexer-core",
  "libp2p/go-libp2p",
  "libp2p/go-libp2p-testing",
  "libp2p/go-yamux",
  "libp2p/hydra-booster",
  "multiformats/go-multiaddr"
]

Using read but not crypto (29):

[
  "filecoin-project/go-address",
  "filecoin-project/go-data-segment",
  "filecoin-project/go-hamt-ipld",
  "ipfs/go-cid",
  "ipfs/go-cidutil",
  "ipfs/go-datastore",
  "ipfs/go-ds-badger",
  "ipfs/go-ds-badger2",
  "ipfs/go-ds-badger3",
  "ipfs/go-filestore",
  "ipfs/go-graphsync",
  "ipfs/go-ipfs-chunker",
  "ipfs/go-ipfs-cmds",
  "ipfs/go-ipfs-util",
  "ipfs/go-libipfs",
  "ipfs/go-merkledag",
  "ipfs/go-mfs",
  "ipfs/go-unixfs",
  "ipld/go-car",
  "ipld/go-ipld-prime",
  "ipld/go-storethehash",
  "libp2p/go-libp2p",
  "libp2p/go-libp2p-gorpc",
  "libp2p/go-libp2p-xor",
  "libp2p/go-mplex",
  "libp2p/go-msgio",
  "multiformats/go-base32",
  "multiformats/go-multiaddr",
  "multiformats/go-multibase"
]

@marten-seemann
Copy link
Contributor

The alternative would be to do nothing which could be a fine starting point. We can see in practice how many repos are affected.

That would be my preference. The worst thing that'll happen is a slight performance hit, isn't it?

@marten-seemann
Copy link
Contributor

Using seed (12):

[
  "filecoin-project/go-address",
  "filecoin-project/go-data-segment",
  "ipfs/go-ds-flatfs",
  "ipfs/go-ipfs-util",
  "ipfs/go-mfs",
  "ipld/go-storethehash",
  "ipni/go-indexer-core",
  "libp2p/go-libp2p",
  "libp2p/go-libp2p-testing",
  "libp2p/go-yamux",
  "libp2p/hydra-booster",
  "multiformats/go-multiaddr"
]

Using read but not crypto (29):

[
  "filecoin-project/go-address",
  "filecoin-project/go-data-segment",
  "filecoin-project/go-hamt-ipld",
  "ipfs/go-cid",
  "ipfs/go-cidutil",
  "ipfs/go-datastore",
  "ipfs/go-ds-badger",
  "ipfs/go-ds-badger2",
  "ipfs/go-ds-badger3",
  "ipfs/go-filestore",
  "ipfs/go-graphsync",
  "ipfs/go-ipfs-chunker",
  "ipfs/go-ipfs-cmds",
  "ipfs/go-ipfs-util",
  "ipfs/go-libipfs",
  "ipfs/go-merkledag",
  "ipfs/go-mfs",
  "ipfs/go-unixfs",
  "ipld/go-car",
  "ipld/go-ipld-prime",
  "ipld/go-storethehash",
  "libp2p/go-libp2p",
  "libp2p/go-libp2p-gorpc",
  "libp2p/go-libp2p-xor",
  "libp2p/go-mplex",
  "libp2p/go-msgio",
  "multiformats/go-base32",
  "multiformats/go-multiaddr",
  "multiformats/go-multibase"
]

It would probably make sense to filter out test files here.

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

The alternative would be to do nothing which could be a fine starting point. We can see in practice how many repos are affected.

That would be my preference. The worst thing that'll happen is a slight performance hit, isn't it?

Given the numbers, how about we automatically change (math/rand).Read to (crypto/rand).Read but we leave rand.Seed as is for the owners to fix?

@marten-seemann
Copy link
Contributor

The alternative would be to do nothing which could be a fine starting point. We can see in practice how many repos are affected.

That would be my preference. The worst thing that'll happen is a slight performance hit, isn't it?

Given the numbers, how about we automatically change (math/rand).Read to (crypto/rand).Read but we leave rand.Seed as is for the owners to fix?

I'd prefer to leave that decision to package maintainers. If I remember correctly, math/rand.Read was significantly faster, and if you're generating large amounts of pseudorandom data (e.g. for tests), this might become relevant.

Since the function is deprecated, it will start showing up in staticcheck when we drop Go 1.19.

@galargh
Copy link
Contributor Author

galargh commented Feb 8, 2023

OK, I think that's fair enough. Let me revert the automatic fix changes and we'll proceed with the release today :)

@galargh galargh merged commit 453222d into next Feb 8, 2023
@galargh galargh deleted the go-1.20 branch February 8, 2023 08:49
galargh added a commit that referenced this pull request Feb 8, 2023
* update go version to 1.20.x

* fix: go 1.20 upgrade

* Revert "fix: go 1.20 upgrade"

This reverts commit ceb72ef.
galargh added a commit that referenced this pull request Feb 8, 2023
* Add option to skip `32-bit` go test (#412)

Introduce an option to configure `go-test` to allow completely skipping
`32-bit` tests.

Fixes #388

* Run at most 1 dispatch job per ref (#414)

* fix: check if git tag returns any results (#415)

* make go generate print the commands it executs (#440)

* use pull_request_target event for release-check workflow (#295)

* Revert "include cross-package coverage in codecov"

* Revert "Revert "include cross-package coverage in codecov""

* Make automerge a reusable workflow (#260)

* move automerge from template to workflows

* make automerge reusable and use it from new automerge template

* pass parent job name to reusable automerge

* check github actions yamls (#272)

* check github actions yamls

* make yaml linter happy about go-test

* mention VS Code YAML extension in the readme

* add info about other YAML checking extensions

* make yaml checker more generic

* use validate-yaml-schema action from mainline (#277)

* upgrade lewagon/wait-on-check-action to v1.1.1 (#278)

* always add a version.json file if it doesn't exist (#281)

* fix go-test runner string

* check if tag already exists in release-check (#287)

* allow specifying custom PATH for 386 arch (#289)

* use pull_request_target event for release-check workflow

* add comment on missing version.json

* chore: revert release checker path trigger change

* chore: add footnote when non-docs files are modified with the release

* fix: prev version calculation

* feat: allow configuring custom go-test runners (#443)

* feat: allow configuring custom go-test runners

* docs: update readme to include info on configuration variables

* feat: allow skipping go-test on certain OSes (#455)

* feat: standarise JSON config reading

* feat: allow skipping go-test on certain OSes

* fix: go-test conditional

* chore: show config after extracting it

* chore: udpate actions and go modules (#458)

* fix: source read-config from next for now

* simplify Go version upgrade procedure (#280)

* chore: simplify Go version upgrade procedure

* chore: add default for the go-version input of release-check

* Update .github/actions/copy-workflow-go/action.yml

* Update configs/README.md

Co-authored-by: Laurent Senta <[email protected]>

---------

Co-authored-by: Laurent Senta <[email protected]>

* Go through all the workflows and clean them up ahead of the next major release (#462)

* chore: clean up deprecated set-output

* chore: do not use substitution inside run

* chore: do not use substitution in if

* chore: skip env var brakets where possible

* fix: env var substitution

* fix: double toJSON

* Update templates/.github/workflows/js-test-and-release.yml

* feat: create gh releases in release-check/releaser workflows (#456)

* feat: create gh releases in release-check/releaser workflows

* fix: fill expr in release check workflow

* fix: add missing gh token in release check

* fix: add missing prev version env var in release check workflow

* fix: release check in release check

* chore: clean up obsolete step from releaser

* fix: step outputs in release workflows

* fix: labels in releaser

* fix: action gh release

* update go version to 1.20.x (#463)

* update go version to 1.20.x

* fix: go 1.20 upgrade

* Revert "fix: go 1.20 upgrade"

This reverts commit ceb72ef.

* clean up where source ref was set to next (#464)

* perform self-review before final release

---------

Co-authored-by: Masih H. Derkani <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Laurent Senta <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants