Skip to content

Detects GOROOT for go.mod using make#386

Merged
codefromthecrypt merged 4 commits intomasterfrom
detect-goroot-make
Oct 6, 2021
Merged

Detects GOROOT for go.mod using make#386
codefromthecrypt merged 4 commits intomasterfrom
detect-goroot-make

Conversation

@codefromthecrypt
Copy link
Copy Markdown
Contributor

@codefromthecrypt codefromthecrypt commented Oct 4, 2021

This switches from external (GitHub Actions workflow) to internal
(Makefile) for configuring GOROOT. Doing so reduces the amount of
copy/paste.

This also drops build cache, and moves tools to Tools.mk to
make the go cache config simpler and more precise.

A future change will remove configure_go.sh entirely, by rewriting how
internal images are produced.

@codefromthecrypt codefromthecrypt force-pushed the detect-goroot-make branch 4 times, most recently from d7a748d to b3512d4 Compare October 5, 2021 21:15
This switches from external (GitHub Actions workflow) to internal
(Makefile) for configuring GOROOT. Doing so reduces the amount of
copy/paste. To further simplify, this drops caching of the build cache
as it is too complicated to maintain.

A future change will remove configure_go.sh entirely, by rewriting how
internal images are produced.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Comment thread Makefile
# * GOROOT ensures versions don't conflict with /usr/local/go or c:\Go
# * PATH ensures tools like golint can fork and execute the correct go binary.
#
# We may be using a very old version of Make (ex. 3.81 on macOS). This means we
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this took longer because I had to learn about floor features of make, and how things like exporting variables are very hard given the floor version installed on OS/x. Rather than force all devs to upgrade Make, this bears with the glitches and hopefully documents why.

Comment thread Tools.mk
@@ -0,0 +1,7 @@
# Copyright 2021 Tetrate
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this file is a much more appropriate way to taint the go mod cache

Comment thread Makefile
test_packages := . ./internal/...
test: ## Run all unit tests
@printf "$(ansi_format_dark)" test "running unit tests"
@go test . $(test_packages)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

extra dot

Comment thread Makefile Outdated
go_release := $(shell sed -ne 's/^go //gp' go.mod)
# https://github.com/actions/runner/blob/master/src/Runner.Common/Constants.cs
github_runner_arch := $(if $(findstring $(shell uname -m),x86_64),X64,ARM64)
goroot_github_env := $(GOROOT_$(subst .,_,$(go_release))_$(github_runner_arch))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

$(go_release:.=_) feels suspiciously like it could work, but it calls pathsubst which implies the inputs are like files, so it doesn't accomplish replacing a character anywhere (ala tr)

Comment thread Makefile

# We must ensure `go` executes with GOROOT and PATH variables exported:
# * GOROOT ensures versions don't conflict with /usr/local/go or c:\Go
# * PATH ensures tools like golint can fork and execute the correct go binary.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this may seem crazy, but it is really required and literally because golint-ci forking go. Took forever to figure this out.

echo "${go_root}/bin" >>"${GITHUB_PATH}"

# Add the OS-specific GOCACHE (build cache) variable for actions/cache
echo GOCACHE=$(${go} env GOCACHE) >> "${GITHUB_ENV}"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we could continue to support this, if anyone feels strongly about it. I noticed basically no one does and even actions/cache decided to not document doing this as it was too complicated

Comment thread Makefile
goarch := $(shell go env GOARCH)
goexe := $(shell go env GOEXE)
# This selects the goroot to use in the following priority order:
# 1. ${GOROOT} - Ex actions/setup-go
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is only here to allow someone to ad-hoc test using actions/setup-go. We no longer use it though, as we use the runner variables.

Comment thread .github/workflows/commit.yaml Outdated
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
path: ~/go/pkg/mod
# go.mod for go release version, go.sum for modules used, and Tools.mk for 'go run' tools
key: test-${{ runner.os }}-go-${{ hashFiles('go.mod', 'go.sum', 'Tools.mk') }}
restore-keys: test-${{ runner.os }}-go-
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

taking the go version out of the restore key is ok as we aren't building with multiple versions of go intentionally. Basically, when we upgrade go it will dump the old cache, but so would it have before this simplification.. just it would have with more lines!

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Comment thread Makefile
goroot_path := $(shell go env GOROOT 2>/dev/null)
goroot := $(firstword $(GOROOT) $(github_goroot_val) $(github_goroot_cache) $(goroot_path))

ifndef goroot
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added this because I was blind due to a typo in my Dockerfile until I added it :D

@codefromthecrypt codefromthecrypt merged commit 9b2df84 into master Oct 6, 2021
@codefromthecrypt codefromthecrypt deleted the detect-goroot-make branch October 6, 2021 02:10
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

thanks for the look @mathetake!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants