Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: spin build failed #109

Merged

Conversation

dierbei
Copy link
Contributor

@dierbei dierbei commented Jun 30, 2023

Fix #108

@Mossaka
Copy link
Member

Mossaka commented Jul 7, 2023

Thanks for your contribution!

This does resolve the problem that #108 brings. However, I'd love to propose an alternative way - instead of adding another target at the build-spin step, could we add it to a install-rust-targets target that does something similar to https://github.com/deislabs/containerd-wasm-shims/blob/main/.github/workflows/build.yaml#L30-L33?

This allows us to reuse the same step in both the CI and local dev environment.

Makefile Outdated
@@ -55,7 +55,7 @@ build-wws-cross-%: install-cross

.PHONY: build-spin
build-spin:
cargo build --release --manifest-path=containerd-shim-spin-v1/Cargo.toml
rustup target add wasm32-unknown-unknown && cargo build --release --manifest-path=containerd-shim-spin-v1/Cargo.toml
Copy link
Member

Choose a reason for hiding this comment

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

I propose to add new step called install-rust-targets that adds both wasm32-unknown-unknown and wasm32-wasi targets, and make it a prerequisite for build-spin and build-slight.

Copy link
Contributor Author

@dierbei dierbei Jul 8, 2023

Choose a reason for hiding this comment

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

Thanks for your advice. I modified a few places:

  1. Makefile added install-rust-targets
  2. build.yaml and ci.yaml use install-rust-targets
  3. There seems to be an issue with names being overridden in the Makefile
.PHONY: build-slight
build-slight: 

.PHONY: build-wws
build-slight:

@dierbei
Copy link
Contributor Author

dierbei commented Jul 8, 2023

@microsoft-github-policy-service agree

@dierbei dierbei force-pushed the build-spin-add-wasm32-unknown-unknown branch from 5ca7a1e to d045c1c Compare July 8, 2023 13:58
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm!

@Mossaka
Copy link
Member

Mossaka commented Jul 10, 2023

@dierbei could you please sign your commits?

@dierbei dierbei force-pushed the build-spin-add-wasm32-unknown-unknown branch from d045c1c to 99f7db0 Compare July 11, 2023 01:32
@dierbei dierbei force-pushed the build-spin-add-wasm32-unknown-unknown branch from 99f7db0 to b6cb79b Compare July 11, 2023 01:45
@dierbei
Copy link
Contributor Author

dierbei commented Jul 11, 2023

@Mossaka I tried doing a rebase so maybe the commit will be clearer.

@Mossaka Mossaka merged commit 3004cad into deislabs:main Jul 11, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An error occurred when executing make in version 0.7.0
2 participants