mgunyho / tere

Terminal file explorer
European Union Public License 1.2
1.67k stars 36 forks source link

Nix development environment #100

Open ProducerMatt opened 6 months ago

ProducerMatt commented 6 months ago

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:

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 5a03e567f90c795a27237a6dedf045504323cc0e 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 5a03e567f90c795a27237a6dedf045504323cc0e 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 commented 6 months ago

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 commented 6 months ago

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

ProducerMatt commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

ProducerMatt commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago
  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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.