Skip to content

Conversation

@Mossaka
Copy link
Member

@Mossaka Mossaka commented Nov 5, 2024

An attempt to resolve #326 (comment)

Questions: should we add cargo build -p runc to the CI since the CI is not building runc with default target at all?

Can you take a look at this PR? @ningmingxiao, @mxpv

@github-actions github-actions bot added the C-runc runc helper label Nov 5, 2024
@jokemanfire
Copy link
Member

jokemanfire commented Nov 6, 2024

I think this change is great ,but how about using pipe but not using fifo directly, because there's so many problem which I meet in FIFO direct. #278

@Mossaka
Copy link
Member Author

Mossaka commented Nov 6, 2024

My goal is to do a refactoring so that the code can compile. I am happy to rebase on your Pr if that works

@jokemanfire
Copy link
Member

My goal is to do a refactoring so that the code can compile. I am happy to rebase on your Pr if that works

I think I should wait for your split , because I forget the sync feature too, that pipe problem is only existing in async feature.

@mxpv
Copy link
Member

mxpv commented Nov 6, 2024

Questions: should we add cargo build -p runc to the CI since the CI is not building runc with default target at all?

I wonder why is it not included. On Linux, the entire Cargo workspace should be included?

@Mossaka
Copy link
Member Author

Mossaka commented Nov 16, 2024

I was AFK this entire week. Will follow up to address the comments when I get back to work.

Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
@Mossaka
Copy link
Member Author

Mossaka commented Nov 16, 2024

I think I should wait for your spli

Okay, I will leave my PR to merge as it is, and the issue regarding FIFO could be left to a follow up. I don't want to block your PR

@Mossaka
Copy link
Member Author

Mossaka commented Nov 16, 2024

I wonder why is it not included. On Linux, the entire Cargo workspace should be included?

I think we are missing a --workspace flag on ci.yml

The runc shim, like the containerd-shim, has a 'sync' feature that is not checked by the workspace.
Thus we need to add that individually to the CI to get full coverage

Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
@github-actions github-actions bot added the T-CI Changes in project's CI label Nov 16, 2024
@Mossaka Mossaka closed this Nov 17, 2024
@Mossaka Mossaka reopened this Nov 17, 2024
@Mossaka
Copy link
Member Author

Mossaka commented Nov 27, 2024

Can we merge this PR now? @mxpv

@mxpv mxpv added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@mxpv mxpv added this pull request to the merge queue Dec 3, 2024
Merged via the queue into containerd:main with commit f87972b Dec 3, 2024
35 of 36 checks passed
@Mossaka Mossaka deleted the fix-runc branch December 3, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-runc runc helper T-CI Changes in project's CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants