Go: miscellaneous cleanup #196

epk · 2022-07-27T18:20:52Z

Signed-off-by: Aditya Sharma [email protected]

Description of your changes

Cleans up some of the stuff in the Go build module that is no longer relevant. Inline comments follow

I have:

Read and followed Upbound's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Tested all go targets with managed-control-plane and shimmer

Signed-off-by: Aditya Sharma <[email protected]>

epk · 2022-07-27T18:22:23Z

makelib/golang.mk

@@ -30,10 +30,6 @@ GO_SUBDIRS ?= cmd pkg
 # Optional. Additional subdirs used for integration or e2e testings
 GO_INTEGRATION_TESTS_SUBDIRS ?=

-# Optional directories (relative to CURDIR)
-GO_VENDOR_DIR ?= vendor


We don't vendor if GO111MODULE is on. GO111MODULE is set to on automatically if there is a go.mod file since Go 1.16: https://go.dev/blog/go116-module-changes

epk · 2022-07-27T18:26:02Z

makelib/golang.mk

-DEP_VERSION=v0.5.1
-DEP := $(TOOLS_HOST_DIR)/dep-$(DEP_VERSION)


DEP was deprecated long ago (Go 1.13 iirc)

epk · 2022-07-27T18:26:06Z

makelib/golang.mk

@@ -30,10 +30,6 @@ GO_SUBDIRS ?= cmd pkg
 # Optional. Additional subdirs used for integration or e2e testings
 GO_INTEGRATION_TESTS_SUBDIRS ?=

-# Optional directories (relative to CURDIR)
-GO_VENDOR_DIR ?= vendor
-GO_PKG_DIR ?= $(WORK_DIR)/pkg


Use the default/system global $GOPATH/pkg directory, this helps go mod cache ($GOPATH/pkg/mod) reuse between different repos.

Not to mention having $(WORK_DIR)/pkg messes up with vscode + gopls when it sees several thousand files.

I think I'm mostly concerned about this part of the changes. I agree with the cache reuse aspect; however I want to dig into why the mod cache was placed in the directories to get that context.

however I want to dig into why the mod cache was placed in the directories to get that context

To tie a bow on this, it looks like this behavior was introduced during the introduction to using Go modules here #74. Most likely this was done for two reasons:

we were still supporting dep at that time

it wasn't entirely clear how to work with go modules yet

With that context, I'm comfortable moving forward with this changeset.

epk · 2022-07-27T18:27:13Z

makelib/golang.mk

-# NOTE: the install suffixes are matched with the build container to speed up the
-# the build. Please keep them in sync.
-
-ifneq ($(GO_PKG_DIR),)
-GO_PKG_BASE_DIR := $(abspath $(GO_PKG_DIR)/$(PLATFORM))
-GO_PKG_STATIC_FLAGS := -pkgdir $(GO_PKG_BASE_DIR)_static
-endif
-
-GO_COMMON_FLAGS = $(GO_BUILDFLAGS) -tags '$(GO_TAGS)'
-GO_STATIC_FLAGS = $(GO_COMMON_FLAGS) $(GO_PKG_STATIC_FLAGS) -installsuffix static  -ldflags '$(GO_LDFLAGS)'


Use default/system global $GOPATH/pkg directory, helps with reuse across multiple repos

epk · 2022-07-27T18:27:47Z

makelib/golang.mk

-
-GO_COMMON_FLAGS = $(GO_BUILDFLAGS) -tags '$(GO_TAGS)'
-GO_STATIC_FLAGS = $(GO_COMMON_FLAGS) $(GO_PKG_STATIC_FLAGS) -installsuffix static  -ldflags '$(GO_LDFLAGS)'
+GO_COMMON_FLAGS = $(GO_BUILDFLAGS) -tags '$(GO_TAGS)' -trimpath


Add -trimpath to Go invocations:

-trimpath remove all file system paths from the resulting executable. Instead of absolute file system paths, the recorded file names will begin either a module path@version (when using modules), or a plain import path (when using the standard library, or GOPATH).

Signed-off-by: Aditya Sharma <[email protected]>

epk · 2022-07-27T19:39:46Z

makelib/local.mk

@@ -70,7 +70,7 @@ export BUILD_IMAGE_ARCHS=$(subst linux_,,$(filter linux_%,$(BUILD_PLATFORMS)))
 export TARGETARCH

 # Install gomplate
-GOMPLATE_VERSION := 3.7.0
+GOMPLATE_VERSION := 3.11.1


Missed this in one of the older PRs

Signed-off-by: Aditya Sharma <[email protected]>

tnthornton

Nice clean up @epk! Have a few comments below.

Also, I see that you tested this against managed-control-planes and shimmer. Could you also test this against:

crossplane/crossplane
upbound/official-providers

If we merge this and something breaks I'm ok with our repos being broken. However I'd like to be extra cautious about those that impact our external consumers (crossplane/crossplane) as well as the the providers use case with upbound/official-providers.

makelib/golang.mk

tnthornton · 2022-07-27T22:45:53Z

makelib/golang.mk

@@ -18,7 +18,7 @@
 # Optional. The Go Binary to use
 GO ?= go

-# Optional. Minimum Go version.
+# Optional. Required Go version.


Is it really the 'Required' version? What about if someone is using 1.19, etc (once those are released)?

GO_REQUIRED_VERSION=1.19 make ... :D

I just changed the comment and error message because I was not really checking for GO_VERSION < GO_REQUIRED_VERSION but rather GO_REQUIRED_VERSION == GO_VERSION

I just changed the comment and error message because I was not really checking for GO_VERSION < GO_REQUIRED_VERSION but rather GO_REQUIRED_VERSION == GO_VERSION

Gotcha. What're your thoughts on changing the check to this?:

GO_REQUIRED_VERSION <= GO_VERSION

That way we don't have to update the build submodule each time we upgrade our go version.

GO_REQUIRED_VERSION can be set per project: https://github.com/upbound/distro/blob/main/Makefile#L65

So we can change Go versions without needing to change the submodule.

Signed-off-by: Aditya Sharma <[email protected]>

epk · 2022-07-27T23:19:59Z

crossplane/crossplane:

~/src/github.com/crossplane/crossplane   master ±  make build.all
16:09:12 [ .. ] verify go modules dependencies are tidy
16:09:13 [ OK ] go modules are tidy
16:09:13 [ .. ] verify go modules dependencies have expected content
all modules verified
16:09:16 [ OK ] go modules dependencies verified
==> Linting /Users/adi/src/github.com/crossplane/crossplane/cluster/charts/crossplane

1 chart(s) linted, 0 chart(s) failed
16:09:17 [ .. ] helm dep crossplane 1.10.0-rc.0.36.g2015c64c.dirty
16:09:17 [ OK ] helm dep crossplane 1.10.0-rc.0.36.g2015c64c.dirty
16:09:18 [ .. ] Checking that e2e tests compile
16:09:19 [ OK ] Verified e2e tests compile
16:09:19 [ .. ] go build linux_amd64
16:09:19 [ OK ] go build linux_amd64
16:09:20 [ .. ] Checking that e2e tests compile
16:09:21 [ OK ] Verified e2e tests compile
16:09:21 [ .. ] go build linux_arm64
16:09:21 [ OK ] go build linux_arm64
16:09:22 [ .. ] Checking that e2e tests compile
16:09:23 [ OK ] Verified e2e tests compile
16:09:23 [ .. ] go build linux_arm
16:09:23 [ OK ] go build linux_arm
16:09:24 [ .. ] Checking that e2e tests compile
16:09:25 [ OK ] Verified e2e tests compile
16:09:25 [ .. ] go build linux_ppc64le
16:09:25 [ OK ] go build linux_ppc64le
16:09:26 [ .. ] Checking that e2e tests compile
16:09:27 [ OK ] Verified e2e tests compile
16:09:27 [ .. ] go build darwin_amd64
16:09:27 [ OK ] go build darwin_amd64
16:09:28 [ .. ] Checking that e2e tests compile
16:09:29 [ OK ] Verified e2e tests compile
16:09:29 [ .. ] go build darwin_arm64
16:09:29 [ OK ] go build darwin_arm64
16:09:30 [ .. ] Checking that e2e tests compile
16:09:31 [ OK ] Verified e2e tests compile
16:09:31 [ .. ] go build windows_amd64
16:09:31 [ OK ] go build windows_amd64
16:09:32 [ .. ] helm package crossplane 1.10.0-rc.0.36.g2015c64c.dirty
Successfully packaged chart and saved it to: /Users/adi/src/github.com/crossplane/crossplane/_output/charts/crossplane-1.10.0-rc.0.36.g2015c64c.dirty.tgz
16:09:32 [ OK ] helm package crossplane 1.10.0-rc.0.36.g2015c64c.dirty
16:09:32 [ .. ] helm index
16:09:32 [ OK ] helm index
16:09:32 [ .. ] docker build build-62edaca0/crossplane-amd64
[+] Building 1.5s (9/9) FINISHED
 => [internal] load build definition from Dockerfile                                                                                                                            0.0s
 => => transferring dockerfile: 342B                                                                                                                                            0.0s
 => [internal] load .dockerignore                                                                                                                                               0.0s
 => => transferring context: 2B                                                                                                                                                 0.0s
 => [internal] load metadata for gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                               0.3s
 => [1/4] FROM gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                                                 0.0s
 => [internal] load build context                                                                                                                                               1.1s
 => => transferring context: 45.66MB                                                                                                                                            1.1s
 => CACHED [2/4] ADD bin/linux_amd64/crossplane /usr/local/bin/                                                                                                                 0.0s
 => CACHED [3/4] ADD crds /crds                                                                                                                                                 0.0s
 => CACHED [4/4] ADD webhookconfigurations /webhookconfigurations                                                                                                               0.0s
 => exporting to image                                                                                                                                                          0.0s
 => => exporting layers                                                                                                                                                         0.0s
 => => writing image sha256:94d3af3978046dc9849c940c9536ac5586ff74329d9a9d51d99fb7200349115b                                                                                    0.0s
 => => naming to docker.io/build-62edaca0/crossplane-amd64                                                                                                                      0.0s
16:09:35 [ OK ] docker build build-62edaca0/crossplane-amd64
16:09:36 [ .. ] docker build build-62edaca0/crossplane-arm64
[+] Building 1.2s (9/9) FINISHED
 => [internal] load build definition from Dockerfile                                                                                                                            0.0s
 => => transferring dockerfile: 342B                                                                                                                                            0.0s
 => [internal] load .dockerignore                                                                                                                                               0.0s
 => => transferring context: 2B                                                                                                                                                 0.0s
 => [internal] load metadata for gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                               0.0s
 => [1/4] FROM gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                                                 0.0s
 => [internal] load build context                                                                                                                                               1.1s
 => => transferring context: 44.37MB                                                                                                                                            1.1s
 => CACHED [2/4] ADD bin/linux_arm64/crossplane /usr/local/bin/                                                                                                                 0.0s
 => CACHED [3/4] ADD crds /crds                                                                                                                                                 0.0s
 => CACHED [4/4] ADD webhookconfigurations /webhookconfigurations                                                                                                               0.0s
 => exporting to image                                                                                                                                                          0.0s
 => => exporting layers                                                                                                                                                         0.0s
 => => writing image sha256:262cb1c557e72123dceeb82a53562ec0d617215f7c7b38a88b444494fa255b77                                                                                    0.0s
 => => naming to docker.io/build-62edaca0/crossplane-arm64                                                                                                                      0.0s
16:09:38 [ OK ] docker build build-62edaca0/crossplane-arm64
16:09:38 [ .. ] docker build build-62edaca0/crossplane-arm
[+] Building 1.1s (9/9) FINISHED
 => [internal] load build definition from Dockerfile                                                                                                                            0.0s
 => => transferring dockerfile: 342B                                                                                                                                            0.0s
 => [internal] load .dockerignore                                                                                                                                               0.0s
 => => transferring context: 2B                                                                                                                                                 0.0s
 => [internal] load metadata for gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                               0.0s
 => [1/4] FROM gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                                                 0.0s
 => [internal] load build context                                                                                                                                               1.1s
 => => transferring context: 43.05MB                                                                                                                                            1.1s
 => CACHED [2/4] ADD bin/linux_arm/crossplane /usr/local/bin/                                                                                                                   0.0s
 => CACHED [3/4] ADD crds /crds                                                                                                                                                 0.0s
 => CACHED [4/4] ADD webhookconfigurations /webhookconfigurations                                                                                                               0.0s
 => exporting to image                                                                                                                                                          0.0s
 => => exporting layers                                                                                                                                                         0.0s
 => => writing image sha256:8db179e205ea4daabd3447e52349a4e211a6a10a265d03019f90c2c52d5075ef                                                                                    0.0s
 => => naming to docker.io/build-62edaca0/crossplane-arm                                                                                                                        0.0s
16:09:40 [ OK ] docker build build-62edaca0/crossplane-arm
16:09:40 [ .. ] docker build build-62edaca0/crossplane-ppc64le
[+] Building 1.1s (9/9) FINISHED
 => [internal] load build definition from Dockerfile                                                                                                                            0.0s
 => => transferring dockerfile: 342B                                                                                                                                            0.0s
 => [internal] load .dockerignore                                                                                                                                               0.0s
 => => transferring context: 2B                                                                                                                                                 0.0s
 => [internal] load metadata for gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                               0.0s
 => [internal] load build context                                                                                                                                               1.0s
 => => transferring context: 44.56MB                                                                                                                                            1.0s
 => [1/4] FROM gcr.io/distroless/static@sha256:d2b0ec3141031720cf5eedef3493b8e129bc91935a43b50562fbe5429878d96b                                                                 0.0s
 => CACHED [2/4] ADD bin/linux_ppc64le/crossplane /usr/local/bin/                                                                                                               0.0s
 => CACHED [3/4] ADD crds /crds                                                                                                                                                 0.0s
 => CACHED [4/4] ADD webhookconfigurations /webhookconfigurations                                                                                                               0.0s
 => exporting to image                                                                                                                                                          0.0s
 => => exporting layers                                                                                                                                                         0.0s
 => => writing image sha256:c6f04e99c8494593c841d9e407015862495dbb64774804dff38ac2931cd02520                                                                                    0.0s
 => => naming to docker.io/build-62edaca0/crossplane-ppc64le                                                                                                                    0.0s
16:09:42 [ OK ] docker build build-62edaca0/crossplane-ppc64le
16:09:42 [ OK ] Skipping image build for unsupported platform darwin/amd64
16:09:43 [ OK ] Skipping image build for unsupported platform darwin/arm64
16:09:43 [ OK ] Skipping image build for unsupported platform windows/amd64

CI: crossplane/crossplane#3207

Signed-off-by: Aditya Sharma <[email protected]>

epk · 2022-07-27T23:31:28Z

Tested go targets with all providers in upbound/official-providers and make build.all

Signed-off-by: Chuan-Yen Chiang <[email protected]>

Signed-off-by: Chuan-Yen Chiang <[email protected]> Signed-off-by: Chuan-Yen Chiang <[email protected]>

Go: cleanup

175726e

Signed-off-by: Aditya Sharma <[email protected]>

pull-request-size bot added the size/M label Jul 27, 2022

epk commented Jul 27, 2022

View reviewed changes

epk marked this pull request as ready for review July 27, 2022 18:28

Update gomplate to 3.11.1

e0e4050

Signed-off-by: Aditya Sharma <[email protected]>

epk force-pushed the epk/go1-18-cleanup branch from d91ebad to e0e4050 Compare July 27, 2022 19:39

epk commented Jul 27, 2022

View reviewed changes

epk requested review from AaronME and tnthornton July 27, 2022 20:04

pull-request-size bot added size/L and removed size/M labels Jul 27, 2022

Add new go.modules targets

8acbfc4

Signed-off-by: Aditya Sharma <[email protected]>

epk force-pushed the epk/go1-18-cleanup branch from 64e55b9 to 8acbfc4 Compare July 27, 2022 22:30

tnthornton reviewed Jul 27, 2022

View reviewed changes

epk added 2 commits July 27, 2022 15:56

Ping down versions

b276a37

Signed-off-by: Aditya Sharma <[email protected]>

rm go.distclean

700a1cf

Signed-off-by: Aditya Sharma <[email protected]>

epk mentioned this pull request Jul 27, 2022

Update build submodule crossplane/crossplane#3207

Closed

3 tasks

Keep GOFMT_VERSION same as GO_VERSION

e040ab1

Signed-off-by: Aditya Sharma <[email protected]>

tnthornton self-requested a review July 27, 2022 23:34

tnthornton approved these changes Jul 27, 2022

View reviewed changes

epk merged commit 2c8a896 into master Jul 28, 2022

epk deleted the epk/go1-18-cleanup branch July 28, 2022 00:48

tnthornton assigned epk Jul 28, 2022

epk mentioned this pull request Aug 5, 2022

Cleanup CI + update build submodule to the latest crossplane/crossplane#3220

Closed

3 tasks

epk mentioned this pull request Aug 8, 2022

Better error message for go.modules.tidy.check failures #201

Merged

3 tasks

cychiang added a commit to cychiang/build that referenced this pull request Sep 2, 2022

remove go.distclean target based on upbound#196

24ea6cd

cychiang mentioned this pull request Sep 2, 2022

Remove go.distclean in makelib/golang.mk based on #196 #204

Closed

3 tasks

cychiang added a commit to cychiang/build that referenced this pull request Sep 2, 2022

Remove go.distclean target based on upbound#196

e252f1f

Signed-off-by: Chuan-Yen Chiang <[email protected]>

cychiang added a commit to cychiang/build that referenced this pull request Sep 2, 2022

Remove go.distclean target based on upbound#196

a928473

Signed-off-by: Chuan-Yen Chiang <[email protected]>

cychiang added a commit to cychiang/build that referenced this pull request Sep 2, 2022

Remove go.distclean target based on upbound#196

9f05d8c

Signed-off-by: Chuan-Yen Chiang <[email protected]>

cychiang added a commit to cychiang/build that referenced this pull request Sep 2, 2022

Remove go.distclean target based on upbound#196

6f08d99

Signed-off-by: Chuan-Yen Chiang <[email protected]> Signed-off-by: Chuan-Yen Chiang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: miscellaneous cleanup #196

Go: miscellaneous cleanup #196

epk commented Jul 27, 2022

epk Jul 27, 2022

epk Jul 27, 2022

epk Jul 27, 2022

epk Jul 27, 2022

tnthornton Jul 27, 2022

tnthornton Jul 27, 2022

epk Jul 27, 2022

epk Jul 27, 2022

epk Jul 27, 2022

tnthornton left a comment •

edited

Loading

tnthornton Jul 27, 2022

epk Jul 27, 2022

tnthornton Jul 27, 2022

epk Jul 27, 2022

epk commented Jul 27, 2022

epk commented Jul 27, 2022

		DEP_VERSION=v0.5.1
		DEP := $(TOOLS_HOST_DIR)/dep-$(DEP_VERSION)

Go: miscellaneous cleanup #196

Go: miscellaneous cleanup #196

Conversation

epk commented Jul 27, 2022

Description of your changes

How has this code been tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnthornton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epk commented Jul 27, 2022

crossplane/crossplane:

epk commented Jul 27, 2022

tnthornton left a comment •

edited

Loading