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 development environment #100

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

ProducerMatt
Copy link
Contributor

@ProducerMatt ProducerMatt commented Mar 24, 2024

This is a Nix dev environment that you can run on any OS with Nix installed. You could accept the PR, but if you don't want the Nix files in it, you can just check out my branch until you track down the bug. :)

Usage:

  • If you have Nix and DirEnv installed, the dev environment will load automatically. Without Direnv, run nix develop .# and get a shell (more sandboxed than the direnv one, occasionally this helps with bugs).
  • execute tere directly with nix run .# -- (args)
  • load a shell with tere command in it with nix shell .#
  • just build/test it once with nix build .#.

File meanings: All the config for the dev environment is kept in flake.nix. flake.lock is what makes it reproducible, it will always load the same Rust toolchain until you update it with nix flake update.

In revision 5a03e56 I removed 3 optional files if you're interested. They're 2 files for Nix backwards compatibility, and one with a GitHub workflow for automatic package building.

The failures: Something about Nix's buildRustPackage is leading to the failures (the Arch builds are failing in a similar way). You'll notice there's no failure when testing in the dev shell with cargo test, or in my earlier revision 5a03e56 that used naersk's Rust builder.

Note, I tried both dontUseCargoParallelTests = true; and checkType = "debug";, neither fixed the test failures. The docs for buildRustPackage are here, and (should things get really dire) the source code is here, preferably that won't be needed at all :)

@mgunyho
Copy link
Owner

mgunyho commented Mar 24, 2024

Thanks for this! I took a quick look, and indeed it seems like the integration tests pass if I run with cargo test but fail if I run with nix build. I didn't have enough time to wrap my head around what nix build actually does to try and figure out the difference. Could it be that the tere subprocess launched by the integration test can't create temporary files/directories in the isolated environment for some reason?

Note that with this PR, the tests fail at case_sensitive_mode_change, which I have now confirmed to be fixed by 81036ef (although unsure why exactly). If you rebase on to 81036ef (or preferably develop), that test passes and then the integration tests fail.

@ProducerMatt
Copy link
Contributor Author

According to this Rust's tempdir() sources from $TMPDIR, and Nix sets that. Sounds right so far.

@ProducerMatt
Copy link
Contributor Author

Hey there. I don't know Rust but I kicked around with the errors, putting println statements like a noob. It's a direct Rust IO error (so not the Tere executable failing), and I speculated it was one you'd get if the tmp directory was invalid. But it's creating the directory in the right place (Nix does everything in a transient directory at /build/) with the right permissions so I'm not sure what. Any idea what command actually is actually raising the error?

FYI, Nix's buildRustPackage changes the cargo directory structure from ./target/<mode>/tere to ./target/<architecture>/<mode>/tere. It says in the docs that lots of Rust projects have this pattern hardcoded somewhere, maybe yours does too and I couldn't find it?

@mgunyho
Copy link
Owner

mgunyho commented Mar 27, 2024

Thanks!

Nix's buildRustPackage changes the cargo directory structure from ./target//tere to ./target///tere.

The executable path is "hard-coded" in the tests here at compile time, but that didn't turn out to be the issue, see my comment in #93.

putting println statements like a noob

Haha, that's exactly how I found out the root cause of the issue.

Btw, when running with nix build, the whole program and its dependencies are recompiled from scratch, which takes quite a while. Do you know of any tricks to work around this? From what I can tell, it's because cargo build or cargo test is invoked in a fresh new directory created by Nix, so it makes sense that it has to recompile everything, but is there no way to cache the Rust compilation results across Nix builds?

@mgunyho
Copy link
Owner

mgunyho commented Mar 30, 2024

I investigated a bit, and it seems like one way to enable /dev/tty in the build is to invoke the tests as script -c "cargo test", based on this discussion. So I tried to override the checkPhase with checkPhase = ''script -c "cargo test"'' in buildRustPackage.

The script command is provided by the util-linux package (this was very hard to find...), but I couldn't figure out how to add it to the dependencies of the flake. I tried to add buildInputs = [ util-linux ] to buildRustPackage, but it still fails with script: command not found.

Now, even if the script solution works, is this a good approach? It feels a bit hacky, and I don't know how it could be adapted for the chroot case for Arch as discussed in the original issue. It's also a very Linux-specific solution, but maybe this is not such a big problem since the CLI integration tests are currently Linux-only anyway.

@mgunyho mgunyho changed the base branch from master to develop March 30, 2024 14:58
@ProducerMatt
Copy link
Contributor Author

The other user suggested the faketty cargo, that might work.

I'm not sure why buildInputs wouldn't work. Maybe it counts as a runtime dependency which would be "nativeBuildInputs". If you push your change to develop, I can try.

You asked how you could build the package without rebuilding the dependencies every time. The answer is buildRustCrate and/or cargo2nix which are aware of each dependency separately. I avoided using them because some of those setups basically trade the Cargo.lock for a Nix-specific file, meaning you can't build the package without Nix, I didn't want to limit your options. But I need to double-check if that's actually true.

@mgunyho
Copy link
Owner

mgunyho commented Mar 30, 2024

Yeah, I will check out faketty if that enables somehow more of a "Rust-native" way to do this.

I have pushed my attempt at adding buildInputs and overriding the checkPhase in e0b69ea.

buildRustCrate or cargo2nix ... they basically trade Cargo.lock for a Nix-specific file

Ah I see, yeah I would prefer to keep the build to be Cargo-based. While searching for this, I also found naersk and crane which seem to be also doing something like this, but without overriding Cargo.lock? I tried to add naersk to the flake.nix, but again I didn't know how to do it with my zero Nix experience (I even asked ChatGPT).

@mgunyho
Copy link
Owner

mgunyho commented Mar 31, 2024

Okay, I was able to run the tests through script by adding ${pkgs.util-linux}/bin/script, see 3811a1f.

This works as expected and makes /dev/tty available: if I add

println!("/dev/tty: {:?}", std::fs::File::open("/dev/tty"));

to one of the failing tests, when running without script, it outputs

/dev/tty: Err(Os { code: 6, kind: Uncategorized, message: "No such device or address" })

which I suspected was the cause of the issue. With the script, this changes to

/dev/tty: Ok(File { fd: 15, path: "/dev/tty", read: true, write: false })

which seems promising but the tests still fail. I am really at a loss here.

I also tried faketty, which in the end turned out to be just a modern replacement for script, i.e. it's just

checkPhase = ''
${pkgs.faketty}/bin/faketty cargo test
'';

buildInputs = [
  faketty
];

but the result is the same.

@ProducerMatt
Copy link
Contributor Author

ProducerMatt commented Mar 31, 2024

  1. I'm trying a library called crane, it's giving the same strict compilation environment while caching dependencies. Should also be able to get Musl and cross-arch builds working. I'll push that in a bit.
  2. I'm trying script and faketty and it hasn't fixed those error messages, I'm at a loss. ¯\(ᐛ)

@mgunyho
Copy link
Owner

mgunyho commented Apr 1, 2024

Okay, crane sounds like what I would like to have, at least while developing.

I'm trying script and faketty and it hasn't fixed those error messages

Yeah, let's not struggle with this further. You can either disable all of the integration tests with checkPhase = ''cargo test --bins'' or just the failing tests explicitly with

cargo test -- --skip basic_run --skip output_on_exit_without_cd --skip first_run_prompt_cancel --skip first_run_prompt_accept

This way, new tests will be run if they are added. OTOH, integration tests will likely always run the program and run into this same tty issue.

For now, let's not keep the Nix config files in this repo, I think they should be in nixpkgs instead.

@mgunyho
Copy link
Owner

mgunyho commented Apr 1, 2024

Okay, I couldn't help myself but investigate a bit further, and I think I have a solution.

I understand now why adding faketty doesn't help: the integration test launches the app as a subcommand in a subshell, which doesn't inherit the pty created by faketty. However! The rexpect library which I'm using to launch the program already handles creating a pty, and /dev/tty in fact is available inside the main program, even if not in the test that launches it.

More interestingly, the reason the window_size function (even with /dev/tty available!) returns the file not found error is not because of the ioctl() call. I was looking at the wrong version of the function from the crossterm library! The one that my code is actually using this version, which has two important differences: It checks if the terminal size reported by ioctl is zero, which seems to be the case for the nix build, and if it is, it tries to figure out the terminal size by running the tput command. This is where the file not found error is actually coming from: the tput command is not available in the build environment. So I was able to pass the tests with

buildInputs = [
    ncurses  # provides the tput command needed for integration tests
]

checkPhase = ''
export PATH=${pkgs.ncurses}/bin:$PATH
cargo test
''

(see abdc4e1. This is still building on the original buildRustPackage based solution, please adapt it to crane if you end up using it)

I saw some issues on the crossterm tracker that they want to get rid of the tput dependency, but currently it still seems to be there.

@ProducerMatt
Copy link
Contributor Author

Ok, I've pushed a crane based build that caches dependencies and provides a musl variant which can be built with nix build .#musl.

I've left some extra features from the default template if you're interested. One is Darwin build support, which I can't test. The other is a lot of checks (formatting, dependencies with security issues, etc) which can be run with nix flake check. You can erase any of these extra things you don't want to reduce bloat, or ask me to do it, I just figured I'd give you the option :)

I also tried to make the flake able to cross-compile for other architectures, as I'd heard Nix can handle cross compiling fairly well. But as you can see in this example the Rust tooling needs lots of massaging to work and it would require putting parameters absolutely everywhere.

@mgunyho
Copy link
Owner

mgunyho commented Apr 2, 2024

Thank you, that sounds great! I really appreciate your work. Can this configuration be used in nixpkgs? As I mentioned, I would prefer if it lived there.

@ProducerMatt
Copy link
Contributor Author

After looking at Nixpkgs, it turns out that external flakes are not allowed, meaning Crane can't be used there. Details here. So my hopes of porting these changes back to NixPkgs are dashed.

As to the flake being in this repo, notice it contains a lot of non-package conveniences (toolchain, formatting, and it can even be extended to give shell aliases and such). IMO the main advantage of having the flake in your repo is being able to use it to run tests, as you've seen it lets you catch bugs that don't show up in the regular build.

Let me know whether you want to keep the flake or not, or if there's something you'd like to see changed.

@mgunyho
Copy link
Owner

mgunyho commented Apr 13, 2024

Hm, that's unfortunate. I suppose the same applies for naersk, or other libs for building Rust projects? But maybe for the build on the nixpkgs server that caching doesn't help anyway, or does it?

In any case, thank you very much for this flake. Do you mind if we leave it out of this repo for now? We can keep your patch/branch around in this PR if it's needed for testing.

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.

None yet

2 participants