Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add dial-stdio command #2112
Add dial-stdio command #2112
Changes from all commits
760244e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we extract the logic for dial-stdio in buildkit into a reusable function so we could just call that here after getting the
conn
? Ideally, we wouldn't end up with diverging implementations in buildctl/buildx (e.g. that would make it a pain to fix bugs in the future, etc).We're also having the same thing in dagger, where we need
dial-stdio
for the connhelper, but don't really want to ship all ofbuildctl
, so it would be cool to have just one implementation to call instead of needing to extract the relevant code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One implementation of what exactly?
the main part of this is just wiring up copy operations.
I'm not sure its worth sharing an implementation of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I'm just confused by the differences in handling
CloseRead
/CloseWrite
here versus in buildctl. Given that they seem to be doing the same thing, I'm not quite sure I understand why the code is so different.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AkihiroSuda I'm curious on this case where its wrapping with a nop-closer.https://github.com/moby/buildkit/blame/5ae9b23c40a926615b6c3cc1da3f5457d2f61fd4/cmd/buildctl/dialstdio.go#L35
I don't think we necessarily need the stream to handle a half-close here, just that it s a little cleaner in terms of the connection semantics (hence why its implemented here).
Other than that I think the implementation is pretty much the same, just this is taking advantage of errgroup and buildctl is basically doing its own hand-rolled equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my memory, tty stuff didn't work correctly without handling half-close there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to test for using tty as stdin/stdout but this broke http2.
Not sure how to check for this.
Fundamentally though, the only real change as compared to buildctl would be to not fallback to
Close
on whenCloseRead
is not implemented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a bit more description/usage here. It isn't entirely obvious for new user what "dial stdio" means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added extra docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what changed for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
I wrote stuff... apparently I did not commit it somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed up again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I know what happened... I did docs the wrong way.