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

Refactor use statements to enable building Windows #238

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Aug 9, 2023

This PR does not enable windows features but it does make it so the shim will build on Windows. This will make the following PR's smaller and easier to digest to do it in steps. It also makes it easier while other changes go in without having to rebase complex changes.

one small step towards #49

For now this disables dead-code and unused imports but should be turned on before in following PR's as functionality is added.

Todo in the PR:
Add a CI

  • job that validates the build on windows.

@jsturtevant jsturtevant force-pushed the build-on-windows branch 12 times, most recently from 8c1e606 to 9449ca0 Compare August 10, 2023 00:19
@jsturtevant
Copy link
Contributor Author

The CI is passing 🥳

I've opened #240, #239 to make this PR smaller and easier to review. Since #226 is close to merging I would suggest we get these merged and that would also make this PR much smaller and simpler.

@jsturtevant jsturtevant mentioned this pull request Aug 10, 2023
30 tasks
Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

Thanks for working in this :-).
I think native windows support would add a lot of value.

I haven't finished, but here are my comments so far.

Also, I'm not a big fan of the new cfg_window / cgf_unix macros.
I think they add a lot of cyclomatic complexity to the code.
I would much prefer to create separate os-specific modules, and factor out the code that differs between each os.

.cargo/config.toml Show resolved Hide resolved
.github/scripts/build-windows.sh Outdated Show resolved Hide resolved
.github/scripts/build-windows.sh Outdated Show resolved Hide resolved
.github/scripts/build-linux.sh Outdated Show resolved Hide resolved
crates/containerd-shim-wasm/src/sandbox/oci.rs Outdated Show resolved Hide resolved
@jsturtevant jsturtevant force-pushed the build-on-windows branch 2 times, most recently from 3199cd8 to 5c51fd0 Compare August 18, 2023 18:33
@jsturtevant
Copy link
Contributor Author

latest failure was the outofspace we are seeing on other PRs:

 Warning  FailedCreatePodSandBox  43s   kubelet            Failed to create pod sandbox: rpc error: code = Unknown desc = failed to create containerd container: failed to create prepare snapshot dir: mkdir /var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/new-3941906082/fs: no space left on device

@jsturtevant jsturtevant force-pushed the build-on-windows branch 7 times, most recently from 0ee6fe6 to ea1e5e6 Compare August 21, 2023 21:26
@jsturtevant jsturtevant marked this pull request as ready for review August 21, 2023 21:26
@jsturtevant
Copy link
Contributor Author

Also, I'm not a big fan of the new cfg_window / cgf_unix macros.
I think they add a lot of cyclomatic complexity to the code.
I would much prefer to create separate os-specific modules, and factor out the code that differs between each os.

@jprendes I cleaned this up where it made sense. In some cases, I believe its easier to read if it is in-lined but let me know what you think. The one last place I would like to do this is around the mounting code but I don't know the abstraction to put in place just yet. I've started this on the Windows side but haven't full flushed it out but will follow up on moving it to an abstraction in the next PR.

@jsturtevant
Copy link
Contributor Author

/assign @Mossaka @jprendes @cpuguy83

@jsturtevant
Copy link
Contributor Author

test failure is #252

cpuguy83
cpuguy83 previously approved these changes Aug 21, 2023
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking great!
Looking forward to having this merged :-)

crates/containerd-shim-wasm/src/lib.rs Show resolved Hide resolved
Mossaka
Mossaka previously approved these changes Aug 21, 2023
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!!

Makefile Show resolved Hide resolved
scripts/setup-windows.sh Show resolved Hide resolved
crates/containerd-shim-wasm/src/sys/mod.rs Outdated Show resolved Hide resolved
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!

Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
@jsturtevant
Copy link
Contributor Author

looks like a different issue with ubuntu 22.04, now that storage has been solved.

Signed-off-by: James Sturtevant <[email protected]>
@Mossaka
Copy link
Member

Mossaka commented Aug 22, 2023

Cool, shall I merge this in? @jsturtevant

@Mossaka Mossaka merged commit 87fb110 into containerd:main Aug 22, 2023
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.

4 participants