Skip to content

Conversation

@jannotti
Copy link
Contributor

@jannotti jannotti commented Nov 7, 2025

For go tool usage, it uses -modfile and new files tool.mod and tool.sum so that build dependency versions are clear, but not mixed with runtime dependencies.

Tools that are invoked from a single place, (msgp, golangci-lint, swagger) are handled with go run which avoids mingling dependencies for different tools in tool.mod, but still only puts the version in one place.

Summary

Test Plan

@jannotti jannotti requested a review from cce November 7, 2025 20:29
@jannotti jannotti self-assigned this Nov 7, 2025
@jannotti jannotti changed the title Use go tool and separate module file, tool.mod for stringer Vuild: Use go tool and separate module file, tool.mod for stringer Nov 7, 2025
@cce
Copy link
Contributor

cce commented Nov 7, 2025

This is a good start, I tried making a tools.mod too at one point but gave up, because go-swagger broke backwards compatibility when they updated for Go versions ... fixed in go-swagger/go-swagger#3239 but now we're using github.com/algorand/go-swagger until their next official release. So having a tools.mod should work now I think. There might be dependency clashes with golangci-lint because it imports so many other packages, we might want to just run that with a make target that runs go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@VERSION instead (?)

@codecov
Copy link

codecov bot commented Nov 7, 2025

Codecov Report

❌ Patch coverage is 52.77778% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.42%. Comparing base (f2da11f) to head (87c9c84).
⚠️ Report is 6 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
data/transactions/logic/fields_string.go 71.11% 0 Missing and 13 partials ⚠️
agreement/coservicetype_string.go 0.00% 3 Missing ⚠️
data/transactions/application_string.go 0.00% 3 Missing ⚠️
ledger/store/trackerdb/hashkind_string.go 0.00% 3 Missing ⚠️
logging/logspec/agreementtype_string.go 0.00% 3 Missing ⚠️
logging/logspec/component_string.go 0.00% 3 Missing ⚠️
logging/logspec/ledgertype_string.go 0.00% 3 Missing ⚠️
agreement/actiontype_string.go 66.66% 0 Missing and 1 partial ⚠️
agreement/eventtype_string.go 66.66% 0 Missing and 1 partial ⚠️
agreement/statemachinetag_string.go 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6491      +/-   ##
==========================================
- Coverage   47.57%   47.42%   -0.16%     
==========================================
  Files         666      659       -7     
  Lines       88478    88417      -61     
==========================================
- Hits        42096    41932     -164     
- Misses      43618    43712      +94     
- Partials     2764     2773       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jannotti jannotti changed the title Vuild: Use go tool and separate module file, tool.mod for stringer Build: Use go tool and separate module file, tool.mod for stringer Nov 7, 2025
@cce
Copy link
Contributor

cce commented Nov 7, 2025

@jannotti
Copy link
Contributor Author

jannotti commented Nov 7, 2025

That was on https://github.com/cce/go-algorand/commits/go1.24

What became of that? We skipped 1.24 for 1.25 but didn't make all the extra changes?

@jannotti jannotti force-pushed the go-tool branch 2 times, most recently from 051fb3a to cb84f14 Compare November 10, 2025 18:07
`go tool` is nice for managing versions in the tool.mod file, but
there's no point when you have excatly one call to the tool.
@jannotti
Copy link
Contributor Author

jannotti commented Nov 10, 2025

This is reviewable now, but I took out the caching of gotestsum in CI, which maybe costs us a little time. It would be nice if we could install any tools in tool.mod, and them cache them. I wouldn't want to list them all out though. I think we would need to in order to a) Convince go to install them, and b) list their paths for caching.

@cce ideas?

@jannotti jannotti changed the title Build: Use go tool and separate module file, tool.mod for stringer Build: Use go tool and separate module file, go run to remove some build scripts Nov 11, 2025
@jannotti jannotti marked this pull request as ready for review November 11, 2025 18:09
@cce
Copy link
Contributor

cce commented Nov 12, 2025

I think the go mod and build caching we're getting already from setup-go should be helping caching the compilation of gotestsum and its dependencies already?

@jannotti
Copy link
Contributor Author

I think the go mod and build caching we're getting already from setup-go should be helping caching the compilation of gotestsum and its dependencies already?

I'll try to figure that out, but if it is, why did we have explicit gotestsum caching tasks in CI?

I would think that just installing/compiling Go would not cache the compilation of these tools unless they've been invoked.

@jannotti jannotti requested a review from gmalouf November 12, 2025 14:35
@cce
Copy link
Contributor

cce commented Nov 12, 2025

The entire go build cache and mod cache directories are saved and restored at the beginning and end of CI jobs ... so eventually it should make it in there.

I added the gotestsum caching to CI in #6397 because the additional step of running install_buildtools.sh just to get that one tool was slowing down the builds, especially since we only needed one of the several tools installed in there. So I made it its own step and added caching for good measure.

@cce
Copy link
Contributor

cce commented Nov 12, 2025

I would think that just installing/compiling Go would not cache the compilation of these tools unless they've been invoked.

Oh that's true since not all jobs are invoking gotestsum, I guess it might not be guaranteed to be cached in the go build/mod cache ... but that's why I added a cache-prefix key to the setup-go job so that the different types of jobs with different possible cache end states share cache.

@jannotti jannotti merged commit e1ff8a1 into algorand:master Nov 12, 2025
39 checks passed
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.

3 participants