Add support for Windows (cross compiled) #108

pmengelbert · 2024-02-05T21:38:19Z

Very rough, mostly looking to get feedback. To test (of course with the re-built frontend):

docker build -f test/fixtures/moby-containerd.yml --target=windows/zip --output=_zip .

cpuguy83

For tests, I'd like to move over to using the new integration test suite.
Additionally I really don't want to be building containerd (or runc) on every test run.

This is part of why I'm pushing for inline sources (#98).
We can test that a spec does everything we expect it to without having to build a bunch of projects.

We can of course add an e2e suite (later) that double checks real projects, but probably I'd only run it either on merge or before releases.

frontend/windows/handle_zip.go

pmengelbert · 2024-02-07T18:56:20Z

This same logic of entering a subshell, adding env vars with export before command, and writing the command, is re-used now across several places (RPM, Jammy PR, now here). I wonder if we can abstract this out into a separate function or consider how best to reduce duplicated code here

I copied/pasted this from Brian's jammy PR, we should definitely abstract 🙂

cpuguy83 · 2024-03-11T17:17:58Z

Looks like the rebase on this was incomplete.
Code is not compiling.

pmengelbert · 2024-03-11T17:29:22Z

Looks like the rebase on this was incomplete. Code is not compiling.

Working on it now

cpuguy83 · 2024-03-11T17:28:50Z

frontend/windows/handle_zip.go

+)
+
+var (
+	varCacheAptMount = llb.AddMount("/var/cache/apt", llb.Scratch(), llb.AsPersistentCacheDir("dalec-windows-var-cache-apt", llb.CacheMountLocked))


For these to work, we also need to remove /etc/apt/apt.conf.d/docker-clean from the base image.
You should be able to acheive this by mounting over the file, like llb.AddMount("/etc/apt/apt.conf.d/docker-clean", llb.Scratch())

Just a thought, you could have all this stored as a RunOpt.

(Peter) Fix this

cpuguy83 · 2024-03-11T17:35:21Z

test/fixtures/moby-containerd.yml

+# syntax=ghcr.io/azure/dalec/frontend:latest
+
+# TODO: The spec is not currently setting the revision in the containerd version
+# This should be fixed before this spec is used to build a real package.
+
+args:
+  CONTAINERD_COMMIT: 7c3aca7a610df76212171d200ca3811ff6096eb8
+  REVISION: 1
+  VERSION: "1.7.13"
+  HCS_SHIM_REPO: https://github.com/Microsoft/hcsshim.git
+
+name: moby-containerd
+description: |
+  Industry-standard container runtime
+   containerd is an industry-standard container runtime with an emphasis on
+   simplicity, robustness and portability. It is available as a daemon for Linux
+   and Windows, which can manage the complete container lifecycle of its host
+   system: image transfer and storage, container execution and supervision,
+   low-level storage and network attachments, etc.
+   .
+   containerd is designed to be embedded into a larger system, rather than being
+   used directly by developers or end-users.
+website: https://containerd.io/
+version: ${VERSION}
+revision: ${REVISION}
+vendor: Moby
+
+targets: # Distro specific build requirements
+  windows:
+    dependencies:
+      build:
+        golang-1.20:
+
+packager: Microsoft <[email protected]>
+license: Apache 2.0
+sources:
+  containerd:
+    git:
+      url: https://github.com/containerd/containerd.git
+      commit: "${CONTAINERD_COMMIT}"
+  hcsshim:
+    path: /build/_out
+    image:
+      ref: mcr.microsoft.com/oss/go/microsoft/golang:1.20
+      cmd:
+        dir: /build/containerd
+        steps:
+          - command: |
+              apt update && apt install -y jq
+              shim_mod_dir="$(
+                go mod download -json github.com/Microsoft/hcsshim | jq -r '.Dir'
+              )"
+              chmod +w -R "$shim_mod_dir"
+              mv "$shim_mod_dir"/*  /build/_out
+              cd /build/_out
+              go mod vendor
+        mounts:
+          - dest: /build/containerd/go.mod
+            spec:
+              path: /go.mod
+              git:
+                url: https://github.com/containerd/containerd.git
+                commit: "${CONTAINERD_COMMIT}"
+
+
+build:
+  env:
+    CGO_ENABLED: 1
+    GOOS: windows
+    GOGC: "off"
+    GOPROXY: "off"
+    GOFLAGS: -trimpath
+    VERSION: ${VERSION}
+    COMMIT: ${CONTAINERD_COMMIT}
+    GOROOT: /usr/lib/go-1.20
+    CC: x86_64-w64-mingw32-gcc
+  steps:
+    - command: |
+        set -e
+        export PATH="${GOROOT}/bin:${PATH}"
+        cd containerd
+        make VERSION="$VERSION" REVISION="$COMMIT" DESTDIR=./bin binaries
+        rm -f bin/containerd-stress.exe
+    - command: |
+        set -e
+        export PATH="${GOROOT}/bin:${PATH}"
+        cd hcsshim
+        GO111MODULE=on go build \
+          -mod=vendor \
+          -o bin/containerd-shim-runhcs-v1.exe \
+          ./cmd/containerd-shim-runhcs-v1
+
+artifacts:
+  binaries:
+    hcsshim/bin/containerd-shim-runhcs-v1.exe:
+    containerd/bin/containerd.exe:
+    containerd/bin/ctr.exe:


Can we move this to an integration test?

We do have a (somewhat) generic test function that could possible be reused here.
It may need an extra option or something since we can't really test any container output at this time.

Move yaml fixtures to integration test suit

pmengelbert · 2024-03-11T21:46:59Z

Just pushed a few changes. We should have a working container target, but I haven't migrated the test yet.

cpuguy83

I think we need to validate the spec for runtime dependencies for Windows, since TMK, we don't really have any way to support that.

frontend/windows/handler.go

cpuguy83 · 2024-03-14T17:57:59Z

frontend/windows/handle_zip.go

+)
+
+var (
+	varCacheAptMount = llb.AddMount("/var/cache/apt", llb.Scratch(), llb.AsPersistentCacheDir("dalec-windows-var-cache-apt", llb.CacheMountLocked))


Just a thought, you could have all this stored as a RunOpt.

cpuguy83 · 2024-03-14T18:01:59Z

frontend/windows/handle_zip.go

+
+func SpecToSourcesLLB(spec *dalec.Spec, sOpt dalec.SourceOpts) (map[string]llb.State, error) {
+	m := make(map[string]llb.State, len(spec.Sources))
+	keys := dalec.SortMapKeys(spec.Sources)


This sorting isn't doing anything since it is putting everything back into a map.

The calls to AsState don't matter? I was thinking that because we're manipulating LLB we want to do them in the same order so we don't get cache misses. LMK if that doesn't matter here.

The states are all separate from each other and do not interact at all until we actually copy them to a merged llb.State.

Remove sorting here

cpuguy83 · 2024-03-14T18:02:08Z

frontend/windows/handle_zip.go

+	f(ei)
+}
+
+func SpecToSourcesLLB(spec *dalec.Spec, sOpt dalec.SourceOpts) (map[string]llb.State, error) {


Can we unexport this?

frontend/windows/handle_zip.go

Creates the broad structure for a windows target. Modeled after the mariner target. Signed-off-by: Peter Engelbert <[email protected]>

Inspired by github.com/Azure/moby-packaging Signed-off-by: Peter Engelbert <[email protected]>

This will be used for testing during the development of the target. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

Currently this has only been tested for the moby-containerd spec in test/fixtures. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

This reverts commit 251a6b1.

Signed-off-by: Peter Engelbert <[email protected]>

Please the linter Signed-off-by: Peter Engelbert <[email protected]>

image.go

source.go

frontend/windows/handle_zip.go

Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]> Refactor Signed-off-by: Peter Engelbert <[email protected]> Revert "Refactor" This reverts commit 9ac4675b4c3d3c19539266b5f20d76f97e9d6b84.

This reverts parts of commit 845d6a1. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 · 2024-03-20T17:18:00Z

frontend/imgconfig.go

+	return &img, nil
+}
+
+func getBaseOutputImage(spec *dalec.Spec, target, defaultBase string) string {


Let's make this exported.
Then we can pass in the output of this as in input to BuildImageConfig, which removes a function paramerter for BuildImageConfig.

test/windows_test.go

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 · 2024-03-21T20:56:21Z

Dockerfile

@@ -4,8 +4,8 @@ FROM go  AS frontend-build
 WORKDIR /build
 COPY . .
 ENV CGO_ENABLED=0 GOFLAGS=-trimpath
-ARG TARGETARCH TARGETOS
-ENV GOOS=${TARGETOS} GOARCH=${TARGETARCH}
+ARG TARGETARCH TARGETOS GOFLAGS


What's the purpose of this?

The purpose of this is to make it easier to build the binaries with debug symbols and without optimizations. I find it helpful to compile a debug version, and then attach to the frontend process with a debugger. I had been manually editing the Dockerfile to add -gcflags=all="-N -l" in the go compilation commands. I kept accidentally checking in the changes. It also got annoying having to add it over and over.

I think this would need to go above the ENV line with GOFLAGS with the default value for the arg as -trimpath.

cpuguy83 · 2024-03-21T21:05:59Z

source.go

-		states[sourceName] = patchSource(worker, sourceState, states, patches, withConstraints(opts))
+
+		var nestedPath string
+		if SourceIsDir(spec.Sources[sourceName]) {


This is new behavior that is not mimicked in the rpm patcher.
Was there a particular reason to add it?

I think the reason it was added here (correct me if I'm wrong @pmengelbert) is because specToSourcesLLB takes the extra step of nesting a source within a directory in the LLB itself. I.e., the LLB contains the directory as well as the contents. I think that nesting step could be moved out of spectToSourcesLLB (if indeed it is needed), so we don't have to handle that behavior here. Otherwise, we could use patch -d which I believe is how it works in the rpm patcher?

@adamperlin yes that's correct. I went back and changed specToSourcesLLB so that it doesn't nest the sources in the directories. Now that's happening in the RunOpt that mounts the sources. I restored the two patching functions (the public and private) to the original versions. This is much cleaner. Thanks for pointing this out @cpuguy83.

This is done

This is not done. Working on it now.

Now this is done.

cpuguy83 · 2024-03-21T21:08:05Z

frontend/windows/handle_container.go

+	case 1:
+		platform = bc.TargetPlatforms[0]
+	default:
+		return ocispecs.Platform{}, fmt.Errorf("multiple targets for a windows build, only amd64 is supported")


This error mentions amd64 but doesn't validate it.
Also I'm not sure why it would matter since we aren't trying to run anything.

We are making sure there are not multiple platforms specified. Because only one platform is supported (windows/amd64), if there is more than one platform requested by the user, the request cannot be satisfied.

The amd64 mention was an attempt at providing additional information. The idea was to tell the user, "try again, but only with amd64".

I improved the error message. LMK if you want something different.

done unless you want me to change it again.

cpuguy83 · 2024-03-22T14:58:22Z

Dockerfile

 ENV CGO_ENABLED=0 GOFLAGS=-trimpath
-ARG TARGETARCH TARGETOS
-ENV GOOS=${TARGETOS} GOARCH=${TARGETARCH}
+ARG TARGETARCH TARGETOS GOFLAGS
+ENV GOOS=${TARGETOS} GOARCH=${TARGETARCH} GOFLAGS=${GOFLAGS}


Suggested change

ENV CGO_ENABLED=0 GOFLAGS=-trimpath

ARG TARGETARCH TARGETOS

ENV GOOS=${TARGETOS} GOARCH=${TARGETARCH}

ARG TARGETARCH TARGETOS GOFLAGS

ENV GOOS=${TARGETOS} GOARCH=${TARGETARCH} GOFLAGS=${GOFLAGS}

ENV CGO_ENABLED=0

ARG TARGETARCH TARGETOS GOFLAGS=-trimpath

Makes running the frontend in a container much easier Signed-off-by: Peter Engelbert <[email protected]>

The files are actually copied to their new locations, rather than linked. This will likely change in the future when Windows-in-buildkit reaches stable status. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 · 2024-03-22T15:51:00Z

Great work, @pmengelbert.
Thanks for working through the constant back and forth on the review on this one.

pmengelbert requested review from cpuguy83 and adamperlin February 5, 2024 21:38

cpuguy83 reviewed Feb 7, 2024

View reviewed changes

adamperlin reviewed Feb 7, 2024

View reviewed changes

frontend/windows/handle_zip.go Show resolved Hide resolved

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from fcdb783 to 7e2e52c Compare March 9, 2024 23:25

pmengelbert requested a review from a team as a code owner March 9, 2024 23:25

cpuguy83 requested changes Mar 11, 2024

View reviewed changes

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from e2e16b0 to 76545ad Compare March 11, 2024 21:43

cpuguy83 requested changes Mar 14, 2024

View reviewed changes

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from fc38d4f to 014b5da Compare March 18, 2024 17:17

pmengelbert mentioned this pull request Mar 18, 2024

Rewrite target routing logic #172

Merged

pmengelbert added 15 commits March 18, 2024 14:41

Add windows target skeleton

403e5c0

Creates the broad structure for a windows target. Modeled after the mariner target. Signed-off-by: Peter Engelbert <[email protected]>

Set up windows builder image

d7ae44f

Inspired by github.com/Azure/moby-packaging Signed-off-by: Peter Engelbert <[email protected]>

Add windows spec for moby-containerd

59d6bec

This will be used for testing during the development of the target. Signed-off-by: Peter Engelbert <[email protected]>

Improve hcs_shim source fetching & building

64095fa

Signed-off-by: Peter Engelbert <[email protected]>

Remove commented code in spec

6c079c9

Signed-off-by: Peter Engelbert <[email protected]>

Update containerd spec

e76d87a

Signed-off-by: Peter Engelbert <[email protected]>

Working zip target

a1350fe

Currently this has only been tested for the moby-containerd spec in test/fixtures. Signed-off-by: Peter Engelbert <[email protected]>

Refactor windows zip handler

8e2bcd8

Signed-off-by: Peter Engelbert <[email protected]>

Refactor utility code for use elsewhere

a23b624

Signed-off-by: Peter Engelbert <[email protected]>

Implement Windows container target

3cadeab

Signed-off-by: Peter Engelbert <[email protected]>

Fix errors that were preventing dalec from building

65e8d1a

Signed-off-by: Peter Engelbert <[email protected]>

Remove unused function

e3d9166

Signed-off-by: Peter Engelbert <[email protected]>

save all

d76374a

Signed-off-by: Peter Engelbert <[email protected]>

Save

929162e

Signed-off-by: Peter Engelbert <[email protected]>

Revert "Save"

ee421a9

This reverts commit 251a6b1.

pmengelbert added 3 commits March 19, 2024 12:19

Move build dep installation to llb.StateOption

d4e6d38

Signed-off-by: Peter Engelbert <[email protected]>

Mount directory sources instead of merging

7a5c627

Signed-off-by: Peter Engelbert <[email protected]>

Remove dead code

b9d03e0

Please the linter Signed-off-by: Peter Engelbert <[email protected]>

DannyBrito reviewed Mar 19, 2024

View reviewed changes

image.go Outdated Show resolved Hide resolved

DannyBrito reviewed Mar 19, 2024

View reviewed changes

source.go Show resolved Hide resolved

DannyBrito reviewed Mar 19, 2024

View reviewed changes

frontend/windows/handle_zip.go Outdated Show resolved Hide resolved

pmengelbert added 6 commits March 19, 2024 14:43

Remove closer around patching function

0487d9a

Signed-off-by: Peter Engelbert <[email protected]>

Move switching logic to Source method

f43c0b7

Signed-off-by: Peter Engelbert <[email protected]>

Quote env vars in build script

c66ca68

Signed-off-by: Peter Engelbert <[email protected]>

Tidy

327e98e

Signed-off-by: Peter Engelbert <[email protected]> Refactor Signed-off-by: Peter Engelbert <[email protected]> Revert "Refactor" This reverts commit 9ac4675b4c3d3c19539266b5f20d76f97e9d6b84.

Revert "Enable static validations per-handler"

bc50ecb

This reverts parts of commit 845d6a1. Signed-off-by: Peter Engelbert <[email protected]>

Validate that Windows output containers do not have runtime deps

b4cfaa5

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from c317605 to b4cfaa5 Compare March 20, 2024 15:59

Move image config building to frontend package

c1ca48e

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 mentioned this pull request Mar 20, 2024

[REQ] Target handlers should have standard target paths #182

Open

1 task

cpuguy83 requested changes Mar 20, 2024

View reviewed changes

Continue refactor of BuildImageConfig

3eccd15

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert requested a review from cpuguy83 March 21, 2024 20:48

cpuguy83 requested changes Mar 21, 2024

View reviewed changes

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from fce5b0f to 3a56e6d Compare March 22, 2024 14:45

cpuguy83 reviewed Mar 22, 2024

View reviewed changes

pmengelbert added 4 commits March 22, 2024 11:37

Enable setting GOFLAGS in Dockerfile via build args

3a85b5b

Makes running the frontend in a container much easier Signed-off-by: Peter Engelbert <[email protected]>

Add "symlink" support for Windows

c42779a

The files are actually copied to their new locations, rather than linked. This will likely change in the future when Windows-in-buildkit reaches stable status. Signed-off-by: Peter Engelbert <[email protected]>

Improve platform-related error message for Windows outputs

3dff0e4

Signed-off-by: Peter Engelbert <[email protected]>

Restore patching to prior functionality

ce6fd9b

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert force-pushed the pmengelbert/windows_target/1 branch from 3a56e6d to ce6fd9b Compare March 22, 2024 15:37

cpuguy83 approved these changes Mar 22, 2024

View reviewed changes

cpuguy83 merged commit ee11e7b into Azure:main Mar 22, 2024
9 checks passed

cpuguy83 mentioned this pull request May 6, 2024

Add Windows cross-compile toolchain #62

Closed

Add support for Windows (cross compiled) #108

Add support for Windows (cross compiled) #108

Conversation

pmengelbert commented Feb 5, 2024

cpuguy83 left a comment

Choose a reason for hiding this comment

pmengelbert commented Feb 7, 2024 via email • edited Loading

cpuguy83 commented Mar 11, 2024

pmengelbert commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert commented Mar 11, 2024

cpuguy83 left a 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

pmengelbert Mar 18, 2024 • 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

adamperlin Mar 21, 2024 • 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

cpuguy83 commented Mar 22, 2024

pmengelbert commented Feb 7, 2024 via email •

edited

Loading

pmengelbert Mar 15, 2024 •

edited

Loading

pmengelbert Mar 11, 2024 •

edited

Loading

pmengelbert Mar 18, 2024 •

edited

Loading

adamperlin Mar 21, 2024 •

edited

Loading