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

remote: use winio DialPipeContext for named pipes #2287

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

iankingori
Copy link
Contributor

@iankingori iankingori commented Feb 27, 2024

This PR fixes an issue where the Dialer in go does not support named pipes which the buildkit daemon on Windows uses. I've added a utility DialContext function that uses DialPipeContext for npipe connections on windows.

@jedevc
Copy link
Collaborator

jedevc commented Feb 27, 2024

Just a note: this seems similar to moby/buildkit#4127.

If DialContext doesn't support named pipes, then maybe the right call is to pull that upstream change in, and fix with that (it creates a connhelper for npipe).

Edit: ignore me, I saw #2286 (not quite sure why it was closed and replaced), and it was a bit clearer. Related but not the same - we explicitly choose to use DialContext in buildx, so we would need to explicitly use winio.DialPipeContext if we want that here.


Future note - could you link between PRs/issues/etc when they're all related to make it a bit easier to navigate between - I hadn't quite realized this was connected to #2250 (comment).

@tonistiigi
Copy link
Member

@iankingori Check the linter error and remove draft if ready.

@tonistiigi tonistiigi added this to the v0.13.0 milestone Feb 27, 2024
@iankingori iankingori marked this pull request as ready for review February 27, 2024 19:39
@iankingori
Copy link
Contributor Author

iankingori commented Feb 27, 2024

Thanks @jedevc, I'll drop some more context on the next one.

I'm creating an issue to track remote driver tests for windows

@iankingori
Copy link
Contributor Author

The test failure looks like a flake, previous runs passed and only change since was a linting error.

Signed-off-by: Ian King'ori <kingorim.ian@gmail.com>
@tonistiigi tonistiigi merged commit 8db86e4 into docker:master Feb 28, 2024
63 checks passed
@tianon-sso
Copy link
Member

tianon-sso commented Feb 28, 2024

Can confirm this fixes the problem I had in #2250 (comment)!! Thanks all! 😄 ❤️

$ ~/git/buildx/buildx.exe --builder windows du
Reclaimable:    0B
Total:          0B

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants