Add go:build tags #3185

kolyshkin · 2021-08-27T00:45:54Z

*: rm redundant linux build tag

For files that end with _linux.go or _linux_test.go, there is no need to
specify linux build tag, as it is assumed from the file name.

In addition, rename libcontainer/notify_linux_v2.go -> libcontainer/notify_v2_linux.go
for the file name to make sense.
Rm build tags from some packages

The linux build tag to main pkg was added by commit 5aa82c9
back in the day when we thought
runc is going to be cross-platform. It's very clear now it's Linux-only
package.

Also, there are some packages that can't currently be compiled on
non-linux anyway. Remove linux build tag (and _unsupported.go files)
from those as well.

While at it, further clarify it in README that we're Linux only.
*: add go-1.17+ go:build tags

Go 1.17 introduce this new (and better) way to specify build tags.
For more info, see https://golang.org/design/draft-gobuild.

As a way to seamlessly switch from old to new build tags, gofmt (and
gopls) from go 1.17 adds the new tags along with the old ones.

Later, when go < 1.17 is no longer supported, the old build tags
can be removed.

Now, as I started to use latest gopls (v0.7.1), it adds these tags
while I edit. Rather than to randomly add new build tags, I guess
it is better to do it once for all files.

Brought to you by
```
     go1.17 fmt ./...
```

AkihiroSuda · 2021-08-27T04:55:17Z

checkpoint.go

@@ -1,3 +1,4 @@
+//go:build linux


Why not just rename them to _linux.go
(We will lose git log / git blame contexts, though)

Or maybe just say "runc is a Linux-only project. Don't import any package from this repo if your app needs to be portable" and remove all linux tags

I think for some packages in libcontainer, it makes sense to keep build-tags (some provide stubs for other platforms)

For files at the root, I guess it would be ok, but not sure if it complicates linting the code on non-linux 🤔

I was thinking about that, but people will inevitably export some of the libcontainer packages into their code. This can be partially fixed by implementing #3028; alas it's currently stuck waiting for more input from maintainers :(

I would not like all files having _linux suffix either, as it will clutter directory listing.

I agree with removal linux build tags from the main package -- DONE

AkihiroSuda · 2021-08-27T04:55:43Z

delete.go

@@ -1,3 +1,4 @@
+//go:build !solaris
 // +build !solaris


I don't think we still support solaris

Yes, I think we can remove these. At least nobody is testing it, so should be good to remove

Nice catch :) Removed

thaJeztah · 2021-08-27T13:40:52Z

libcontainer/container_linux.go

@@ -1,5 +1,3 @@
-// +build linux


I recall doing something similar a while back. I'm wondering if there's a linter for this, or perhaps if this is something that gofmpt could do 🤔

Would be nice to add a linter for that, if such linter exists.

Was not able to find a tool to fix these, so ended up with some script.

thaJeztah · 2021-08-27T13:44:13Z

libcontainer/configs/namespaces_syscall_unsupported.go

+//go:build !linux && !windows
 // +build !linux,!windows


I was discussing this yesterday, do we know if gofmt checks if the "old style" and "new style" build-tags match? If not, wondering if there's a linter for that (as there's a potential risk that build-tags for < go 1.16 and go 1.17+ are different.

(Just a comment / thinking out loud)

gopls does it (and shows a warning in my vi).

go:build design doc (linked to in description and commit msg) says

The buildtags check in go vet will add support for //go:build constraints. It will fail when a Go source file contains //go:build and // +build lines with different meanings. If the check fails, one can run gofmt -w.

and yes indeed

[kir@kir-rhat runc]$ git diff diff --git a/libcontainer/cgroups/cgroups_test.go b/libcontainer/cgroups/cgroups_test.go index 56f6b045..417a91ff 100644 --- a/libcontainer/cgroups/cgroups_test.go +++ b/libcontainer/cgroups/cgroups_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || freebsd // +build linux package cgroups [kir@kir-rhat runc]$ go1.17 vet ./... # github.com/opencontainers/runc/libcontainer/cgroups libcontainer/cgroups/cgroups_test.go:2:1: +build lines do not match //go:build condition [kir@kir-rhat runc]$ golangci-lint run ./... libcontainer/cgroups/cgroups_test.go:2:1: buildtag: +build lines do not match //go:build condition (govet) // +build linux ^

Ah, perfect! I wanted to check but didn't find the time to do so 👍

thaJeztah · 2021-08-27T13:45:02Z

delete.go

@@ -1,3 +1,4 @@
+//go:build !solaris
 // +build !solaris


Yes, I think we can remove these. At least nobody is testing it, so should be good to remove

thaJeztah · 2021-08-27T13:47:05Z

checkpoint.go

@@ -1,3 +1,4 @@
+//go:build linux


I think for some packages in libcontainer, it makes sense to keep build-tags (some provide stubs for other platforms)

For files at the root, I guess it would be ok, but not sure if it complicates linting the code on non-linux 🤔

thaJeztah

LGTM

kolyshkin · 2021-08-31T03:14:43Z

OK, it seems we can remove +build linux from packages which currently can not be compiled on non-linux platforms.

For example, libcontainer/cgroups can't be compiled now:

[kir@kir-rhat cgroups]$ GOOS=freebsd go build ./...
go build github.com/opencontainers/runc/libcontainer/cgroups/devices: build constraints exclude all Go files in /home/kir/go/src/github.com/opencontainers/runc/libcontainer/cgroups/devices
# github.com/opencontainers/runc/libcontainer/cgroups
./file.go:81:14: undefined: unix.Openat2
./file.go:81:45: undefined: unix.OpenHow
./file.go:102:18: undefined: unix.RESOLVE_BENEATH
./file.go:102:41: undefined: unix.RESOLVE_NO_MAGICLINKS
./file.go:103:17: undefined: unix.CGROUP2_SUPER_MAGIC
./file.go:105:20: undefined: unix.RESOLVE_NO_XDEV
./file.go:105:43: undefined: unix.RESOLVE_NO_SYMLINKS
./v1_utils.go:152:9: undefined: Mount
./v1_utils.go:160:87: undefined: Mount
./v1_utils.go:188:37: undefined: Mount
./file.go:105:43: too many errors
# github.com/opencontainers/runc/libcontainer/utils
../utils/utils.go:127:31: undefined: unix.O_PATH
../utils/utils_unix.go:19:17: undefined: unix.PROC_SUPER_MAGIC
# github.com/godbus/dbus/v5
../../vendor/github.com/godbus/dbus/v5/transport_unix.go:52:3: cannot use t (type *unixTransport) as type transport in return argument:
	*unixTransport does not implement transport (missing SendNullByte method)
../../vendor/github.com/godbus/dbus/v5/transport_unix.go:58:3: cannot use t (type *unixTransport) as type transport in return argument:
	*unixTransport does not implement transport (missing SendNullByte method)

so we can just strip all // +build linux tags from it.

Let me rework it so we can remove tags from most packages.

For files that end with _linux.go or _linux_test.go, there is no need to specify linux build tag, as it is assumed from the file name. In addition, rename libcontainer/notify_linux_v2.go -> libcontainer/notify_v2_linux.go for the file name to make sense. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This was added by commit 5aa82c9 back in the day when we thought runc is going to be cross-platform. It's very clear now it's Linux-only package. While at it, further clarify it in README that we're Linux only. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Only some libcontainer packages can be built on non-linux platforms (not that it make sense, but at least go build succeeds). Let's call these "good" packages. For all other packages (i.e. ones that fail to build with GOOS other than linux), it does not make sense to have linux build tag (as they are broken already, and thus are not and can not be used on anything other than Linux). Remove linux build tag for all non-"good" packages. This was mostly done by the following script, with just a few manual fixes on top. function list_good_pkgs() { for pkg in $(find . -type d -print); do GOOS=freebsd go build $pkg 2>/dev/null \ && GOOS=solaris go build $pkg 2>/dev/null \ && echo $pkg done | sed -e 's|^./||' | tr '\n' '|' | sed -e 's/|$//' } function remove_tag() { sed -i -e '\|^// +build linux$|d' $1 go fmt $1 } SKIP="^("$(list_good_pkgs)")" for f in $(git ls-files . | grep .go$); do if echo $f | grep -qE "$SKIP"; then echo skip $f continue fi echo proc $f remove_tag $f done Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

These are not needed as these packages (libcontainer/cgroups, libcontainer/cgroups/fs, and libcontainer/cgroups/systemd) can not be built under non-linux anyway (for various reasons). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Go 1.17 introduce this new (and better) way to specify build tags. For more info, see https://golang.org/design/draft-gobuild. As a way to seamlessly switch from old to new build tags, gofmt (and gopls) from go 1.17 adds the new tags along with the old ones. Later, when go < 1.17 is no longer supported, the old build tags can be removed. Now, as I started to use latest gopls (v0.7.1), it adds these tags while I edit. Rather than to randomly add new build tags, I guess it is better to do it once for all files. Mind that previous commits removed some tags that were useless, so this one only touches packages that can at least be built on non-linux. Brought to you by go1.17 fmt ./... Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-08-31T04:03:58Z

OK, more commits but hopefully it makes more sense. I have removed // +build linux from those packages that can not be built on non-linux anyway (so removal does not really break anything).

The only packages that can be compiled (tried GOOS=solaris and GOOS=freebsd) are (under github.com/opencontainers/runc/libcontainer):

system
userns
cgroups/devices
user
configs
devices
integration
nsenter
seccomp
seccomp/patchbpf
apparmor
logs
capabilities

These may or may not be used by some software built not just for linux, so let's keep them that way (doesn't make much sense but at least we're not breaking anything).

From now on, I guess, using linux build tag (or _linux.go suffix) should be discouraged, as runc is linux-only software.

thaJeztah

LGTM

kolyshkin added the kind/refactor refactoring label Aug 27, 2021

kolyshkin changed the title ~~Go117 build tags~~ Add go:build tags Aug 27, 2021

kolyshkin requested review from AkihiroSuda and thaJeztah August 27, 2021 00:46

AkihiroSuda reviewed Aug 27, 2021

View reviewed changes

thaJeztah reviewed Aug 27, 2021

View reviewed changes

kolyshkin force-pushed the go117-build-tags branch 2 times, most recently from 463f0df to 6a2cf11 Compare August 27, 2021 15:48

thaJeztah previously approved these changes Aug 27, 2021

View reviewed changes

kolyshkin added this to the 1.1.0 milestone Aug 31, 2021

kolyshkin marked this pull request as draft August 31, 2021 03:02

kolyshkin added 5 commits August 30, 2021 20:15

libct/cg: rm "unsupported.go" files

1b17ec9

These are not needed as these packages (libcontainer/cgroups, libcontainer/cgroups/fs, and libcontainer/cgroups/systemd) can not be built under non-linux anyway (for various reasons). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin dismissed thaJeztah’s stale review via d8da003 August 31, 2021 03:59

kolyshkin force-pushed the go117-build-tags branch from 6a2cf11 to d8da003 Compare August 31, 2021 03:59

kolyshkin marked this pull request as ready for review August 31, 2021 04:00

kolyshkin requested review from AkihiroSuda and thaJeztah August 31, 2021 04:04

thaJeztah approved these changes Aug 31, 2021

View reviewed changes

AkihiroSuda approved these changes Sep 2, 2021

View reviewed changes

AkihiroSuda merged commit 5fb9b2a into opencontainers:master Sep 2, 2021

kolyshkin mentioned this pull request Nov 19, 2021

stats: modernize stats collection and implement Pod level stats reporting cri-o/cri-o#5427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add go:build tags #3185

Add go:build tags #3185

kolyshkin commented Aug 27, 2021 •

edited

Loading

AkihiroSuda Aug 27, 2021

AkihiroSuda Aug 27, 2021

thaJeztah Aug 27, 2021

kolyshkin Aug 27, 2021

AkihiroSuda Aug 27, 2021

thaJeztah Aug 27, 2021

kolyshkin Aug 27, 2021

thaJeztah Aug 27, 2021

kolyshkin Aug 27, 2021

thaJeztah Aug 27, 2021

kolyshkin Aug 27, 2021

thaJeztah Aug 27, 2021

thaJeztah Aug 27, 2021

thaJeztah Aug 27, 2021

thaJeztah left a comment

kolyshkin commented Aug 31, 2021

kolyshkin commented Aug 31, 2021

thaJeztah left a comment

Add go:build tags #3185

Add go:build tags #3185

Conversation

kolyshkin commented Aug 27, 2021 • 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

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

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 31, 2021

kolyshkin commented Aug 31, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 27, 2021 •

edited

Loading