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

Build: Golang 1.20 upgrade. #5462

Merged
merged 14 commits into from Jun 15, 2023
Merged

Conversation

gmalouf
Copy link
Contributor

@gmalouf gmalouf commented Jun 8, 2023

Summary

Exactly what it sounds like; upgrade go-algorand to compile with Golang 1.20.5.

Note: Upon merge, developers need to re-run ./scripts/buildtools/install_buildtools.sh to get updated versions of our buildtools.

Test Plan

Existing unit/functional/integration tests all pass. Builds (on PR, nightly, etc) all function as intended.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #5462 (a167659) into master (8ab4aad) will decrease coverage by 4.56%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5462      +/-   ##
==========================================
- Coverage   55.63%   51.07%   -4.56%     
==========================================
  Files         447      447              
  Lines       63405    63401       -4     
==========================================
- Hits        35274    32384    -2890     
- Misses      25751    28542    +2791     
- Partials     2380     2475      +95     
Impacted Files Coverage Δ
agreement/actor.go 47.36% <ø> (ø)
agreement/cryptoRequestContext.go 100.00% <ø> (ø)
agreement/events.go 66.03% <ø> (-1.53%) ⬇️
agreement/proposalManager.go 94.54% <ø> (-1.82%) ⬇️
agreement/proposalStore.go 100.00% <ø> (ø)
agreement/proposalTracker.go 94.44% <ø> (ø)
agreement/router.go 99.02% <ø> (ø)
agreement/sort.go 77.27% <ø> (-22.73%) ⬇️
agreement/voteAggregator.go 84.73% <ø> (-1.53%) ⬇️
agreement/voteAuxiliary.go 84.00% <ø> (ø)
... and 43 more

... and 147 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

go.sum Show resolved Hide resolved
@cce
Copy link
Contributor

cce commented Jun 9, 2023

Also look at bumping https://github.com/algorand/go-algorand/blob/master/scripts/buildtools/versions for golangci-lint and golang.org/x/tools, perhaps gotestsum too

@gmalouf
Copy link
Contributor Author

gmalouf commented Jun 9, 2023

Also look at bumping https://github.com/algorand/go-algorand/blob/master/scripts/buildtools/versions for golangci-lint and golang.org/x/tools, perhaps gotestsum too

Yeah, had bumped golangci-lint locally, been spending time putting in excludes or fixing minor issues.

.golangci.yml Outdated
confidence: 0.8

# In order to disable any default rules, you have to list out all rules you would like enabled.
# We explicitly disable (vs omitting) rules to make it clear how we are diverging.
Copy link
Contributor

@cce cce Jun 12, 2023

Choose a reason for hiding this comment

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

Why not just add the ones we don't like to the exclude-rules section below? It seems like less work than checking the revive default rules list periodically to see if it changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't clear you could finetune revive's sub-rules in exclude-rules and saw multiple PR discussions laying out that going off the beaten path overrode golangci's default config for revive (requiring a full lay out like so).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be willing to pull cce@a167659 ?

@gmalouf gmalouf marked this pull request as ready for review June 13, 2023 19:32
@gmalouf gmalouf requested a review from a team as a code owner June 13, 2023 19:32
@gmalouf gmalouf requested review from excalq and algobarb June 13, 2023 19:32
@algogm algogm requested a review from cce June 14, 2023 00:15
@bbroder-algo
Copy link
Contributor

it looks ok to me gary

excalq
excalq previously approved these changes Jun 14, 2023
Copy link
Contributor

@excalq excalq left a comment

Choose a reason for hiding this comment

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

Nice to see this! HyperFlow is assessing what corresponding changes are needed among Jenkinsfiles and Dockerfiles in the CI repo.

cce
cce previously approved these changes Jun 14, 2023
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

LGTM

@gmalouf gmalouf dismissed stale reviews from cce and excalq via a167659 June 14, 2023 15:25
@gmalouf gmalouf requested a review from excalq June 14, 2023 15:48
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

LGTM. CI-related changes, get_golang_version, and go.mod changes look good to me.

Other than formatting changes, I guess the other changes were also to support the new golang version that I saw in some files:

  • Removing .(uint64) and .(int)
  • Adding [string]

@gmalouf gmalouf merged commit 9aee021 into algorand:master Jun 15, 2023
25 checks passed
@gmalouf gmalouf deleted the golang-v20-upgrade branch June 15, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants