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

Enhancement: 1. go mod tidy + 2. diffutils when mac OS #5398

Merged
merged 11 commits into from
May 25, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented May 18, 2023

Summary

Introducing make tidy and enforcing it in codegen_verification.sh as well as make sanity.

As go mod tidy's behavior can change from version to version of go, make tidy insists that the expected go build major version be applied.

(The build version is defined in scripts/get_golang_version.sh).

Also adding diffutils to configure_dev.sh mac install_or_upgrade()'s (see the comment below for an explanation)

Test Plan

CI and locally to ensure that make tidy works when the expected go-version hasn't already been installed.

@cce
Copy link
Contributor

cce commented May 19, 2023

I think if you added your preferred invocation of go mod tidy (with whatever -compat flags we should use) to this shell script, it would complain at the end if there were dirty / uncommitted changes to go.mod and go.sum. We use this pattern for checking if you forgot to run make msgp, the swagger gen stuff, similar use case https://github.com/algorand/go-algorand/blob/master/scripts/travis/codegen_verification.sh

@cce
Copy link
Contributor

cce commented May 19, 2023

You could also have this implemented with a few lines of circleci YAML or github actions YAML — I'm not sure that writing a custom Go program has any significant advantage over the shell and some CI glue e.g. https://github.com/katexochen/go-tidy-check/blob/main/action.yml

@tzaffi

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #5398 (7cbf8ca) into master (44f4da8) will increase coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5398      +/-   ##
==========================================
+ Coverage   54.99%   55.34%   +0.34%     
==========================================
  Files         452      452              
  Lines       63851    63851              
==========================================
+ Hits        35117    35339     +222     
+ Misses      26314    26083     -231     
- Partials     2420     2429       +9     

see 28 files with indirect coverage changes

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

wip

netdeploy: Copy ledger directory for kv tracker database (algorand#5392)

assembler: Error if extra args are present in pragmas (algorand#5400)

ledger: Exclude stake at R-320 that is expired by R (algorand#5403)

Co-authored-by: cce <[email protected]>

algod: Don't return a top level array from algod (algorand#5404)

revert

go get github.com/algorand/[email protected]

go get github.com/getkin/[email protected]

go mod tidy -compat=1.17

go get github.com/algorand/[email protected]

go mod tidy -compat=1.17

go get github.com/algorand/[email protected]

revert to master

github.com/getkin/kin-openapi v0.117.0

github.com/getkin/kin-openapi v0.117.0

github.com/getkin/kin-openapi v0.117.0

go get golang.org/x/mod/modfile

tidy #0

go get github.com/algorand/[email protected]

go mod tidy

tidy up after every go get

simplify

revert

tidier

tg

tg
Makefile Outdated Show resolved Hide resolved
@tzaffi tzaffi changed the title NOT READY FOR REVIEW: go mod tidier Enhancement: go mod tidy May 24, 2023
@tzaffi tzaffi marked this pull request as ready for review May 24, 2023 02:58
Makefile Outdated
Comment on lines 110 to 111
# NOTE: tidy will download and install go version GOLANG_VERSION_BUILD
# if this isn't the system's current version
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice that go install can manage this, but it still seems a little strange to me that this is done in the Makefile.

I'm not completely oppose to the change here, but exiting with an error that tells you the go version is wrong would be my preference. Maybe even split it out with a check target which checks your environment, and add that as a dependency to other things which might be wrongish with the wrong version of go installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makefile Outdated Show resolved Hide resolved
winder
winder previously approved these changes May 24, 2023
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

I'm mostly good with this, think you should add a call to tidy under the sanity make alias (I think just before the fix call).

cce
cce previously approved these changes May 24, 2023
@@ -86,6 +86,7 @@ elif [ "${OS}" = "darwin" ]; then
install_or_upgrade automake
install_or_upgrade python3
install_or_upgrade lnav
install_or_upgrade diffutils
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 able to get make lint to pass locally until I installed diffutils per these links:

Makefile Outdated Show resolved Hide resolved
@tzaffi tzaffi requested review from gmalouf, cce and winder May 24, 2023 23:02
@tzaffi
Copy link
Contributor Author

tzaffi commented May 24, 2023

I'm mostly good with this, think you should add a call to tidy under the sanity make alias (I think just before the fix call).

See the latest

@tzaffi tzaffi changed the title Enhancement: go mod tidy Enhancement: 1. go mod tidy + 2. diffutils when mac OS May 24, 2023
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

4 participants