Skip to content

Windows fixes#14817

Merged
Ericson2314 merged 3 commits intomasterfrom
fix-socket-mingw
Dec 18, 2025
Merged

Windows fixes#14817
Ericson2314 merged 3 commits intomasterfrom
fix-socket-mingw

Conversation

@Ericson2314
Copy link
Member

Motivation

See each commit for details

Context

This will hopefully fix msys2/MINGW-packages#22499 . Clang had Identified we were missing a cast between HANDLE and SOCKET.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314 Ericson2314 added the backport 2.33-maintenance Automatically creates a PR against the branch label Dec 17, 2025
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Dec 17, 2025
@Ericson2314
Copy link
Member Author

Both before and after this PR, select-ing standard input on Windows doesn't actually work, but this does at least get the types right for that, and I think that still counts for something.

Because socket.hh is included in unix-domain-socket.hh, this is not a breaking API change, and thus something we're able to backport.

We don't have the dirFd on window at this time.
There are other types of sockets.
Comment on lines +440 to +456
Socket fromSock = toSocket(from), stdinSock = getStandardInput();
auto nfds = std::max(fromSock, stdinSock) + 1;
while (true) {
fd_set fds;
FD_ZERO(&fds);
FD_SET(from, &fds);
FD_SET(STDIN_FILENO, &fds);
FD_SET(fromSock, &fds);
FD_SET(stdinSock, &fds);
if (select(nfds, &fds, nullptr, nullptr, nullptr) == -1)
throw SysError("waiting for data from client or server");
if (FD_ISSET(from, &fds)) {
if (FD_ISSET(fromSock, &fds)) {
auto res = splice(from, nullptr, STDOUT_FILENO, nullptr, SSIZE_MAX, SPLICE_F_MOVE);
if (res == -1)
throw SysError("splicing data from daemon socket to stdout");
else if (res == 0)
throw EndOfFile("unexpected EOF from daemon socket");
}
if (FD_ISSET(STDIN_FILENO, &fds)) {
if (FD_ISSET(stdinSock, &fds)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically won't really work on windows, since select is socket-only, but at we'll probably need a better primitive to abstract away this kind of operation for pipes.

These functions use `SOCKET` not `int`, despite them being unix
functions.
Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Makes the usage slightly more correct. Would be nice to have unit tests for this though.

@Ericson2314 Ericson2314 added this pull request to the merge queue Dec 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 17, 2025
@Eveeifyeve
Copy link
Member

@xokdvium Windows unit tests will be done once it's fully working at the moment our goal is just to get it working and then worry about unit-tests later on.

@Ericson2314 Ericson2314 added this pull request to the merge queue Dec 18, 2025
Merged via the queue into master with commit 188cb79 Dec 18, 2025
20 checks passed
@Ericson2314 Ericson2314 deleted the fix-socket-mingw branch December 18, 2025 01:15
@internal-nix-ci
Copy link

Backport failed for 2.33-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.33-maintenance
git worktree add -d .worktree/backport-14817-to-2.33-maintenance origin/2.33-maintenance
cd .worktree/backport-14817-to-2.33-maintenance
git switch --create backport-14817-to-2.33-maintenance
git cherry-pick -x 30cd9e43e1bbf0ccb8d4e131988ac9eeb6e3dab3 79750a3ccc103f670a3db72bddf64aeb576ef36b 208ed3c538e9ea2f4027b6976c4f689ba28035fd

@xokdvium
Copy link
Contributor

our goal is just to get it working and then worry about unit-tests later on.

Well, how does you know if it works or not without unit tests? When it comes to this sort of sneaky OS API usage its imposssible to get right consistently without any unit tests

@Eveeifyeve
Copy link
Member

Eveeifyeve commented Dec 18, 2025

our goal is just to get it working and then worry about unit-tests later on.

Well, how does you know if it works or not without unit tests? When it comes to this sort of sneaky OS API usage its imposssible to get right consistently without any unit tests

Like right when I tested it doesn't even know the file format of it compiling via debuging via gcc.

Like try testing nix-instantiate --eval --expr "1+1" and it will just quit. In gdb debug it tells us it doesn't know the file format. Once we have a working nix we will start adding the unit-tests.

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

Labels

backport 2.33-maintenance Automatically creates a PR against the branch new-cli Relating to the "nix" command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants