Skip to content

Conversation

@dcantah
Copy link
Contributor

@dcantah dcantah commented Jul 10, 2025

readabilityHandler is a bit crummy if it's a regular file. This just writes our own loop and exits and shuts down the write end of the pipe if we get to the end of the file.

@dcantah
Copy link
Contributor Author

dcantah commented Jul 10, 2025

@jglogan This hopefully should get piping a file in working, will test tomorrow. We'll need both this and bump of containerization that has the closing stdin work however.

@jglogan
Copy link
Contributor

jglogan commented Jul 12, 2025

@dcantah

Can you check to see if terminal I/O works for you in this branch with:

container run -it --rm alpine:latest sh

@dcantah
Copy link
Contributor Author

dcantah commented Jul 12, 2025

@jglogan It doesn't, it's why it's in draft right now. FileHandle.read(upToCount reads until EOF by default, so I'll need to write my own read(2) loop (and make stdin nonblocking).

@dcantah dcantah force-pushed the close-stdin-pipe branch 2 times, most recently from 7afeca1 to 1d7e6c3 Compare July 13, 2025 02:49
@dcantah
Copy link
Contributor Author

dcantah commented Jul 13, 2025

@jglogan tty fixed, and I can confirm we close the relay on eof for regular files. What's the avenue for testing against containerization main these days? It seem make init-block returns the version as unspecified

}
}

struct OSFile: Sendable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I've just copied a smaller portion of this same type we have for vminitd here. I'll see if it makes sense to make that public over in CZ, and we can just use directly.

@jglogan
Copy link
Contributor

jglogan commented Jul 13, 2025

What's the avenue for testing against containerization main these days?

What's missing from the build documentation is that you need to set up the default init image as vminit:latest, and then restart everything so that both the CLI and the API server pick up the correct default init image.

make all install
defaults write com.apple.container.defaults image.init vminit:latest
container system stop
container system start
container run -d --rm --name test python:slim python3 -m http.server --bind 0.0.0.0 8000
echo hello | container exec -t exec -i test cp /dev/stdin /tmp/hello.txt

FWIW I'm seeing that this seems to suffice for the exec test:

diff --git a/Sources/CLI/RunCommand.swift b/Sources/CLI/RunCommand.swift
index 9a7f0d7..1d3f0cc 100644
--- a/Sources/CLI/RunCommand.swift
+++ b/Sources/CLI/RunCommand.swift
@@ -218,6 +218,7 @@ struct ProcessIO {
                     let data = handle.availableData
                     if data.isEmpty {
                         pin.readabilityHandler = nil
+                        try? stdin.fileHandleForWriting.close()
                         return
                     }
                     try! stdin.fileHandleForWriting.write(contentsOf: data)

That we fetch the init filesystem in the client at https://github.com/apple/container/blob/main/Sources/ContainerClient/Utility.swift#L113, and then again when creating the container in the API server https://github.com/apple/container/blob/main/Sources/APIServer/Containers/ContainersService.swift#L165 is curious.

@dcantah dcantah marked this pull request as ready for review July 14, 2025 23:11
@dcantah dcantah requested a review from jglogan July 21, 2025 21:24
Copy link
Contributor

@jglogan jglogan left a comment

Choose a reason for hiding this comment

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

mainly readability nits

readabilityHandler is a bit crummy if it's a regular file.
This just writes our own loop and exits and shuts down the
write end of the pipe if we get to the end of the file.
@dcantah dcantah force-pushed the close-stdin-pipe branch from ecdb44d to a0518e6 Compare July 22, 2025 20:08
@dcantah dcantah merged commit c0d1f8f into apple:main Jul 22, 2025
2 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.

2 participants