feat: wasi platform build by hown3d · Pull Request #738 · ko-build/ko

hown3d · 2022-06-30T08:53:21Z

This PR implements the scenario described in #675 by using tinygo as the builder for target WASI.

Documentation is not yet done, but can be created if this feature is wanted.

SBOM generation is currently not implemented, since tinygo doesn't produce the same buildInfo that go does.

Signed-off-by: Höhl, Lukas lukas.hoehl@accso.de

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

imjasonh

First of all, this is great. Thank you for putting in the time to hack on this, I think wasm is really exciting and I'm excited that ko might work with it.

I need some time to play with it before I'm confident this works (I believe you, I just don't know enough yet about the platform's expectations to know by looking).

I'd also like to think through what the UX of building a multiplatform image is, when one of those platforms is wasm. Historically, ko has only built for platforms provided by the base image, and in this case we're adding a new platform that isn't provided by any base image, but should still show up in the final image. So that's going to be weird, but nothing we can't address with enough integration tests and docs.

Thanks again for sending this, let me chew on it and play around with it a bit, but this looks really promising so far.

pkg/build/gobuild.go

imjasonh · 2022-06-30T13:31:03Z

pkg/build/gobuild.go

 	return config
 }

+const wasmLayerAnnotationKey = "module.wasm.image/variant"


Is this annotation documented anywhere? What expects to read it?

Specification of this annotation are here: https://github.com/solo-io/wasm/blob/master/spec/spec-compat.md

crun also has an experimental annotation run.oci.handler which can be used to specifiy the handler for the container (e.g. wasm, wasm-smart). More in https://man.archlinux.org/man/crun.1.en

imjasonh · 2022-06-30T13:32:02Z

pkg/build/gobuild.go

-	appPath := path.Join(appDir, appFilename(ref.Path()))
+	var filename string
+	if platform.String() == "wasm/wasi" {
+		// module.wasm.image/variant=compat-smart expects the entrypoint binary to be of form .wasm


Can you link to where this expectation is documented? I'd also just love to learn more about runtimes that run these things.

I'm currently focusing on the implementation in crun: https://github.com/containers/crun/blob/main/src/libcrun/handlers/handler-utils.c

https://github.com/containers/crun/blob/65028ce3057180395a1826d8c0906f9ff299030a/src/libcrun/custom-handler.c#L208 will check all the registered handlers by calling wasm_can_handle_container function from handler-utils.c.

pkg/build/gobuild.go

imjasonh · 2022-06-30T13:35:12Z

pkg/build/gobuild.go

 		updatePath(cfg, `C:\ko-app`)
 		cfg.Config.Env = append(cfg.Config.Env, `KO_DATA_PATH=C:\var\run\ko`)
-	} else {
+	} else if platform.String() != "wasm/wasi" {


If we keep doing this comparison, I wonder if it warrants having a helper method isWasi(platform) (and isWindows(platform)) to make this easier to read.

Then we can also avoid the string comparison without bloating the callsite:

func isWasi(p v1.Platform) bool { return p.OS == "wasm" && p.Architecture == "wasi" }

Yes, this is a good idea. The constant comparison with platform string was kinda hacky anyway.

imjasonh · 2022-06-30T13:35:52Z

pkg/build/gobuild.go

+		// user specified a platform, so force this platform, even if it fails
+		var platform *v1.Platform
+		if len(g.platformMatcher.platforms) == 1 {
+			platform = &g.platformMatcher.platforms[0]
+		}
+		res, err = g.buildOne(ctx, s, baseImage, platform)


Why's this necessary?

platform had been set to nil otherwise, which would remove the possibility to check for the platform string in the build function: https://github.com/hown3d/ko/blob/ceac5e5fcea61bea80231a689bb990cce3198827/pkg/build/gobuild.go#L274

Ah okay. In that case, I think we can remove this check here: https://github.com/hown3d/ko/blob/ceac5e5fcea61bea80231a689bb990cce3198827/pkg/build/gobuild.go#L722

This does make me wonder how ko will behave (before and after this change) when asked to build on top of a single-platform image that isn't linux/amd64, with --platform unset... 🤔

I created a single platform image (no OS or Architecture set) here: https://quay.io/repository/hown3d/ko-wasm-tests/manifest/sha256:2374307703312554852605270f7979f1057e4d6b98bcb9f5f7cf4cabf9ca8b4f and used it as as the base image. The result was that, the new image didn't have architecture and os set aswell.

imjasonh · 2022-06-30T13:37:45Z

pkg/commands/config.go

 		}
+
+		// look for wasm/wasi platform
+		for _, p := range bo.Platforms {


Does this mean that if a user passes --platform=linux/amd64,wasm/wasi, that they'll only get the wasi image?

Ah good argument, haven't thought about providing multiple architectures with wasi. I'll figure out, if there's a good way when using wasi in multiple platforms.

I've filed a request for clarification here: solo-io/wasm#291

imjasonh · 2022-06-30T13:41:31Z

cc @squillace 👀

Co-authored-by: Jason Hall <jason@chainguard.dev>

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

codecov-commenter · 2022-07-05T15:31:23Z

Codecov Report

Merging #738 (3f9801f) into main (691da5a) will decrease coverage by 0.45%.
The diff coverage is 61.78%.

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
- Coverage   51.52%   51.07%   -0.46%     
==========================================
  Files          44       45       +1     
  Lines        3276     3350      +74     
==========================================
+ Hits         1688     1711      +23     
- Misses       1377     1417      +40     
- Partials      211      222      +11

Impacted Files	Coverage Δ
pkg/commands/config.go	`56.09% <33.33%> (-1.80%)`	⬇️
pkg/build/gobuild.go	`57.85% <50.94%> (-3.60%)`	⬇️
pkg/build/platform/platform.go	`91.11% <91.11%> (ø)`
pkg/commands/build.go	`56.25% <0.00%> (-5.05%)`	⬇️
pkg/commands/options/validate.go	`0.00% <0.00%> (ø)`
pkg/commands/options/build.go	`69.56% <0.00%> (+0.69%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 691da5a...3f9801f. Read the comment docs.

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

hown3d · 2022-07-06T10:32:02Z

I tried to declutter the build package a bit to provide a more readable structure.
The current build package is huge and it's hard to understand what build (building binary), Build (top level build), BuildAll (image building, this is extremely confusing when build func builds the binary) and BuildOne do.

Thats why I moved everything regarding platform matching inside a platform package, config structs into a config package and binary building into a binary package.

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

imjasonh · 2022-07-06T13:55:47Z

I tried to declutter the build package a bit to provide a more readable structure. The current build package is huge and it's hard to understand what build (building binary), Build (top level build), BuildAll (image building, this is extremely confusing when build func builds the binary) and BuildOne do.

Thats why I moved everything regarding platform matching inside a platform package, config structs into a config package and binary building into a binary package.

This isn't a bad change, but it does make the PR quite a lot bigger and harder to follow. It also makes previously unexported methods exported, and we should probably hide more things in internal so that we don't get external dependencies on them.

In other words, I agree restructuring pkg/build sounds like a worthwhile endeavor, but I think there's more to talk about there, and it distracts from the goal of adding wasi support. Can we do a separate restructuring PR, then use it to add wasi? Or would you prefer to add wasi first, leaving the structure as-is?

hown3d · 2022-07-06T15:59:21Z

Yes, that's reasonable. I think I'll go for the restructuring first because that makes implementing wasi support a bit easier and cleaner.

hown3d · 2022-09-03T11:02:02Z

I did manage to find some time to reiterate on this feature, and I created a new branch to start of fresh.
I'll open a second pull request with the new changes.

feat: wasi platform build

22b047b

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

imjasonh self-requested a review June 30, 2022 13:27

imjasonh reviewed Jun 30, 2022

View reviewed changes

Lukas and others added 5 commits June 30, 2022 16:09

Update pkg/build/gobuild.go

3b180a1

Co-authored-by: Jason Hall <jason@chainguard.dev>

Update pkg/build/gobuild.go

86c5e0f

Co-authored-by: Jason Hall <jason@chainguard.dev>

Update pkg/build/gobuild.go

ceac5e5

Co-authored-by: Jason Hall <jason@chainguard.dev>

refactor(build): create platform pkg

66cc19d

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

ci(wasm): include script directly in actions

3f9801f

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

hown3d mentioned this pull request Jul 5, 2022

feat: implement oci conversion google/go-containerregistry#1293

Closed

refactor: declutter build package

c3a0ebe

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

fix: force oci conversion for wasi platform

abcad87

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

hown3d closed this Sep 3, 2022

hown3d mentioned this pull request Sep 3, 2022

feat: wasi platform build #810

Closed

hown3d deleted the wasi branch September 9, 2022 14:04

Conversation

hown3d commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imjasonh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hown3d Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hown3d Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imjasonh commented Jun 30, 2022

Uh oh!

codecov-commenter commented Jul 5, 2022

Codecov Report

Uh oh!

hown3d commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imjasonh commented Jul 6, 2022

Uh oh!

hown3d commented Jul 6, 2022

Uh oh!

hown3d commented Sep 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hown3d commented Jun 30, 2022 •

edited

Loading

hown3d Jun 30, 2022 •

edited

Loading

hown3d Jun 30, 2022 •

edited

Loading

hown3d commented Jul 6, 2022 •

edited

Loading