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

treewide: Ensure that binaries are built with at least Go 1.17 #17322

Merged
merged 1 commit into from Oct 12, 2021
Merged

treewide: Ensure that binaries are built with at least Go 1.17 #17322

merged 1 commit into from Oct 12, 2021

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Sep 6, 2021

Cilium relies on a number of Go features that are not covered by Go's
backward compatibility guarantee, for example:

  • //go:embed (available since Go 1.16)
  • //go:build (available since Go 1.17)

There may be other cases where, for example, we rely on bug fixes in the
standard library.

Building Cilium with an earlier version of Go may complete without
error, but there is no guarantee that the binary will behave correctly.

This PR adds //go:build tags to the main source file of each of the
principle binaries that will cause compilation to fail early and loudly
so the use of old toolchains can be detected quickly.

Signed-off-by: Tom Payne tom@isovalent.com

@twpayne twpayne added release-note/misc This PR makes changes that have no direct user impact. area/build Anything to do with the build, more general then area/CI labels Sep 6, 2021
@twpayne twpayne requested a review from a team September 6, 2021 13:47
@twpayne twpayne requested review from a team as code owners September 6, 2021 13:47
@twpayne twpayne requested review from a team September 6, 2021 13:47
@twpayne twpayne requested a review from a team as a code owner September 6, 2021 13:47
@twpayne twpayne requested a review from a team September 6, 2021 13:47
@twpayne twpayne requested a review from a team as a code owner September 6, 2021 13:47
@twpayne
Copy link
Contributor Author

twpayne commented Sep 6, 2021

The bug that triggered this PR was I ran make on a clean checkout of master:

$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
$ make
contrib/scripts/check-logging-subsys-field.sh
contrib/scripts/check-fmt.sh
Unformatted Go source code:
./Documentation/_api/v1/models/zz_generated.deepequal.go
diff -u ./Documentation/_api/v1/models/zz_generated.deepequal.go.orig ./Documentation/_api/v1/models/zz_generated.deepequal.go
--- ./Documentation/_api/v1/models/zz_generated.deepequal.go.orig       2021-09-06 15:10:13.546643942 +0200
+++ ./Documentation/_api/v1/models/zz_generated.deepequal.go    2021-09-06 15:10:13.546643942 +0200
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Copyright 2017-2021 Authors of Cilium
./Documentation/_api/v1/models/zz_generated.deepcopy.go
diff -u ./Documentation/_api/v1/models/zz_generated.deepcopy.go.orig ./Documentation/_api/v1/models/zz_generated.deepcopy.go
--- ./Documentation/_api/v1/models/zz_generated.deepcopy.go.orig        2021-09-06 15:10:13.554643998 +0200
+++ ./Documentation/_api/v1/models/zz_generated.deepcopy.go     2021-09-06 15:10:13.554643998 +0200
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Copyright 2017-2021 Authors of Cilium
./Documentation/_api/api/v1/models/zz_generated.deepequal.go
diff -u ./Documentation/_api/api/v1/models/zz_generated.deepequal.go.orig ./Documentation/_api/api/v1/models/zz_generated.deepequal.go
--- ./Documentation/_api/api/v1/models/zz_generated.deepequal.go.orig   2021-09-06 15:10:13.742645311 +0200
+++ ./Documentation/_api/api/v1/models/zz_generated.deepequal.go        2021-09-06 15:10:13.742645311 +0200
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Copyright 2017-2021 Authors of Cilium
./Documentation/_api/api/v1/models/zz_generated.deepcopy.go
diff -u ./Documentation/_api/api/v1/models/zz_generated.deepcopy.go.orig ./Documentation/_api/api/v1/models/zz_generated.deepcopy.go
--- ./Documentation/_api/api/v1/models/zz_generated.deepcopy.go.orig    2021-09-06 15:10:13.750645368 +0200
+++ ./Documentation/_api/api/v1/models/zz_generated.deepcopy.go 2021-09-06 15:10:13.750645368 +0200
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Copyright 2017-2021 Authors of Cilium
make: *** [Makefile:507: precheck] Error 1

Refs #17190 and specifically this comment:

Third commit adds //go:build lines in addition to existing //+build lines, as automatically done by gofmt in Go 1.17. Note that also some files generated by deepcopy-gen were changed. These changes will be lost on the next update until we update to a version of deepcopy-gen which includes kubernetes-sigs/controller-tools#595.

@twpayne
Copy link
Contributor Author

twpayne commented Sep 6, 2021

Note that these early checks should also help shake out cases where out test infrastructure is using an older version of Go.

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Would be nice to update the make update-go-version target as well to update these //go:build lines based on the Go version specified in GO_VERSION.

bugtool/main.go Show resolved Hide resolved
@tklauser
Copy link
Member

tklauser commented Sep 6, 2021

Refs #17190 and specifically this comment:

Third commit adds //go:build lines in addition to existing //+build lines, as automatically done by gofmt in Go 1.17. Note that also some files generated by deepcopy-gen were changed. These changes will be lost on the next update until we update to a version of deepcopy-gen which includes kubernetes-sigs/controller-tools#595.

FWIW, christarazi/controller-tools#1 should take care of updating our forked version of controller-tools to generate //go:build lines.

@twpayne twpayne requested a review from a team as a code owner September 6, 2021 17:19
@twpayne
Copy link
Contributor Author

twpayne commented Sep 6, 2021

Would be nice to update the make update-go-version target as well to update these //go:build lines based on the Go version specified in GO_VERSION.

Great idea, done.

Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Looks pretty good, one comment on making the discovery of main.go files more dynamic. Not sure if it over-searches tho.

Makefile Show resolved Hide resolved
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, pending Louis's suggestion.

@twpayne
Copy link
Contributor Author

twpayne commented Sep 9, 2021

ConformanceKind1.19 failure is #16938.

@twpayne
Copy link
Contributor Author

twpayne commented Sep 9, 2021

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sLRPTests Checks local redirect policy LRP connectivity

Failure Output

FAIL: Expected

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests LoadBalancer Connectivity to endpoint via LB

Failure Output

FAIL: Can not connect to service "http://192.168.1.146" from outside cluster (1/10)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

sed -i is not portable, but it seems we already use it in several places so let it be.

@twpayne
Copy link
Contributor Author

twpayne commented Sep 13, 2021

test-me-please

@twpayne
Copy link
Contributor Author

twpayne commented Sep 14, 2021

This needs #17391 and a rebase before the tests will pass as the current Vagrant box images have Go 1.16.

@twpayne twpayne marked this pull request as draft September 14, 2021 14:41
@twpayne
Copy link
Contributor Author

twpayne commented Sep 14, 2021

Correction: #17352 #17394 is needed.

Cilium relies on a number of Go features that are not covered by Go's
backward compatibility guarantee, for example:

* //go:embed (available since Go 1.16)
* //go:build (available since Go 1.17)

There may be other cases where, for example, we rely on bug fixes in the
standard library.

Building Cilium with an earlier version of Go may complete without
error, but there is no guarantee that the binary will behave correctly.

This PR adds //go:build tags to the main source file of each of the
principle binaries that will cause compilation to fail early and loudly
so the use of old toolchains can be detected quickly.

Signed-off-by: Tom Payne <tom@isovalent.com>
@twpayne twpayne marked this pull request as ready for review October 12, 2021 08:15
@twpayne
Copy link
Contributor Author

twpayne commented Oct 12, 2021

/test

@twpayne twpayne added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 12, 2021
@errordeveloper errordeveloper merged commit 726b4fb into cilium:master Oct 12, 2021
@twpayne twpayne deleted the pr/twpayne/force-go1.17 branch October 13, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Anything to do with the build, more general then area/CI ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants