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

[nix] Flake #119

Merged
merged 1 commit into from
Feb 17, 2023
Merged

[nix] Flake #119

merged 1 commit into from
Feb 17, 2023

Conversation

jbboehr
Copy link
Contributor

@jbboehr jbboehr commented Feb 5, 2023

Todo

  • all - can't export all the binaries because they collide - could move them to rosettaboy-<lang>-<build>. Should they even be in packages.default? Maybe add another packages.all? later
  • all - readme update
  • ci - hashFiles should probably run on flake.nix / ${{ inputs.working-dir}}/derivation.nix
  • nix - 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 🤷 later
  • nix - shim shell.nix and default.nix with flake-compat
  • go - gomod2nix is using my fork to avoid using an overlay - just use the overlay? - switched to manual/non-flake initialization to avoid introducing an overlay
  • go - currently failing format
  • utils - not packaged - could be used to run the entire test suite in nix 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 with overridePythonAttrs
  • py - default package is symlinking all the python code, not just the bin - just use makeWrapper? later
  • py - currently failing CI

Notes

  • all - don't have access to a Mac to test on it 🤷
  • nix - I regretfully deleted your beautiful and comprehensive comment in shell.nix because it's not quite relevant to flakes - not sure if you want to re-elaborate
  • go - will need to run gomod2nix if the dependencies are changed
  • nim - doesn't seem to have a *2nix, so nimble packages may get out of sync with nix
  • py - made some changes to setup.py and the source files to get it to build with buildPythonApplication
  • zig - I, again, regretfully deleted your comprehensive comment in zig/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 that looks like a Mac thing tried to pull in the previous sdl2-lowercase but untested.
  • utils - added blargg to flake checks but not currently using utils/blargg.py - the interface doesn't quite jive with the Nix directory structure
  • utils - changed blargg and bench to use roms managed by the nix flake by default, if in a dev shell

Default package with symlinkJoin:

$ nix build .
$ find result/bin/ | sort
result/bin/
result/bin/rosettaboy-c
result/bin/rosettaboy-cpp
result/bin/rosettaboy-go
result/bin/rosettaboy-nim
result/bin/rosettaboy-php
result/bin/rosettaboy-py
result/bin/.rosettaboy-py-wrapped
result/bin/rosettaboy-rs
result/bin/rosettaboy-zig

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:

$ nix path-info  -rsSh '.#c' | tail -3
/nix/store/wfi3pk89p0mzp1gb067by595nqlrpax2-pipewire-0.3.60-lib                    9.0M  539.8M
/nix/store/z4xwhv1zi3maa2dmvghvs2xmi45mhss3-SDL2-2.24.2                            2.3M  542.1M
/nix/store/jxiqfj8m8ybc6m367dmx58w4xw2mg8g4-rosettaboy-c                         283.4K  542.4M

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

@jbboehr jbboehr force-pushed the nix-flake branch 2 times, most recently from 91e5a0e to d6163f5 Compare February 5, 2023 20:28
@jbboehr jbboehr changed the title Nix Flake [nix] Flake Feb 5, 2023
@jbboehr jbboehr marked this pull request as ready for review February 8, 2023 07:28
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 8, 2023

@shish I think I've gotten it to a state where it's almost ready to merge. The only remaining blocker is it needs a touch-up on Mac since I don't have one - hopefully someone can make a pass at it. Everything else I think can be left as-is or addressed later.

cc @rrbutani

@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 8, 2023

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;

Copy link
Contributor

@rrbutani rrbutani left a 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!

cpp/derivation.nix Outdated Show resolved Hide resolved
cpp/derivation.nix Outdated Show resolved Hide resolved
flake.lock Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
zig/derivation.nix Outdated Show resolved Hide resolved
zig/derivation.nix Show resolved Hide resolved
.github/workflows/common.yml Outdated Show resolved Hide resolved
@rrbutani
Copy link
Contributor

rrbutani commented Feb 8, 2023

Fixed nim on macOS: rrbutani@812198a

Which just leaves PHP (fails during check on a particularly strange error with exec that I can't reproduce outside of the nix store 😕).

Edit: it's this issue; nix store vs not was a red herring (zsh quietly does the right thing).

@rrbutani
Copy link
Contributor

rrbutani commented Feb 8, 2023

Okay! Couple more fixes:

  • rrbutani@16c4517 (should maybe just invoke blargg.py instead; we can get access to the package from within mkDerivation with mkDerivation ({ finalPackage, ... }: ...)
  • rrbutani@85603fc

Here's all the changes together.

And now nix flake check passes on aarch64-darwin and x86_64-darwin.

Edit: I just tested nix flake check github:rrbutani/rosettaboy/nix-flake on aarch64-linux and x86_64-linux; those pass too. 🎉

@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 9, 2023

@rrbutani Thanks for checking that out! I should get a chance to work on integrating your changes soon.

@jbboehr jbboehr marked this pull request as draft February 9, 2023 02:12
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 9, 2023

@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.

Copy link
Contributor

@rrbutani rrbutani left a 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:


Thanks for pushing on this @jbboehr!

py/derivation.nix Show resolved Hide resolved
cpp/derivation.nix Outdated Show resolved Hide resolved
cpp/derivation.nix Outdated Show resolved Hide resolved
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 10, 2023

@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 setup.py earlier. I'm just not a python guy and haven't figured out how to remap it for mypycify. Or maybe just revert the package name mapping.

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

@jbboehr jbboehr force-pushed the nix-flake branch 2 times, most recently from a77707e to f601aec Compare February 10, 2023 05:07
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 10, 2023

@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

Run nix run .#zig -- --help
/home/runner/work/_temp/0844677d-b414-4746-aff7-294198b4b375.sh: line 1:  4585 Illegal instruction     (core dumped) nix run .#zig -- --help

(edit) Here's the exact same commit passing in my develop branch: https://github.com/jbboehr/rosettaboy/actions/runs/4141160177/jobs/7160484835 (/edit)

@jbboehr jbboehr marked this pull request as ready for review February 11, 2023 01:09
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 12, 2023

I think I got pxd packaged but there's something weird going on - exit code is ok though. Could be my fault.

$ /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

@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 12, 2023

@shish @rrbutani I think this is ready to go barring another check on Mac.

Some other things that could be done / general notes

  • nix - build docker images using Nix. I looked into it briefly but they're ~700MB on my machine, probably due to SDL pulling in lots of system level dependencies I noted above about the closure size (like pipewire, mesa, etc). prototype
  • nix - pre-commit hooks for the languages. It's a little tricky as mkShell merges the shell hooks.
  • nix - use makeWrapper, ln, or something like it instead of symlinkJoin to build the default package as only the binaries are required. Doesn't really matter a whole lot but would be a bit cleaner.
  • nix - decide whether to export the non-default builds (e.g. cpp-lto) in packages.default
  • nix - pick a formatter. I'm on the fence. Samples: alejandra nixpkgs-fmt.
  • nix - use utils/blargg.py to run blargg in nix flake check. It needs some changes to work with the nix directory structure and I'm just not a Python guy.
  • nix - expose a package and/or check to run utils/bench.py. It also needs some changes to work with the nix directory structure.
  • pxd - figure out what's up with the test output mentioned in [nix] Flake #119 (comment)
  • py - mypy takes about 15 minutes to compile on my machine and in CI. Might be better to disable the mypyc nix build until mypy is in nixpkgs (or set up cachix?). Could also disable compiling mypy with mypyc maybe see
  • zig - the illegal instruction issue has not returned 🎊

but I don't think any of these are blockers.

@will-ca
Copy link
Contributor

will-ca commented Feb 13, 2023

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.

@shish
Copy link
Owner

shish commented Feb 13, 2023

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 :) )

@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 14, 2023

@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.

@shish
Copy link
Owner

shish commented Feb 14, 2023

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 <language>/flake.nix doesn't exist 🤔 (Adding a boolean config option in common.yml works, but being totally automatic would be nicer)

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

@will-ca
Copy link
Contributor

will-ca commented Feb 14, 2023

Personally, I see value in declaring build instructions in a reproducible manner, which is why I switched to NixOS ~6 years ago.

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.

@jbboehr jbboehr force-pushed the nix-flake branch 2 times, most recently from c0ee188 to 4ca6c18 Compare February 15, 2023 03:08
Co-authored-by: John Boehr <[email protected]>
Co-authored-by: Rahul Butani <[email protected]>
@jbboehr
Copy link
Contributor Author

jbboehr commented Feb 15, 2023

In my latest commit I:

  • Squashed the entire branch - it was getting out of hand and most of the non-Nix commits have been merged into master already
  • Brought back the old shell.nix for each language (except c) 🤷
  • Split Nix into two (three technically) workflows which check for the presence of a shell.nix or derivation.nix and run the shell-only checks or flake-only checks respectively.
  • Added support for [skip format] [skip build] [skip nix] [skip nix shell] [skip nix flake] to the workflows - feel free to remove of course

@shish shish merged commit 2d8ed9f into shish:master Feb 17, 2023
@shish
Copy link
Owner

shish commented Feb 17, 2023

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/

@will-ca will-ca mentioned this pull request Sep 29, 2023
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.

nix flakes
4 participants