Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

Go: Clamp down minimum required version to 1.18 #195

Merged
merged 5 commits into from
Jul 27, 2022
Merged

Go: Clamp down minimum required version to 1.18 #195

merged 5 commits into from
Jul 27, 2022

Conversation

epk
Copy link
Contributor

@epk epk commented Jul 21, 2022

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

Description of your changes

  • Enforce/Require usage of Go 1.18
  • Drops usage of -i flag for go invocations

    Why?

    The -i flag installs the packages that are dependencies of the target.
    The -i flag is deprecated. Compiled packages are cached automatically.
    

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

Signed-off-by: Aditya Sharma <[email protected]>
@epk epk changed the title Go: drop usage of -I flag Go: drop usage of -i flag Jul 21, 2022
@epk epk changed the title Go: drop usage of -i flag Go: 1.18 fixes Jul 21, 2022
@epk epk force-pushed the epk/go1-18 branch 2 times, most recently from 8a877ef to fcd8424 Compare July 21, 2022 18:57
@epk epk changed the title Go: 1.18 fixes Go: Clamp down minimum required version to 1.18 Jul 21, 2022
epk added 2 commits July 21, 2022 12:01
Signed-off-by: Aditya Sharma <[email protected]>
Signed-off-by: Aditya Sharma <[email protected]>
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

I cannot remember the exact scenario, but I have a vague memory recently @epk where some of our tooling, or code gen, or something wasn't compatible with go 1.18 yet. Have you been able to use these changes to build a few different projects that consume this build submodule?

@epk
Copy link
Contributor Author

epk commented Jul 21, 2022

I cannot remember the exact scenario, but I have a vague memory recently @epk where some of our tooling, or code gen, or something wasn't compatible with go 1.18 yet. Have you been able to use these changes to build a few different projects that consume this build submodule?

I have tried some upbound repos in: https://github.com/upbound/squad-prodeng/issues/932 and make all + few other common make commands work fine

@muvaf
Copy link
Contributor

muvaf commented Jul 22, 2022

@epk Could you test updating crossplane-runtime and crossplane? I remember some k8s libraries we use weren't compatible with go 1.18

@epk
Copy link
Contributor Author

epk commented Jul 22, 2022

@epk Could you test updating crossplane-runtime and crossplane? I remember some k8s libraries we use weren't compatible with go 1.18

Opened PRs for both:

@epk
Copy link
Contributor Author

epk commented Jul 25, 2022

@muvaf @jbw976 can we merge this?

Signed-off-by: Aditya Sharma <[email protected]>
@@ -84,10 +84,14 @@ GOIMPORTS := $(TOOLS_HOST_DIR)/goimports
GO := go
GOHOST := GOOS=$(GOHOSTOS) GOARCH=$(GOHOSTARCH) go
GO_VERSION := $(shell $(GO) version | sed -ne 's/[^0-9]*\(\([0-9]\.\)\{0,4\}[0-9][^.]\).*/\1/p')
GO_REQUIRED_VERSION := 1.18
ifneq ($(GO_VERSION),$(GO_REQUIRED_VERSION))
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have something like this but the friction it creates turned out to be more than the benefits we're getting for streamlining. So, I'd opt to let individual projects impose this requirement if they'd like to in their Makefiles.

Copy link
Contributor Author

@epk epk Jul 26, 2022

Choose a reason for hiding this comment

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

Made GO_REQUIRED_VERSION customizable in 09af1ab

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 26, 2022
Copy link
Contributor

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Thanks @epk !

@muvaf muvaf merged commit 87db883 into master Jul 27, 2022
@muvaf muvaf deleted the epk/go1-18 branch July 27, 2022 08:07
@muvaf
Copy link
Contributor

muvaf commented Aug 1, 2022

@epk I have different projects using different Go versions and I went ahead and added this override for each of them, but now I have to have a different Go binary for each version. It's possible to set up but I'm questioning the value we get here with this requirement given that Go is perfectly fine building projects with different versions and it errors out when the compatibility is broken.

Do you feel strongly about this? Is there a value I'm not seeing other than having the locally built binaries same as production? I'd like to revert it if not.

@epk
Copy link
Contributor Author

epk commented Aug 1, 2022

@epk I have different projects using different Go versions and I went ahead and added this override for each of them, but now I have to have a different Go binary for each version. It's possible to set up but I'm questioning the value we get here with this requirement given that Go is perfectly fine building projects with different versions and it errors out when the compatibility is broken.

Do you feel strongly about this? Is there a value I'm not seeing other than having the locally built binaries same as production? I'd like to revert it if not.

I have been using https://github.com/travis-ci/gimme to automatically load the go versions as required.

I'll comment back tomorrow but several upstream K8s dependencies also now strictly require 1.18 because of the usage of any.

I'd rather try to get everything to 1.18 and then change the version check to support two versions like rook's build libraries: https://github.com/rook/rook/blob/8df00d7dd10cf784842c04cbabceabe9cacfcf7c/build/makelib/golang.mk#L105

@muvaf
Copy link
Contributor

muvaf commented Aug 1, 2022

I'll comment back tomorrow but several upstream K8s dependencies also now strictly require 1.18 because of the usage of any.

FWIW, when Go 1.17 sees any it returns error just like the one we have here anyway, so it already forces you to upgrade. The thing is the whole transition cost will be needed to be paid for the next versions as well since we're forcing a specific version even though it may not really be required by the toolchain and gimme helps but it's yet another tool developers need to know about. All of it can be worked out and let's keep it if you feel strongly about it, but I just want us to consider what we're getting in return. Is it reproducibility or implicit push for owners to upgrade to the latest version or something else?

@turkenh
Copy link
Member

turkenh commented Nov 2, 2022

This check is causing trouble and confusion since it is being invoked even if you don't call a target building go code.
See: upbound/official-providers-ci#39 (comment)

@muvaf muvaf mentioned this pull request Nov 5, 2022
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants