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.

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

Simple api on Windows #314

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Simple api on Windows #314

merged 3 commits into from
Sep 19, 2023

Conversation

jprendes
Copy link
Collaborator

This PR adds the skeleton to make the simple API available on windows.
The actual implementation is still todo!(), but this change allows us to unify the implementation of the shim binaries.

@jprendes jprendes force-pushed the simple-api-windows branch 2 times, most recently from f3fd055 to 174d545 Compare September 14, 2023 10:04
@jprendes jprendes marked this pull request as ready for review September 14, 2023 10:19
@jprendes
Copy link
Collaborator Author

@jsturtevant PTAL, let me know if something like this makes sense for Windows, or if there's something fundamentally wrong.

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

This looks great, This greatly simplifies the work a shim implementor will need to do to support both platforms. I believe this will work for Windows even with our discussion around the differences between launching new process on Windows/Linux.

Comment on lines 45 to 46
default = []
libcontainer_default = ["libcontainer/default"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This would mean that shim-implementors would be required to use libcontainer? Should the Instance api still be available for workloads that don't want to use libcontainer? Is that a scenario we should support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be a required dependency in Linux, only.
You can still choose not to use it by using the Instance API instead of the Engine API.
The main downside of this is that we are forcing downstream dependencies to build libcontianer even if they don't use it. The linker should be smart enough to remove almost every trace of libcontainer from the final binary (except for the error definitions).

The main motivation for this is that the features system is not great with system dependent branching, and the number of combinations we have to handle grows exponentially.
But I'm open to suggestions

@jprendes jprendes force-pushed the simple-api-windows branch 2 times, most recently from dae6ec0 to 612a79d Compare September 19, 2023 08:07
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!

@@ -20,26 +20,26 @@ homepage = "https://github.com/containerd/runwasi"

[workspace.dependencies]
anyhow = "1.0"
cap-std = "1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Just curious - are you using some script to sort the deps?

@Mossaka Mossaka merged commit c2c6d14 into containerd:main Sep 19, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants