-
Notifications
You must be signed in to change notification settings - Fork 22
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
[nix] Flake #119
[nix] Flake #119
Conversation
91e5a0e
to
d6163f5
Compare
These are the current cases of Mac-specific behavior: $ rg isDarwin
zig/derivation.nix
45: ++ lib.optional (!stdenv.isDarwin) SDL2
46: ++ lib.optional stdenv.isDarwin SDL2-lowercase
50: ++ lib.optional (!stdenv.isDarwin) autoPatchelfHook
rs/derivation.nix
17: devTools = [ rustfmt rustc cargo ] ++ lib.optional stdenv.isDarwin [libiconv];
nim/derivation.nix
41: ++ lib.optional (!stdenvNoCC.isDarwin) sdl2;
c/derivation.nix
30: ++ lib.optional (!stdenv.isDarwin) autoPatchelfHook;
utils/derivation.nix
30: ++ lib.optional (!stdenvNoCC.isDarwin) elfutils;
cpp/derivation.nix
22: ++ lib.optional (!stdenv.isDarwin) autoPatchelfHook; |
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.
@jbboehr This is looks really good!
I left some suggestions and some notes about fixes I had to apply to get things to build on macOS. The GitHub PR suggestion machinery is a little annoying to use to write/review longer suggested changes so I starting making commits for them on this branch and I've linked the relevant commits where appropriate; feel free to cherry-pick anything you think is useful!
nix flake check
still isn't totally clean for me on aarch64-darwin
and x86_64-darwin
(nim and php have build failures) but this is definitely great progress!
Fixed Which just leaves PHP (fails during check on a particularly strange error with Edit: it's this issue; nix store vs not was a red herring ( |
Okay! Couple more fixes:
Here's all the changes together. And now Edit: I just tested |
@rrbutani Thanks for checking that out! I should get a chance to work on integrating your changes soon. |
@rrbutani I've integrated a fair number of your changes but ran into some issues with zig and mypyc. Might've borked the merge - I should get a chance to try again soon. |
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.
Couple of nits; nothing major. I think only these commits remain unmerged:
-
mypyc
fixes: rrbutani@199e73d - update the zig toolchain to pick up
NIX_CFLAGS_COMPILE
fixes for macOS: rrbutani@b74e957 - zig
derivation.nix
fixes for macOS: rrbutani@1366315 - drop the zig SDL2 fix: rrbutani@0d3709e (optional)
- rrbutani@16c4517 (optional)
Thanks for pushing on this @jbboehr!
@rrbutani I managed to get your zig changes working, I'll pull them into the PR soon. I pulled in your mypyc changes [branch] but it seems like it's not using mypyc: $ nix run .#py-mypyc -- --headless --frames 100 $GB_DEFAULT_BENCH_ROM
Emulated 100 frames in 11.85s (8fps)
$ nix run .#py -- --headless --frames 100 $GB_DEFAULT_BENCH_ROM
Emulated 100 frames in 12.34s (8fps) mypy takes about 15 minutes to compile on my machine. Might be better to wait until it's in nixpkgs. It might be my fault as I had tried remapping the package name in Result Directory$ nix build -L .#py-mypyc
$ find result/lib/python3.11/site-packages
result/lib/python3.11/site-packages
result/lib/python3.11/site-packages/14f66929a8af613578ab__mypyc.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/METADATA
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/RECORD
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/INSTALLER
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/direct_url.json
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/top_level.txt
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/REQUESTED
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/WHEEL
result/lib/python3.11/site-packages/rosettaboy-0.0.0.dist-info/entry_points.txt
result/lib/python3.11/site-packages/src
result/lib/python3.11/site-packages/src/ram.cpython-311-x86_64-linux-gnu.so <- named src here
result/lib/python3.11/site-packages/src/main.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/consts.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/gameboy.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/clock.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/cpu.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/buttons.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/args.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/__init__.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/gpu.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/cart.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/src/errors.cpython-311-x86_64-linux-gnu.so
result/lib/python3.11/site-packages/rosettaboy
result/lib/python3.11/site-packages/rosettaboy/buttons.py <- named rosettaboy here
result/lib/python3.11/site-packages/rosettaboy/errors.py
result/lib/python3.11/site-packages/rosettaboy/consts.py
result/lib/python3.11/site-packages/rosettaboy/gpu.py
result/lib/python3.11/site-packages/rosettaboy/gameboy.py
result/lib/python3.11/site-packages/rosettaboy/args.py
result/lib/python3.11/site-packages/rosettaboy/cart.py
result/lib/python3.11/site-packages/rosettaboy/clock.py
result/lib/python3.11/site-packages/rosettaboy/main.py
result/lib/python3.11/site-packages/rosettaboy/cpu.py
result/lib/python3.11/site-packages/rosettaboy/__pycache__
result/lib/python3.11/site-packages/rosettaboy/__pycache__/errors.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/ram.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/consts.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/cpu.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/gpu.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/cart.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/clock.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/main.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/__init__.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/buttons.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/gameboy.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/__pycache__/args.cpython-311.pyc
result/lib/python3.11/site-packages/rosettaboy/ram.py
result/lib/python3.11/site-packages/rosettaboy/__init__.py |
a77707e
to
f601aec
Compare
@rrbutani This was the issue I was having earlier with zig, seems to be intermittent because the CI just passed earlier with no changes. AFAICT started happening since the toolchain was updated but hard to tell. Might be better to revert to an older release. Maybe it's cache related, I don't know. See: https://github.com/shish/rosettaboy/actions/runs/4141038600/jobs/7160260155
(edit) Here's the exact same commit passing in my develop branch: https://github.com/jbboehr/rosettaboy/actions/runs/4141160177/jobs/7160484835 (/edit) |
I think I got $ /nix/store/wpkn4j3jv25057sncr248ppvdr0irfdg-rosettaboy-pxd/bin/rosettaboy-pxd --turbo --silent --headless --frames 200 /nix/store/gmm7glcs2qsfpvdvrnkw6ci1hqkidf11-source/blargg-cpu-instructions/04-op_r,imm.gb && echo ok
04-op r,imm
Passed
Traceback (most recent call last):
File "src/cpu.py", line 468, in src.cpu.CPU.tick_instructions
File "src/cpu.py", line 1068, in src.cpu.CPU.tick_main
src.errors.UnitTestPassed: Unit test passed
Exception ignored in: 'src.cpu.CPU.tick'
Traceback (most recent call last):
File "src/cpu.py", line 468, in src.cpu.CPU.tick_instructions
File "src/cpu.py", line 1068, in src.cpu.CPU.tick_main
src.errors.UnitTestPassed: Unit test passed
Traceback (most recent call last):
File "src/cpu.py", line 468, in src.cpu.CPU.tick_instructions
File "src/cpu.py", line 1070, in src.cpu.CPU.tick_main
src.errors.UnitTestFailed: Unit test failed
Exception ignored in: 'src.cpu.CPU.tick'
Traceback (most recent call last):
File "src/cpu.py", line 468, in src.cpu.CPU.tick_instructions
File "src/cpu.py", line 1070, in src.cpu.CPU.tick_main
src.errors.UnitTestFailed: Unit test failed
Emulated 200 frames in 0.78s (256fps)
ok |
@shish @rrbutani I think this is ready to go barring another check on Mac. Some other things that could be done / general notes
but I don't think any of these are blockers. |
To be honest, though I contributed the original Nix shells, as an outside observer at this point— Not to dismiss what Nix Flakes can do, nor that it's cool that they can do that.— This feels like a lot of work and increased maintenance and barrier to entry for marginal practical gain. E.G. Just speaking on my personal experience, I suspect if I had seen this at the time, it may have intimidated me enough to deter me from contributing the Cythonized implementation. |
Complexity of the nix bits is certainly something to think about; but also this PR seems to have a bunch of stuff that doesn't all need to happen in one go, which makes it harder to understand what's being changed -- I've cherry-picked most of the independent parts (updates to utils scripts, allowing github actions to specify nim or zig versions, rebuilding everything when common.yml changes, etc etc); once that's done I'll have a look at cherry-picking the "use zig 0.11" bits, which should also be independent but slightly trickier to cherry-pick; then hopefully what's left after that is just the flakes by themselves, and fingers crossed the flakes by themselves are less intimidating (I confess that I too don't really understand them, hence getting everything-except-the-flakes merged first :) ) |
@will-ca Virtually all the complexity is in building the packages in Nix, the Nix way (TM). @rrbutani's original 3 commits were sufficient for just the devShells. Personally, I see value in declaring build instructions in a reproducible manner, which is why I switched to NixOS ~6 years ago. I was initially hesitant to upgrade to flakes, myself, them still being "experimental", but made the switch a few months ago and have been very pleased. No more of this nonsense. In my personal projects, I don't expect drive-by contributors to work on the Nix parts, but this is @shish's project so he'll have to make the call on whether or not he wants to do that. Could also mark the Nix builds with continue-on-error, I suppose. |
Thinking along similar lines, I wonder if it's possible to skip the nix parts completely if My current high-level thinking is that I like all the nice things that come with nix and flakes, and I'm gradually starting to use them for all my personal development-environment-setup tasks, but I wouldn't want nix expertise to be required at any point -- ideally the only barrier to entry for contributing a new implementation should be familiarity with that language + enough git knowledge to create a pull request |
I think I see the value in theory, but in practice I'm not sure it's worth the cost of making the build process much harder to understand/more intimidating for anyone without that relatively specialized knowledge of "The Nix Way". If Nix Flakes were the standard workflow, then presumably we would be horrified at the thought of doing things the non-reproducible way. But as it is, the non-reproducible workflow is the standard, so any deviation from that, even if strictly better on technical merits, also carries a logistical cost in relation to the rest of the ecosystem. I can only read so many API docs in a day, and while a couple hundred lines of Nix is certainly better than fighting with disparate distros and build systems, at some point I feel it would start to eat into the resources for writing application code and deploying on non-Nix systems. …Nix sans Flakes is both innovative and a large practical improvement over the old way; Nix Flakes may be just as innovative but it feels like less of a relative practical improvement compared to Nix sans Flakes (mainly because Nix sans Flakes is already quite good in most cases). 'Just my two cents. Innovation vs. convention, etc. I'm not arguing for or against Flakes either in general or in this PR; I'm just confusedly trying to understand the landscape of what is still a very exotic way of structuring a Unix-style computing environment. |
c0ee188
to
4ca6c18
Compare
Co-authored-by: John Boehr <[email protected]> Co-authored-by: Rahul Butani <[email protected]>
In my latest commit I:
|
This all seems to work for me, and my brain-buffer is full up from trying to keep track of wide-spread changes - so I'm merging now, and we can fix forward with smaller PRs for future work /o/ |
Todo
all
-can't export all the binaries because they collide - could move them tolaterrosettaboy-<lang>-<build>
. Should they even be inpackages.default
? Maybe add anotherpackages.all
?all
- readme updateci
- hashFiles should probably run on flake.nix / ${{ inputs.working-dir}}/derivation.nixnix
-could use a nix formatter. Personally, I've been using nixpkgs-fmt but there are a few others. Currently leaning towards alejandra but they both do things I don't care for and are completely unconfigurable by design 🤷laternix
- shimshell.nix
anddefault.nix
with flake-compatgo
-gomod2nix is using my fork to avoid using an overlay - just use the overlay?- switched to manual/non-flake initialization to avoid introducing an overlaygo
- currently failing formatutils
- not packaged - could be used to run the entire test suite innix flake check
pxd
- not packaged - I gave up for now. devShell still works though.py
- mypyc build isn't working - needs an untagged commit from mypy master. Couldn't get it to work withoverridePythonAttrs
py
-default package is symlinking all the python code, not just the bin - just use makeWrapper?laterpy
- currently failing CINotes
all
- don't have access to a Mac to test on it 🤷nix
- I regretfully deleted your beautiful and comprehensive comment inshell.nix
because it's not quite relevant to flakes - not sure if you want to re-elaboratego
- will need to rungomod2nix
if the dependencies are changednim
- doesn't seem to have a*2nix
, so nimble packages may get out of sync with nixpy
- made some changes tosetup.py
and the source files to get it to build withbuildPythonApplication
zig
- I, again, regretfully deleted your comprehensive comment inzig/shell.nix
as it's not quite relevant to flakes - not sure if you want to re-elaborate. SDL2 lowercase doesn't seem to be an issue in CI or locally on NixOS though,not sure what's up with thatlooks like a Mac thingtried to pull in the previous sdl2-lowercase but untested.utils
- added blargg to flake checks but not currently usingutils/blargg.py
- the interface doesn't quite jive with the Nix directory structureutils
- changed blargg and bench to use roms managed by the nix flake by default, if in a dev shellDefault package with
symlinkJoin
:Closure size
This might only be an issue on my system due to using pipewire but the closure size for the built binary is a bit large right now:
I might see if static linking SDL2 can keep the final closure size down at some point in the future.Looks like it just needs to link into a lot of system-level dependencies for hardware support SDL2.Closes #111