Skip to content

Enable the unix:// store on Windows#10556

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
nix-windows:uds-remote-on-windows
May 2, 2024
Merged

Enable the unix:// store on Windows#10556
Ericson2314 merged 2 commits intoNixOS:masterfrom
nix-windows:uds-remote-on-windows

Conversation

@Ericson2314
Copy link
Member

Motivation

Building nix daemon on Windows I've left for later, because the daemon currently forks per connection but this is not an option on Windows. But we can get the client part working right away.

Context

Reviewers: do double check the random -> rand switch. The long term solution is still #10541 since that library is supposed to be better for many reasons.

Windows now has some basic Unix Domain Socket support, see https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/

Priorities and Process

Add 👍 to pull requests you find important.

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

I don't think fewer bits matters for this, and `rand` but not `random`
is available on Windows.
Windows now has some basic Unix Domain Socket support, see
https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/

Building `nix daemon` on Windows I've left for later, because the daemon
currently forks per connection but this is not an option on Windows. But
we can get the client part working right away.
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Apr 18, 2024
@Ericson2314 Ericson2314 changed the title Uds remote on windows Enable the unix:// store on Windows Apr 18, 2024
Copy link
Member

@puffnfresh puffnfresh left a comment

Choose a reason for hiding this comment

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

Looks good. Code has mostly just been moved around. I don't know whether srandom and srand have any internal relationship but seems fine to me.

@puffnfresh
Copy link
Member

I wonder if we can expose a socket from WSL. With this PR we'd then have nix.exe control WSL builds, which would be pretty cool!

@Ericson2314
Copy link
Member Author

Great point! If not, we can always use TCP #5265

@Ericson2314
Copy link
Member Author

I decided rand() vs random() should be fine because the random file name is just temporary. Glibc supposedly makes them the same, but the Apple man page implies rand() is much worse (e.g. cyclic lower order bits). This is would be very bad for e.g statics or cryptography applications, but is fine for this sort of temporary temp file (when honestly a global counter + pid would probably suffice).

/**
* Bind a Unix domain socket to a path.
*/
void bind(Socket fd, const std::string & path);
Copy link
Member

Choose a reason for hiding this comment

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

Won't "public" use of types like Socket and Descriptor cause type errors that are basically only caught in CI? (When forgetting to call toSocket, for example.)

I'd prefer a distinct type so that the compiler helps us get it right.

struct Socket {
#ifdef _WIN32
  SOCKET socket;
#else
  int socket;
#endif
};

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you feel about this (for both types) being a follow up PR? I agree it is better, but I was wary of the "but now Unix needs to jump through more hoops" counter argument.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-04-22-nix-team-meeting-minutes-141/44083/1

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

LGTM, modulo types for followup. 👍

#include <cstring>

#ifdef _WIN32
# include <winsock2.h>
Copy link
Member

@qknight qknight Apr 24, 2024

Choose a reason for hiding this comment

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

# include vs. #include, i like it to be consistent, this happens multiple times in multiple documents. i wonder is this a _WIN32 thing we want to have?

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked the indentation to make nested if..endif easier to follow

Copy link
Member

Choose a reason for hiding this comment

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

modern IDEs detect the _WIN32 switch an grey out others but so far didn't manage this in my visual studio code ssh IDE to pick up the compiler settings. for clion this required cmake to work properly and for other build systems at least one complete compile run and then IDEs sometimes pick up these settings.

this level of support would be nice for nix hacking!

Copy link
Member

Choose a reason for hiding this comment

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

@qknight We have instructions for the clangd LSP if that's of use to you.
In support of this, we have make compile_commands.json. Maybe CLion supports that format too?

bear or similar isn't needed anymore since recently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that clang-format is configured to do this sort of indentation now.

Copy link
Member

@qknight qknight May 7, 2024

Choose a reason for hiding this comment

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

@Ericson2314 @roberth interesting! So i would have to use the stdenv.cc.isClang for formatting? Or would I install nix-env -i clang-format into my shell before nix develop .#devShells.x86_64-linux.x86_64-w64-mingw32?

  nativeBuildInputs = attrs.nativeBuildInputs or []
    # TODO: Remove the darwin check once
    # https://github.com/NixOS/nixpkgs/pull/291814 is available
     ++ lib.optional (stdenv.cc.isClang && !stdenv.buildPlatform.isDarwin) pkgs.buildPackages.bear
     ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) pkgs.buildPackages.clang-tools;

Tried to get my visual studio code setup to work with make compile_commands.json but the problem is that I would need to make the login shell (it uses ssh to attach to the machine) have what the nix develop ... brings in and I couldn't make this happen. There is no field in the configuration, https://code.visualstudio.com/docs/remote/ssh, where one could set env vars or commands to run prior.

Copy link
Member Author

@Ericson2314 Ericson2314 May 7, 2024

Choose a reason for hiding this comment

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

Just run make format, it is in all of Nix's dev shells.

#ifndef _WIN32
srandom(tv.tv_usec);
#endif
srand(tv.tv_usec);
Copy link
Member

Choose a reason for hiding this comment

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

Should it not be:

#ifndef _WIN32
    srandom(tv.tv_usec);
#else
    srand(tv.tv_usec);
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it use rand on all platforms in the one spot above, so srand is likewise needed: on Unix we now use all 4 functions, on Windows we use two.

@Ericson2314 Ericson2314 merged commit 1948ec3 into NixOS:master May 2, 2024
@Ericson2314 Ericson2314 deleted the uds-remote-on-windows branch May 2, 2024 13:53
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request May 3, 2024
Fix formatting violations, update blacklist to reflect moved files.

PR NixOS#10556 passed CI before the new formating rules were added, and our
CI has the race condition of allowing old results, resulting in master
getting broken.
@Ericson2314 Ericson2314 mentioned this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store windows

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants