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

Remove sync code from runc shim #155

Merged
merged 3 commits into from
Aug 8, 2023
Merged

Remove sync code from runc shim #155

merged 3 commits into from
Aug 8, 2023

Conversation

mxpv
Copy link
Member

@mxpv mxpv commented Jul 24, 2023

Closes: #134

@mxpv
Copy link
Member Author

mxpv commented Aug 1, 2023

@jsturtevant any ideas why windows integration tests fail? Not sure I get what exactly wrong with it and I don't have windows env to verify (this also fails in main).

error[E0432]: unresolved import `ttrpc::asynchronous`
  --> crates\shim-protos\src\lib.rs:93:20
   |
93 |     pub use ttrpc::asynchronous::Client;
   |                    ^^^^^^^^^^^^ could not find `asynchronous` in `ttrpc`

@jsturtevant
Copy link
Contributor

looks like bringing in the async feature somehow. I will take a look today.

this also fails in main

Looks like main is failing due to #160?

@mxpv
Copy link
Member Author

mxpv commented Aug 2, 2023

Let me rebase this and give it another try.

@mxpv mxpv force-pushed the cleanup branch 3 times, most recently from d5b811e to 7b07c04 Compare August 2, 2023 20:27
@jsturtevant
Copy link
Contributor

rebase didn't help 😞.

CI on main is passing today: https://github.com/containerd/rust-extensions/actions/runs/5743079375 and I was able to run the integration test locally on main. Going to pull this PR to see I can track down what is causing the mix up

@jsturtevant
Copy link
Contributor

Still tracking down what is happening but cargo build -p containerd-shim-protos and cargo build -p containerd-shim builds fine on windows.

but cargo build --example skeleton fails with message error: could not compile containerd-shim-protos (lib) due to 117 previous error

@jsturtevant
Copy link
Contributor

jsturtevant commented Aug 2, 2023

@mxpv I don't know why but it seems the examples are getting the feature=async enabled on them after the changes in this PR. Maybe the changes in the runc crate are causing the compiler/cargo to pick that up by default?

If I run cargo build --example skeleton -p containerd-shim then this works. So the fix would be to add -p containerd-shim and -p containerd-shim-protos to the cargo calls in:

cargo run --example skeleton -- -namespace default -id 1234 -address "\\.\pipe\containerd-containerd" -publish-binary ./bin/containerd start
ps skeleton
cargo run --example shim-proto-connect \\.\pipe\containerd-shim-17630016127144989388-pipe

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv mxpv marked this pull request as ready for review August 8, 2023 17:52
@mxpv
Copy link
Member Author

mxpv commented Aug 8, 2023

@jsturtevant That did the trick!

@jsturtevant
Copy link
Contributor

LGTM

@mxpv mxpv merged commit ad83804 into containerd:main Aug 8, 2023
13 checks passed
@mxpv mxpv deleted the cleanup branch August 8, 2023 19:45
@mxpv mxpv added C-runc-shim Runc shim T-cleanup Project cleanup labels Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-runc-shim Runc shim T-cleanup Project cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: remove sync code
2 participants