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

Make build on Windows #6

Merged
merged 5 commits into from
Mar 28, 2022
Merged

Make build on Windows #6

merged 5 commits into from
Mar 28, 2022

Conversation

nico
Copy link
Contributor

@nico nico commented Mar 25, 2022

Gets things to build and kinda work, but things are a bit janky:

  • The generate_build_file integration tests fails (but all other tests pass), because it depends on sh which doesn't exist on windows. I disabled it for now.
  • output printing looks funny. maybe just calling SetConsoleMode(..., ENABLE_VIRTUAL_TERMINAL_PROCESSING) is enough, but ninja has a comment on how printing via WriteConsoleOutput is less janky, so maybe we'll want to do that.

Actually running n2 complained that /bin/sh doesn't exist. It looks like std::process::Command doesn't have a way to just pass through a string to CreateProcess. I asked about this here rust-lang/rust#92939 (comment) , but https://github.com/rust-lang/rust/blob/master/library/std/src/sys/windows/process.rs#L277 suggests Rust really wants user code to split apart the string (which on windows in general is impossible, since it's the job of every program to do args splitting itself, so they can all do different things and the launcher process can't generally know how every program does it), only to assemble it again in the stdlib. So we have to call CreateProcess and WaitForSingleObject etc ourselves. This part isn't 100% complete, it drops the output of the subprocess on the floor at the moment. But still, simple builds work.

(The current threads + std::process::Command approach seems to cause file leaks, possibly due to the pipe/CLOEXEC race, so maybe we'll move off that at some point.)

@nico
Copy link
Contributor Author

nico commented Mar 25, 2022

Added some first janky CreateProcessA code.

Starts subprocesses, but drops their output for now.
Basic builds work!
nico added 2 commits March 25, 2022 14:16
This test relies heavily on `sh`, which doesn't exist on Windows.
@evmar evmar merged commit c43bd56 into evmar:main Mar 28, 2022
@evmar
Copy link
Owner

evmar commented Mar 28, 2022

Wow, thanks!

One thing I've been thinking about is whether the tests ought to hook into the executable rather than calling it externally. That would allow making a custom command runner that understood commands like "touch" maybe. But the downside is less code is covered by the test, so I'm not sure.

My WIP was https://github.com/evmar/n2/compare/test-fs , mostly just threading a "filesystem" object around. I'm still not sure if it's actually better though.

@evmar
Copy link
Owner

evmar commented Mar 28, 2022

Oh also I added Windows on CI so I hopefully don't break this immediately...

@nico nico deleted the win branch March 28, 2022 11:22
@nico
Copy link
Contributor Author

nico commented Mar 28, 2022

Thanks for setting up CI.

I kind of like the current test setup for integration tests, since it makes sure the tests are representative of the real thing. I've sometimes not written tests in ninja land because some things could only be tested well with a setup like this, so I think it's good that n2 does have this setup.

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

Successfully merging this pull request may close these issues.

2 participants