Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Link against wasmedge static library #187

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Link against wasmedge static library #187

merged 1 commit into from
Jul 21, 2023

Conversation

jprendes
Copy link
Collaborator

@jprendes jprendes commented Jul 19, 2023 •
edited
Loading

This PR links the wasmedge shims agains the wasmedge static library:

  • This simplifies distribution of the shims.

It uses the (new) combination of standalone and static features of the wasmedge-sys crate, which automatically downloads the static library as part of the build process:

  • This removes the need to manually install wasmedge.
  • Simplifies dependency management: the wasmedge version is now set by the upstream wasmedge-sdk crate.
  • This requires switching to a git commit for wasmedge-sdk until tag 0.10.0 is released.

@jprendes jprendes force-pushed the wasmedge-static branch 13 times, most recently from d47ecbc to 1fa970f Compare July 21, 2023 09:59
@jprendes jprendes marked this pull request as ready for review July 21, 2023 10:16
@jprendes jprendes force-pushed the wasmedge-static branch 3 times, most recently from f44283d to 7e556ad Compare July 21, 2023 11:23
rumpl
rumpl approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Beautiful

@CaptainVincent
Copy link
Contributor

Great work! This greatly improves the user's installation experience,
and everything in my tests turned out fine, too. 👍

cpuguy83
cpuguy83 requested changes Jul 21, 2023
View reviewed changes
Dockerfile
@@ -9,22 +9,14 @@ FROM --platform=$BUILDPLATFORM tonistiigi/xx:${XX_VERSION} AS xx

FROM --platform=$BUILDPLATFORM rust:${RUST_VERSION} AS base
COPY --from=xx / /
RUN apt-get update -y && apt-get install --no-install-recommends -y clang jq
RUN apt-get update -y && apt-get install --no-install-recommends -y clang jq wget
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like we should need wget?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wasmedge-sys needs wget to download the library.
We could fix that upstream for the next release of wasmedge-sdk and remove it then.
WasmEdge/wasmedge-rust-sdk#30

jprendes
jprendes commented Jul 21, 2023
View reviewed changes
Makefile
cargo build $(RELEASE_FLAG) && \
cp target/$(TARGET)/containerd-shim-wasmedge-v1 $(PWD)/bin/ && \
sudo cp /var/lib/rancher/k3s/agent/etc/containerd/config.toml /var/lib/rancher/k3s/agent/etc/containerd/config.toml.tmpl && \
echo '[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.wasm]' | sudo tee -a /var/lib/rancher/k3s/agent/etc/containerd/config.toml.tmpl && \
echo ' runtime_type = "$(PWD)/bin/containerd-shim-wasmedge-v1"' | sudo tee -a /var/lib/rancher/k3s/agent/etc/containerd/config.toml.tmpl && \
echo ' [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.wasm.options]' | sudo tee -a /var/lib/rancher/k3s/agent/etc/containerd/config.toml.tmpl && \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed these two lines from here.
I don't know what was the original motivation, but the CI check succeeded.

@defims : what was the motivation for these lines? am I missing something?

Dockerfile
@@ -9,22 +9,14 @@ FROM --platform=$BUILDPLATFORM tonistiigi/xx:${XX_VERSION} AS xx

FROM --platform=$BUILDPLATFORM rust:${RUST_VERSION} AS base
COPY --from=xx / /
RUN apt-get update -y && apt-get install --no-install-recommends -y clang jq
RUN apt-get update -y && apt-get install --no-install-recommends -y clang jq wget
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wasmedge-sys needs wget to download the library.
We could fix that upstream for the next release of wasmedge-sdk and remove it then.
WasmEdge/wasmedge-rust-sdk#30

@cpuguy83
Copy link
Member

This looks great, if there's a change that's needed for the containerd config.toml stuff we can follow-up.

@cpuguy83 cpuguy83 merged commit 2dbde80 into containerd:main Jul 21, 2023
This was referenced Jul 21, 2023
Merged
Closed
@jprendes jprendes mentioned this pull request Jul 22, 2023
Merged
@jprendes jprendes deleted the wasmedge-static branch July 23, 2023 05:26
@ipuustin
Copy link
Contributor

I'm bit on the fence with this one... I really like that we now automatically have the right version of the WasmEdge library. However I think downloading things with custom methods during the build has some issues, such as the possibility of having difficult-to-debug issues when building in a proxy prison or other not-fully-networked environments. It also makes it more difficult to have SBOMs and reproducible builds, because we have no way of knowing what exactly has been downloaded (for example -- is a WasmEdge library security fix part of a built shim or not?).

An optimal case might be to have an option to pre-download the library, or at least specify which library checksum we are expecting during the build?

@jprendes
Copy link
Collaborator Author

@ipuustin , would you mind creating an issue in the wasmedge-rust-stk repo to add this functionality?

I was looking here at how the dbus crate does it. But I'm not sure that's directly applicable to this case.

@ipuustin ipuustin mentioned this pull request Jul 24, 2023
Closed
@cpuguy83
Copy link
Member

Apologies I clearly made a mistake here with regard to automatic downloading stuff.
Ideally this would not download anything at all and use the system libs.
The lib should be a build-time dep that needs to be handled by the builder not our makefile, build.rs, or whatever from dependencies.

We can certainly create a script to install deps that someone who's building can choose to run or not.

Also probably static compilation should be behind a crate feature.

@jprendes
Copy link
Collaborator Author

I agree with making static buils a crate feature. That woud allow reverting to the previous behavior.
I am neutral to having it as a default feature or not.

How exactly the vendoring happens, I think that would be better discussed as an issue in libwasmedge-rust-sdk.
I've been looking around, and most crates vendor libraries by adding the upstream source as a git submodule, and then using cc to build it. I guess that could be a vendor feature for libwasmedge-rust-sdk, while standalone keeps the download behavior.

@jprendes jprendes mentioned this pull request Jul 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@rumpl rumpl rumpl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jprendes @CaptainVincent @cpuguy83 @ipuustin @rumpl