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: Remove build deps management #5915

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Jan 22, 2024

go run supports running arbitrary go modules like this: go run golang.org/x/tools/cmd/[email protected]

It downloads and compiles as needed, caching the result. By using that everywhere that our build wanted to demand a previous install of some program, we don't need any of the complexity around check_deps.sh, install_buildtools.sh and the like. This PR removes them and the bespoke version management we were doing with versions file. Now the versions are explicit in the source/Makefiles.

There shouldn't be a performance problem because the programs will run out of Go's local module cache. Let's see.

go run supports running an arbitrary go modules like this:
go:generate go run golang.org/x/tools/cmd/[email protected]

By using that everywhere that our build wanted to demand a previous
install of some program, we don't need any of the complexity around
`check_deps.sh`, `install_buildtools.sh` and the like.  This PR
removes them.

There shouldn't be a performance problem because the programs will run
out of Go's local module cache.  Let's see.
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.98%. Comparing base (877090b) to head (e1ab7ea).
Report is 46 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5915      +/-   ##
==========================================
- Coverage   55.99%   55.98%   -0.02%     
==========================================
  Files         478      478              
  Lines       67602    67602              
==========================================
- Hits        37855    37845      -10     
- Misses      27187    27196       +9     
- Partials     2560     2561       +1     

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

@jannotti jannotti marked this pull request as ready for review January 22, 2024 00:49
@jannotti jannotti requested a review from a team as a code owner January 22, 2024 00:49
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I like removal some of the shell scripts but I was nice to have all these deps listed in one place with versions. The install_buildtools.sh missed versions pinning but it is ok.
Maybe it is possible grepping for go run and buld such a scripts/buildtools/versions automatically. Or get them in go.mod somehow?

@jannotti
Copy link
Contributor Author

I like removal some of the shell scripts but I was nice to have all these deps listed in one place with versions.

I think it's probably better to git grep for the tool name when you have a question about what version we use. I wouldn't want to spend the time making a script to find the calls, integrate that script into the build so it stays up to date, and then spend time changing it if go run appears in a funny way like it does in here:

 xrun(["go", "run", "gotest.tools/[email protected]",...

git grep gotestsum and a human operator should always work.

Makefile Outdated Show resolved Hide resolved
Co-authored-by: ohill <[email protected]>
@algorandskiy algorandskiy requested a review from ohill April 10, 2024 18:33
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