Adding build constraints #1340

helsaawy · 2022-03-30T19:31:20Z

Adding windows build constraints to code to allow tests and benchmarks to be run on Linux, and to limit compiler/linter/language server errors in general about code base.

Now, go test ./.. will successfully run on both Windows and Linux, and only run the appropriate tests.

Added doc.go to modules (with doc string, where appropriate) to prevent compiler/linter errors about broken imports.
(Ie, if an entire package is only available on one OS, then its import statements will error on the other OS since the compiler/language server cannot find any code to satisfy the import.)
In some cases (ie, winapi and wclayer), the package already had an OS-agnostic file of the same name, along with a doc string. A doc.go file was added to preempt situations where windows-specific code is added to that file in the future.

go-winio/pkg/guid was updated to be OS-agnostic, but until the version is updated in go.mod, then cow/cow.go, gcs/protocol.gcs, hcs/schema1/schema1.go, wclayer/wclayer.go, and winapi/devices.go are restricted to beings Windows specific.

Renamed files in test\cri-containerd to end with _test.go so they can include variables and functions defined in other _test.go. For example, gmsa.go imports gmsaAccount, which is defined in main_test.go.

PR also fixes minor linter issues with extra whitespace and misspelled words.

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

Adding windows build constraints to code to allow tests and benchmarks to be run on Linux. Added doc.go to modules (with doc string, where appropriate) to prevent compiler/linter errors about broken imports. In some cases (ie, winapi and wclayer), the package already had an OS-agnostic file of the same name, along with a doc string. A doc.go file was added to preempt situations where windows-specific code is added to that file in the future. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

dcantah · 2022-03-30T20:52:46Z

My hero 😆. Whenever this is out of draft please assign me.

Renaming files in `test\cri-containerd` to end with `_test.go` so they can include variables and functions defined in other `_test.go`. For example, `gmsa.go` imports `gmsaAccount`, which is defined in `main_test.go`. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

dcantah · 2022-04-04T22:23:38Z

This is going to take a while :)

katiewasnothere

LGTM

cmd/containerd-shim-runhcs-v1/events.go

test/cri-containerd/test-images/jobcontainer_createvhd/main.go

dcantah · 2022-04-04T23:16:23Z

hcn/doc.go

@@ -0,0 +1,3 @@
+// Package hcn is a shim for the Host Compute Networking (HCN) service, which manages networking for Windows Server
+// containers and Hyper-V containers. Previous to RS5, HCN was referred to as Host Networking Service (HNS).


Nit: Prior*

internal/clone/doc.go

internal/cni/doc.go

dcantah · 2022-04-04T23:46:41Z

cmd/ncproxy/ncproxy.go

@@ -1,3 +1,5 @@
+//go:build windows


[Not blocking this pr in anyway] I can only comment on changed lines, but @katiewasnothere it'd be worth splitting the hcn networking from the ncproxynetworking implementations into _windows and _linux files most likely (at least that's what my memory is telling me the github.com/Microsoft/hcsshim/internal/ncproxy/networking definitions were for).

We also use winio in this file as well for DialPipe, which I guess on Linux we could use a Unix domain socket instead.

dcantah

lgtm. Was it just go test./.. that you wanted to unlock with this (or future work)? I think our linux specific tests just amounted to things in /internal/guest (which we run in the build_gcs CI step), unless I'm forgetting some

helsaawy · 2022-04-05T05:07:00Z

lgtm. Was it just go test./.. that you wanted to unlock with this (or future work)? I think our linux specific tests just amounted to things in /internal/guest (which we run in the build_gcs CI step), unless I'm forgetting some

There's actually a bunch of tests in .\ext4\internal\compactext4 that seem to consistently fail.
But it was go test ./... and getting the test/guest directories to not error and raise issues from the linter

Related work items: #1302, #1325, #1331, #1338, #1340, #1346, #1348

This reverts commit 949e46a. Signed-off-by: Daniel Canter <dcanter@microsoft.com>

* Adding build constraints Adding windows build constraints to code to allow tests and benchmarks to be run on Linux. Added doc.go to modules (with doc string, where appropriate) to prevent compiler/linter errors about broken imports. In some cases (ie, winapi and wclayer), the package already had an OS-agnostic file of the same name, along with a doc string. A doc.go file was added to preempt situations where windows-specific code is added to that file in the future. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * Renaming test files Renaming files in `test\cri-containerd` to end with `_test.go` so they can include variables and functions defined in other `_test.go`. For example, `gmsa.go` imports `gmsaAccount`, which is defined in `main_test.go`. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

helsaawy requested a review from a team as a code owner March 30, 2022 19:31

helsaawy marked this pull request as draft March 30, 2022 19:33

helsaawy force-pushed the he/lcowbench branch from 985661b to f371686 Compare March 30, 2022 20:16

helsaawy force-pushed the he/lcowbench branch from f371686 to e40cd44 Compare March 30, 2022 20:38

helsaawy force-pushed the he/lcowbench branch from e78f4d6 to 2824a0b Compare March 30, 2022 21:03

helsaawy marked this pull request as ready for review March 30, 2022 21:10

helsaawy assigned dcantah Mar 30, 2022

helsaawy force-pushed the he/lcowbench branch from 2824a0b to fd2389b Compare March 30, 2022 22:21

helsaawy force-pushed the he/lcowbench branch from fd2389b to 92602a5 Compare April 4, 2022 21:11

katiewasnothere approved these changes Apr 4, 2022

View reviewed changes