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

Add nix flake #1575

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add nix flake #1575

wants to merge 3 commits into from

Conversation

tirimia
Copy link

@tirimia tirimia commented Nov 27, 2024

This PR enables building lychee via nix flake by simply running nix build.
Simultaneously, nix develop will open a shell with the environment required to build and develop it.

@mre mre mentioned this pull request Nov 27, 2024
@mre
Copy link
Member

mre commented Nov 27, 2024

@thomas-zahner, @niklaskorz, any thoughts?

@thomas-zahner
Copy link
Member

@tirimia Did you see #1565? It's pretty much a duplicate of this PR. The main difference is that with my PR I didn't have nix build in mind. I do like the idea, so eventually we could use Nix to build in CI.

Is it possible to easily specify the Rust version in this flake? If not I would probably prefer my approach.

@tirimia
Copy link
Author

tirimia commented Nov 30, 2024

That’s the beauty of using the fenix flake and reading the toolchain configuration, that’s already taken care of and we then also know we provision rust like the non-nix users of the repo 🥳

In general though, I personally avoid thinking of the specific version of software when working with nix as it’s rolling release and if I need something specific, importing a pinned version of nixpkgs is still an open option.

A further advantage of this PR is that people can reference this flake and use lychee without having to figure out how to build it on their own. You can also directly call it with nix run github:lycheeverse/lychee

@thomas-zahner
Copy link
Member

@tirimia Ah I see, thanks for the clarification. I agree that this is probably the better approach. Does this mean that nixpkgs could simply reference our flake?

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@thomas-zahner
Copy link
Member

Also would you mind if we add an .envrc file to the project, as in my PR?
Sorry if my questions seem weird or stupid, I'm quite new to Nix 😅

@tirimia
Copy link
Author

tirimia commented Nov 30, 2024

@tirimia Ah I see, thanks for the clarification. I agree that this is probably the better approach. Does this mean that nixpkgs could simply reference our flake?

Nixpkgs offers a reference to its packages via flakes as a convenience; flakes are not “officially” stable.

You can, however, reference this flake in your own or in the command line

@tirimia
Copy link
Author

tirimia commented Nov 30, 2024

Also would you mind if we add an .envrc file to the project, as in my PR? Sorry if my questions seem weird or stupid, I'm quite new to Nix 😅

I would avoid that. Direnv docs recommend you add .envrc and .direnv to your global git ignore. Committing it forces people to use the flake if they use direnv, and it also prevents people from having their own configuration

As for the “stupid” questions: nix is a beast, the craft of tech is impossible to master in its entirety. Don’t judge yourself for not knowing things, enjoy that you get to learn 💃

@niklaskorz
Copy link

niklaskorz commented Nov 30, 2024

Also would you mind if we add an .envrc file to the project, as in my PR? Sorry if my questions seem weird or stupid, I'm quite new to Nix 😅

I would avoid that. Direnv docs recommend you add .envrc and .direnv to your global git ignore. Committing it forces people to use the flake if they use direnv, and it also prevents people from having their own configuration

As for the “stupid” questions: nix is a beast, the craft of tech is impossible to master in its entirety. Don’t judge yourself for not knowing things, enjoy that you get to learn 💃

As for .direnv, yes, that should be gitignored. But it's common practice to track the .envrc.

flake.nix Outdated 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
flake.nix Outdated Show resolved Hide resolved
@thomas-zahner
Copy link
Member

@tirimia @niklaskorz thank you for the answers and improvements. The flake now targets my platform but it still does not build because of the pinned SHA which is not the same on my platform. With the current version I get a mismatch: error: hash mismatch in fixed-output derivation.

We can't simply remove the argument because it seems to be required: error: in pure evaluation mode, 'fetchurl' requires a 'sha256'.

When an empty string is provided or if nixpkgs.lib.fakeSha256 is used, the following error will occur

       error: hash mismatch in fixed-output derivation ...:
         specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
            got:    sha256-s1RPtyvDGJaX/BisLT+ifVfuhDT1nZkZ1NcK8sbwELM=

So now I see two possibilities:

  • Specify the hashes for every single platform. But I'm not sure if this is possible or if this makes sense as it seems quite cumbersome.
  • Just switch to rust-overlay as in my PR

@tirimia
Copy link
Author

tirimia commented Dec 3, 2024

Oops, I was not aware that would happen, so sorry 😭
Tracked the issue down to https://github.com/nix-community/fenix/blob/a8a983027ca02b363dfc82fbe3f7d9548a8d3dce/default.nix#L73 , it downloads binary distributions of rust and the hash of stable obviously changes over time 🤦

Pushing a new version that uses crane (better for incremental builds), also getting read of flake-utils since it's not necessary

Also, since the rust-toolchain only wants stable and nothing fancy, we can just use whatever rust version nixpkgs is shipping.

@thomas-zahner
Copy link
Member

That's no problem.

Ah, that's interesting. Could you elaborate what the advantages of crane are? I'm I understanding it correctly that the dependencies are now fetched with Nix instead of cargo?

Crane seems to be quite slow. nix build takes almost 3 minutes on my machine whereas cargo build takes 1 minute (without cache/target directory). Also my build fails because a test fails. This doesn't happen with cargo build or cargo test.

error: builder for '/nix/store/k56vbxvafx6zjiwdscx4wvaak7964xgc-lychee-8bbcc0728e1a102e182499031abd91197a7360d5.drv' failed with exit code 101;
       last 25 log lines:
       > assertion failed: res.status().is_success()
       >
       > ---- client::tests::test_youtube stdout ----
       > thread 'client::tests::test_youtube' panicked at lychee-lib/src/client.rs:627:9:
       > assertion failed: res.status().is_success()
       >
       > ---- collector::tests::test_url_without_extension_is_html stdout ----
       > thread 'collector::tests::test_url_without_extension_is_html' panicked at lychee-lib/src/collector.rs:195:9:
       > assertion `left == right` failed
       >   left: 0
       >  right: 1
       >
       >
       > failures:
       >     client::tests::test_basic_auth
       >     client::tests::test_custom_headers
       >     client::tests::test_github
       >     client::tests::test_invalid_ssl
       >     client::tests::test_require_https
       >     client::tests::test_youtube
       >     collector::tests::test_url_without_extension_is_html
       >
       > test result: FAILED. 248 passed; 7 failed; 2 ignored; 0 measured; 0 filtered out; finished in 7.07s
       >
       > error: test failed, to rerun pass `-p lychee-lib --lib`

When I would use crane as developer how would I use clippy and rust-analyzer? If you take a look at my PR you can see that I also include clippy and rust-analyzer explicitly because as a developer I definitely also want to make use of these tools.

@tirimia
Copy link
Author

tirimia commented Dec 6, 2024

Building the package is a different concern from developing it. Crane makes building a lot more cache friendly as you just need to compile the project itself if no dependencies changed (think local development and GitHub Actions). That is not the case with the other approaches. Also, nix builders will always be slower than just running the builder, they are optimized for reproducibility at the cost of speed (you really feel it in go projects 😢)

With regards to clippy and rust-analyzer, I'm more than happy to add a dev shell that provides those, my focus was allowing people to build lychee from main, so I kinda skipped that part 😅

The tests are not failing on my machine(ironic), and I'm not sure what to do, here's a screenshot of the output from nix build --rebuild
image

@thomas-zahner
Copy link
Member

Well that's really weird if it doesn't fail on your side, isn't the whole point of what we try to achieve reproducibility 😅 I think all of the failing tests do some real HTTP calls so this might be a clue. For testing I've also set both buildInputs and nativeBuildInputs to [ pkgs.pkg-config pkgs.openssl ], thought it might be an openssl issue, but there errors remained.

Here's the end of the full error message, maybe it helps?

failures:

---- client::tests::test_github stdout ----
thread 'client::tests::test_github' panicked at lychee-lib/src/client.rs:605:9:
assertion failed: res.status().is_success()

---- client::tests::test_custom_headers stdout ----
thread 'client::tests::test_custom_headers' panicked at lychee-lib/src/client.rs:701:9:
assertion failed: res.status().is_success()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- client::tests::test_require_https stdout ----
thread 'client::tests::test_require_https' panicked at lychee-lib/src/client.rs:751:9:
assertion failed: res.status().is_success()

---- client::tests::test_invalid_ssl stdout ----
thread 'client::tests::test_invalid_ssl' panicked at lychee-lib/src/client.rs:674:9:
assertion failed: res.status().is_success()

---- client::tests::test_basic_auth stdout ----
thread 'client::tests::test_basic_auth' panicked at lychee-lib/src/client.rs:640:9:
assertion `left == right` failed
  left: None
 right: Some(401)

---- client::tests::test_youtube stdout ----
thread 'client::tests::test_youtube' panicked at lychee-lib/src/client.rs:627:9:
assertion failed: res.status().is_success()

---- collector::tests::test_url_without_extension_is_html stdout ----
thread 'collector::tests::test_url_without_extension_is_html' panicked at lychee-lib/src/collector.rs:195:9:
assertion `left == right` failed
  left: 0
 right: 1


failures:
    client::tests::test_basic_auth
    client::tests::test_custom_headers
    client::tests::test_github
    client::tests::test_invalid_ssl
    client::tests::test_require_https
    client::tests::test_youtube
    collector::tests::test_url_without_extension_is_html

test result: FAILED. 248 passed; 7 failed; 2 ignored; 0 measured; 0 filtered out; finished in 7.06s

@thomas-zahner
Copy link
Member

thomas-zahner commented Dec 10, 2024

@tirimia Also thanks for the clarification. I think we have two use cases: building the project and a practical/useful dev shell. For me personally the dev part is more important. But of course it would be awesome if we could also build with Nix, so that in the long run we might even use Nix in CI.

If you know what the problem is, it would be great if you could fix the build issue. But since you don't seem to be able to reproduce it that might be hard.. If we did manage to fix that, then I would also like to add the programs previously mentioned and add the .envrc.

But otherwise I'd like to merge my PR. I think it should also be possible to update my PR so that nix build works as it currently doesn't. Could you make that possible? Also my PR isn't platform independent yet.

@tirimia
Copy link
Author

tirimia commented Dec 10, 2024

We can for sure get your PR working cross-platform, all we need there is to add "aarch64-darwin" to the systems and most devs will be covered; if we go with that approach, I do have to urge you to reconsider checking in the .envrc. Keeping it will force direnv users to use the flake (or get annoying errors), or get in the way by making them have unstaged changes on it to disable the flake and add their own configuration

With regards to the nix build working there, think we can write an overlay to adapt the src, version and cargoHash of the nixpkgs lychee recipe

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.

4 participants