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

add windows CI #1650

Merged
merged 11 commits into from
Jan 27, 2025
Merged

add windows CI #1650

merged 11 commits into from
Jan 27, 2025

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Jan 21, 2025

This is a precursor to further Windows work, mostly stubbing out things that won't ever work on Windows, or for which there isn't a plan yet.

examples: add linux build tag

All of our examples will only work on Linux, some of them will only compile
on Unix-like systems due to depending on x/sys/unix. Add a linux build tag
to them.

Signed-off-by: Lorenz Bauer <[email protected]>

features: add linux build tag

Signed-off-by: Lorenz Bauer <[email protected]>

perf, ringbuf, epoll: add linux build tag

Signed-off-by: Lorenz Bauer <[email protected]>

cmd/bpf2go: disable on Windows

Signed-off-by: Lorenz Bauer <[email protected]>

unix: return ErrNotSupportedOnOS for non-Linux platforms

Change the dummy unix package to return a sentinel error on non-Linux 
platforms. This means that we don't need to add lots of runtime.GOOS checks
further up the call stack.

Signed-off-by: Lorenz Bauer <[email protected]>

internal/tracefs: skip some tests on non-Linux platforms

Signed-off-by: Lorenz Bauer <[email protected]>

internal/kallsyms: skip some tests on non-Linux platforms

Signed-off-by: Lorenz Bauer <[email protected]>

internal/linux: skip some tests on non-Linux platforms

Skip most tests in the package, with the exception of the vdso tests that
use testdata. For those we need to make the string manipulation functions in
package unix work on Windows.

Signed-off-by: Lorenz Bauer <[email protected]>

CI: add windows job

Add a windows job wich installs a debug version of eBPF for Windows, the go
toolchain and gotestsum.

For some reason, drive C: is exceedingly slow on the hosted Windows runners.
Moving cache and temporary directories cuts down the build time of gotestsum
from 2 minutes to 20ish seconds.

For now we only test packages which already pass.

Signed-off-by: Lorenz Bauer <[email protected]>

CI: use latest main build of efW

Use the latest successful build of the main branch of eBPF for Windows. This
makes more sense than using tagged versions since most of the changes we
need are yet to be upstreamed.

Staying on a tagged release would mean waiting six weeks for a new release,
on average.

Signed-off-by: Lorenz Bauer <[email protected]>

internal: add OnLinux constant

Add a constant which indicates whether we are building for Linux. This is so
that we can write

	if !internal.OnLinux {

instead of

	if runtime.GOOS != "linux" {

This makes it easy to remove platform dependent code by removing the 
constant and then fixing the breakage.

Signed-off-by: Lorenz Bauer <[email protected]>

@lmb lmb force-pushed the windows-ci branch 2 times, most recently from 818dfd5 to 86e158f Compare January 21, 2025 10:35
@lmb lmb marked this pull request as ready for review January 21, 2025 10:40
@lmb lmb requested review from ti-mo, mejedi, florianl, rgo3 and a team as code owners January 21, 2025 10:40
@lmb lmb force-pushed the windows-ci branch 2 times, most recently from 1c02f7b to 9006c76 Compare January 21, 2025 15:38
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Left a few small suggestions, but I'd like to bring up the topic of build tags. Having led the effort of removing build tags from cilium/cilium, I know what a pain they can be and how they quickly lead to a slippery slope. I know editor tooling caught up and all, but having a mix of build tags and individual test skips will lead to silently skipping tests you're expecting to run on Windows and general raising of eyebrows when debugging things in a pinch.

On the other hand, marking each and every test we have with a Linux helper would be exceedingly tedious. I lack the energy to reason about a better solution at the moment, but it's an exercise we should do before committing to this approach, because the cleanup effort afterwards will require heroics, if that day comes.

@@ -113,6 +113,10 @@ func sanitizeTracefsPath(path ...string) (string, error) {
// but may be also be available at /sys/kernel/debug/tracing if debugfs is mounted.
// The available tracefs paths will depends on distribution choices.
var getTracefsPath = sync.OnceValues(func() (string, error) {
if runtime.GOOS != "linux" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this helper would make sense:

package internal/linux

func Feature(feat string) error {
	if runtime.GOOS == "linux" {
		return nil
	}
	return fmt.Errorf("%s: %w", feat, internal.ErrNotSupportedOnOS)
}

used like:

if err := linux.Feature("tracefs"); err != nil {
	return "", err
}

That would reduce the sprawl of open coding runtime.GOOS != "linux", since the majority of cases (for now) will check if running on Linux specifically. Also no need for wrapping the error, and all calls will be easily discoverable using reference search. I'm open to other ideas and bike shedding, of course. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is clearer than just open coding if runtime.GOOS != ... in the code base. It's not shorter and also not easier to discover, since one can always find references of runtime.GOOS or grep for the condition. Idle thought: I could add a constant like this https://go.dev/play/p/_JOiIsEduDo if you really dislike the open coding.

if internal.IsWindows { ... }

I like the idea of having something like this in testutils though: testutils.LinuxOnly(reason) or similar. That forces me to document why a test was stubbed out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

P.S. the bits where runtime.GOOS == "windows" come later :P

internal/kallsyms/kallsyms_test.go Outdated Show resolved Hide resolved
@lmb
Copy link
Collaborator Author

lmb commented Jan 24, 2025

Having led the effort of removing build tags from cilium/cilium, I know what a pain they can be and how they quickly lead to a slippery slope.

I think you're referring to the integration_tests build tag? In my opinion there is a difference between that and what I am doing here: the GOOS build tag is always set by the toolchain. So whether you're developing on Windows or on Linux, you always get exactly one configuration. In cilium/cilium this wasn't the case.

To be honest I don't think it's possible to develop cross-platform without using build tags in some fashion.

having a mix of build tags and individual test skips will lead to silently skipping tests you're expecting to run on Windows

You're worried that we'll end up disabling too many tests on Windows? Probably a fair point, I can do another pass over the stubbed out packages and see whether they do make sense after all. Not sure we can do better than "pay attention at the outset".

[...] reason about a better solution at the moment, but it's an exercise we should do before committing to this approach

Maybe the problem is that this PR only contains half of my approach:

  1. Packages that wont exist on Windows (for now) or tests that will never work on the platform are skipped based on runtime.GOOS (if there are other tests that do make sense to run) or build tags (to avoid adding a million runtime.GOOS conditions).
  2. Packages that will exist on Windows but which are not fully implemented, for example package btf. We need the btf reader but loading is not supported. The way this will work is that the new ErrNotSupportedOnOS sentinel is returned on Windows, and tests are updated to call testutils.SkipIfNotSupportedOnOS.

I've only included part 1 here, maybe that's why this seems a bit ad hoc.

@lmb
Copy link
Collaborator Author

lmb commented Jan 24, 2025

Okay, I made two bigger changes:

  1. Made functions in internal/unix return ErrNotSupportedOnOS by default. This will save adding a lot of if runtime.GOOS checks later on. See d9aada0
  2. Removed all build tags from tests and added SkipIfNotSupported instead. This adds one line to most tests but has the benefit that tests start executing once a particular "not supportedness" is fixed. That in turn will help in making the API consistent.

I also added the OnLinux constant as the top commit, so I can remove it easily if it's not worth it.

Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

I like OnLinux.

Let's make sure we're not adding more build tags than necessary, though. Notably in bpf2go, examples, perf.

@@ -1,3 +1,5 @@
//go:build !windows
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like cherry-picking packages in go test in .github/workflows/ci.yml. I think that means these !windows tags can be dropped for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed via dm: the goal is to make go test ./... pass, so stubbing these out for now makes sense. I can't add them to ci.yml since the toolchain returns an error when an explictly mentioned package doesn't contain test files.

cmd/bpf2go/test/doc.go Show resolved Hide resolved
internal/unix/error.go Show resolved Hide resolved
lmb added 11 commits January 27, 2025 13:42
All of our examples will only work on Linux, some of them will only
compile on Unix-like systems due to depending on x/sys/unix.
Add a linux build tag to them.

Signed-off-by: Lorenz Bauer <[email protected]>
Signed-off-by: Lorenz Bauer <[email protected]>
Change the dummy unix package to return a sentinel error on non-Linux
platforms. This means that we don't need to add lots of runtime.GOOS
checks further up the call stack.

Signed-off-by: Lorenz Bauer <[email protected]>
Skip most tests in the package, with the exception of the vdso
tests that use testdata. For those we need to make the string
manipulation functions in package unix work on Windows.

Signed-off-by: Lorenz Bauer <[email protected]>
Add a windows job wich installs a debug version of eBPF for Windows,
the go toolchain and gotestsum.

For some reason, drive C: is exceedingly slow on the hosted Windows
runners. Moving cache and temporary directories cuts down the build
time of gotestsum from 2 minutes to 20ish seconds.

For now we only test packages which already pass.

Signed-off-by: Lorenz Bauer <[email protected]>
Use the latest successful build of the main branch of eBPF for
Windows. This makes more sense than using tagged versions since
most of the changes we need are yet to be upstreamed.

Staying on a tagged release would mean waiting six weeks for a
new release, on average.

Signed-off-by: Lorenz Bauer <[email protected]>
Add a constant which indicates whether we are building for Linux.
This is so that we can write

	if !internal.OnLinux {

instead of

	if runtime.GOOS != "linux" {

This makes it easy to remove platform dependent code by removing the
constant and then fixing the breakage.

Signed-off-by: Lorenz Bauer <[email protected]>
@lmb lmb requested a review from ti-mo January 27, 2025 13:45
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

After discussing offline, we've decided to go with the current state of the PR.

Packages like bpf2go don't currently work due to pathsep issues and requiring llvm to be present on the machine. Excluding all files in a package so go test ./... can be run doesn't work, the Go toolchain doesn't allow that. This will do for now!

@ti-mo ti-mo merged commit 9f20115 into cilium:main Jan 27, 2025
18 checks passed
@lmb lmb deleted the windows-ci branch January 27, 2025 14:16
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.

5 participants