-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Build a minimized Nix with MinGW #8901
Conversation
Oh yes it does! If we could get a cygwin (and msys2) build in CI that would be a great first step. |
I've never had much trouble building nix on cygwin. What I did have a lot of trouble with is fibers/coroutines, which are used by nix via boost. They are sort of fundamentally broken in cygwin, which causes nix to crash (e.g. on network operations). I actually proposed a patch to cygwin which allows them to work, but it hasn't been merged: https://cygwin.com/pipermail/cygwin-developers/2020-September/011970.html Last time I did any work on this, it was mostly on the nixpkgs side. The idea was to be able to cross compile a patched cygwin1.dll, and use that to bootstrap, and build up from there. Unfortunately I haven't had time to work on it for a while. I gave up on bootstrapping using unmodified cygwin. |
🎉 All dependencies have been resolved ! |
21abf88
to
1aaf36b
Compare
🎉 All dependencies have been resolved ! |
I think we'll want to install the unit tests into a separate output so we can run them on windows. |
1ff607e
to
c9f5d8a
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-11-17-nix-team-meeting-minutes-104/35753/1 |
Just curious, what is the oldest version of Windows this supports? (Also I haven't looked too closely at the code but related: if you're touching winapi you might want to enable UTF-8 mode so you don't have to touch wchar_t which I think is only supported on Windows 10 and up) |
Don't know!
Does that support ill-formed UTF-16 the way https://simonsapin.github.io/wtf-8/ does? In any event the plan for paths (main place this comes up) is using |
No, it seems to be actual UTF-8. What it does when you explicitly call the WideCharToMultiByte function it depends on whether the MB_ERR_INVALID_CHARS is set:
I assume the default with conversions in Windows's own functions is that it isn't set, no idea though. For paths I would also just use std::filesystem::path so you don't have to worry about that though, yeah. |
61b74e5
to
78ff3a1
Compare
@@ -151,11 +153,11 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl> | |||
{ | |||
initLibGit2(); | |||
|
|||
if (pathExists(path.native())) { | |||
if (git_repository_open(Setter(repo), path.c_str())) | |||
if (pathExists(path.string())) { |
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'm not entirely clear on the difference between native()
and string()
, but the docs for string()
say:
Otherwise, if path::value_type is wchar_t, conversion, if any, is unspecified. This is the case on Windows, where wchar_t is 16 bit and the native encoding is UTF-16.
which seems worrying.
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 think the more we use std::filesystem::path
the more it will go away --- for now I am fine with it because it has no effect on Unix and it gets Windows at least compiling.
AutoCloseFD fd = toDescriptor(open(path.c_str(), O_RDONLY | ||
// TODO | ||
#ifndef _WIN32 | ||
| O_CLOEXEC |
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.
Maybe we can define O_CLOEXEC as 0
on Windows in a header? Are filehandles created by open()
inherited by default on Windows? We could use CreateFile()
which returns a handle that isn't inherited by default.
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.
Yeah I hope to go to CreateFile
and side-step this issue longer term. (As the original Windows port did.) Just didn't do that for now to minimize CPP / windows-specific code for the first version.
|
||
auto [gens, curGen] = findGenerations(globals.profile); | ||
|
||
RunPager pager; | ||
|
||
for (auto & i : gens) { | ||
#ifdef _WIN32 // TODO portable wrapper in libutil |
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.
This could probably be made portable using std::chrono
, though the time zone stuff is C++20.
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'll make this a good-first-issue follow-up task, since it is localized.
This keeps the call sites simple, eventually this should be filled in.
At this point many features are stripped out, but this works: - Can run libnix{util,store,expr} unit tests - Can run some Nix commands Co-Authored-By volth <[email protected]> Co-Authored-By Brian McKenna <[email protected]>
We don't need it now that our (minimized) Windows build of Nix succeeds!
This is a hacky solution, but it will do for now.
Yay this is finally approved! Fixed what things I could from the review. The rest I will create issues for. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Motivation
Chasing my white whale of Nix on Windows again. Based off of @volth's great work.
At this point many features are stripped out, but this works:
Context
Depends on #8887Depends on #8920Depends on #8886Depends on #9518Depends on #9519Depends on #9736Depends on #9737Depends on #9757Depends on #9767Depends on #10329Depends on #10361Depends on #10362Depends on #10363Depends on #10364Depends on #10399Depends on #10400Depends on #10401Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.