weaveworks / ignite

Ignite a Firecracker microVM
https://ignite.readthedocs.org
Apache License 2.0
3.49k stars 224 forks source link

Cast to uint64 for Darwin platform #860

Closed sc68cal closed 3 years ago

sc68cal commented 3 years ago

I was futzing around with https://github.com/srl-labs/containerlab/ - just trying to build and run the unit tests on OS X to add a feature, and I got dragged down into some dependencies.

stat.Rdev is a int32 on Darwin, so I just casted it to a uint64 to make the compiler happy

https://cs.opensource.google/go/x/sys/+/master:unix/ztypes_darwin_arm64.go;l=73

I'm not proficient with Go, so I don't know if this is wise or not, but I figured I'd submit it back

darkowlzz commented 3 years ago

@sc68cal thanks for submitting the change. Although it's an unnecessary conversion on linux, it's needed to be able to compile on darwin. Since none of the linters have any problem with it and all the e2e tests continue to pass, I'll be happy to merge this change if this helps you work on containerlabs on macOS with ignite as a package dependency. But I would like to get a second opinion on this before we merge it.

sc68cal commented 3 years ago

Perfect. While we're also talking Darwin, I have been scratching my head on this compilation error

/usr/local/opt/go/pkg/mod/github.com/weaveworks/ignite@v0.9.1-0.20210705155449-2dbcdd663727/pkg/network/cni/cni.go:256:14: undefined: ip.TeardownIPMasq

Since this is the last compilation error I have when attempting to compile ignite on Darwin (via containerlab). I dug around a bit but since I'm not as experienced with Golang, I am not sure why it isn't importing the ip package and resolving that method

https://github.com/weaveworks/ignite/blob/main/pkg/network/cni/cni.go#L256

I checked over in containernetworking/plugins and everything seems correct, but I haven't dived further in

darkowlzz commented 3 years ago

@sc68cal hi again, looks like the files in https://github.com/containernetworking/plugins/tree/master/pkg/ip have build constraint for linux and no equivalent files for darwin. Suffix _goos.go (Go OS) is a build constraint. With that, unfortunately, I don't think you can compile ignite on darwin due to the CNI plugin dependency. :( Maybe you can try building it in docker for mac, which uses a linux VM and should be able to help you to at least compile.

stealthybox commented 3 years ago

@sc68cal @networkop Is it possible to build-tag the ignite backend and tests in containerlab for linux, so that it doesn't conflict when folks are running builds on Darwin?

CNI is a linux specific tool, and ignite currently is very linux specific. We could start flagging off linux features with build tags in ignite, but it may require us to shim in some Darwin specific stub interfaces so things will be satisfied downstream.

Flagging off ignite on darwin in containerlab sounds easier to me. WDYT?

sc68cal commented 3 years ago

Flagging off ignite on darwin in containerlab sounds easier to me.

I would be fine with this... although I admit that I don't quite know how to implement that in golang yet.

networkop commented 3 years ago

Hey @stealthybox @sc68cal , I'm happy to update containerlab, only I don't fully understand what needs to be done. Do I need to branch off in clab and select a different sandbox image tag based on the GOOS?

sc68cal commented 3 years ago

I... think we need to add some compiler flags to not build https://github.com/srl-labs/containerlab/tree/master/runtime/ignite when the OS is Darwin

networkop commented 3 years ago

ah yeah, that should be easy

stealthybox commented 3 years ago

If you add // +build linux at the top of any files that are related to the ignite implementation in containerlab, they will only be built when GOOS is linux.

There are also filename suffixes that do this.

See https://dave.cheney.net/tag/build-tags

The only trouble you may run into will be related to callers potentially missing functions when the linux files are disabled. If you run into this problem, you can create stub implementations that invert the build condition for NOT linux: (// +build !linux) and define some empty functions there.

stealthybox commented 3 years ago

Just noting we merged this change, because it solves this issue for multi-OS support in the future, particularly if we support a different hypervisor on darwin ever.

networkop commented 3 years ago

I can add a condition to bail out on darwin in case IGNITE runtime is attempted as well.