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

Fix nix build #579

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Fix nix build #579

merged 2 commits into from
Aug 14, 2024

Conversation

feathecutie
Copy link
Contributor

Currently, running nix build in this repo to build niri using the flake.nix file fails because the tests during the check phase can't find certain runtime dependencies (like libxkbcommon).

This PR fixes this by simply passing the required runtime dependencies to the phases themselves using the LD_LIBRARY_PATH environment variable, fixing the tests without changing the resulting output.

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 8, 2024

@sodiboo hi

@feathecutie
Copy link
Contributor Author

Sadly this is probably unrelated to the issues @sodiboo is facing, since they use a custom build process instead of the one supplied by this repo's flake.nix.
This PR simply fixes the build for people using this flake, not https://github.com/sodiboo/niri-flake

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 8, 2024

Well, I still want someone who knows nix to take a look

@sodiboo
Copy link
Contributor

sodiboo commented Aug 8, 2024

Sadly this is probably unrelated to the issues @sodiboo is facing, since they use a custom build process instead of the one supplied by this repo's flake.nix.
This PR simply fixes the build for people using this flake, not https://github.com/sodiboo/niri-flake

It's worth noting that I was investigating this issue the other day. I got it down to a change made in Rust nightly 2024-05-18, and was going to bisect rustc but didn't quite get around to doing that yet (I was going down the path of "why did this suddenly change??"), and as such this probably is relevant to me even though I mainly deal with that other repo. I'm also a niri contributor, and I do happen to make use of this repo's flake when working on niri (which is hard, when it doesn't build lol).

I'm exhausted right now, though, and I will probably take a look at this PR tomorrow.

flake.nix Outdated
niri = craneLib.buildPackage (craneArgs // {inherit cargoArtifacts;});
niri = craneLib.buildPackage (craneArgs // {
inherit cargoArtifacts;
LD_LIBRARY_PATH = pkgs.lib.makeLibraryPath craneArgs.runtimeDependencies; # Needed for tests to find libxkbcommon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being added here instead of to craneArgs, like with LIBCLANG_PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add it in craneArgs because craneArgs is also used for creating cargoArtifacts, and it's technically not necessary there.
I could also just move it to craneArgs though, it shouldn't make any difference, I simply tried to keep everything as clean as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if I put LD_LIBRARY_PATH into craneArgs, I'd have to turn craneArgs into a recursive attrset in order to access the runtimeDependencies

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to turn craneArgs into a recursive attrset in order to access the runtimeDependencies

craneArgs is in scope though. you can just move the line up without changing a single char on it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I didn't know that was possible. I rebased it now to use craneArgs, and also fixed the deprecation warning:

warning: `crane.lib.${system}` is deprecated. please use `(crane.mkLib nixpkgs.legacyPackages.${system})` instead

Copy link
Contributor

@sodiboo sodiboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me! does what it says on the tin (fixes nix build) and now the diff is also really pretty :3. i'd ask to fix cargo build on NixOS in the same go but uhh, that's a whole 'nother can of worms. i tried. it's awful. i'll submit another PR sometime. this one could do with maybe a nix flake update and then let's merge it!

@feathecutie
Copy link
Contributor Author

feathecutie commented Aug 14, 2024

Should be ready to merge now, but what do you mean by fixing cargo build on NixOS?

A simple cargo build command in the devShell seems to work fine for me, the main problem seems to be that the devShell doesn't correctly set up runtime dependencies for cargo run.
I was able to get cargo run to work too using the following diff:

diff --git a/flake.nix b/flake.nix
index f8c4b63..9b7562e 100644
--- a/flake.nix
+++ b/flake.nix
@@ -77,6 +77,15 @@
             xorg.libXcursor
             xorg.libXi
             libxkbcommon
+            pipewire
+            pango
+            glib
+            cairo
+            seatd
+            systemd
+            libinput
+            pixman
+            mesa # For libgbm
           ];
 
           LIBCLANG_PATH = "${pkgs.libclang.lib}/lib";

(I simply added these packages to runtimeDependencies).

It isn't really obvious why this would make any differences, as most of these packages are already part of buildInputs, and buildInputs also gets converted to LD_LIBRARY_PATH, same as runtimeDependencies.

However, when inspecting the resulting LD_LIBRARY_PATH environment variable in the devShell, you can see that (for whatever reason), the resulting path uses different derivation outputs for the derivations from nativeBuildInputs and buildInputs as compared to runtimeDependencies (as can be seen by the -dev suffix on some of the package paths).
I don't know if there's a more elegant solution to this problem, and I'd like to understand the underlying reason for this, but at least this seems to get cargo run working (for me).

@feathecutie
Copy link
Contributor Author

feathecutie commented Aug 14, 2024

More experimentation:

Running builtins.map toString packages.x86_64-linux.default.buildInputs in a nix repl results in the following:

nix-repl> builtins.map toString packages.x86_64-linux.default.buildInputs
[
  "/nix/store/dhafvd6slhk8hz8g0hvqnqh5i9fmqma4-wayland-1.23.0-dev"
  "/nix/store/prf4i4plz0k43jz0ghyzlf7gwmi8rjvg-systemd-256.2-dev"
  "/nix/store/x4ai78x8i92qz2bxpvp9wzgl0k31jvw7-seatd-0.8.0-dev"
  "/nix/store/8pbh705kblpkhlg25lax7hcfh2zcv2ab-libxkbcommon-1.7.0-dev"
  "/nix/store/gj93mj1hks0xrsj20n7f3dfsdcd471s9-libinput-1.26.1-dev"
  "/nix/store/xgpdv93w3kkvp45ip1xym55z72crz7v0-mesa-24.1.4-dev"
  "/nix/store/zs4v7bsmxvblgv02nak0wcxpjncw80jf-fontconfig-2.15.0-dev"
  "/nix/store/kgmfgzb90h658xg0i7mxh9wgyx0nrqac-gcc-13.3.0-lib"
  "/nix/store/cw78ndjp827zan6hpdk41c45ynfwqrvk-pipewire-1.2.1-dev"
  "/nix/store/khwdaapk8l3kfy764j71cir1lafr5bpm-pango-1.52.2-dev"
]

This shows that, for whatever reason, the buildInputs attribute on the niri package evaluates to the dev output of the contained derivations (which seems to not contain the files that cargo run needs, I guess).
This is probably completely normal behaviour (no idea, I never had to mess around with derivation outputs before), so it might make sense to simply add the above packages to runtimeDependencies for now in order to fix cargo run.

@YaLTeR YaLTeR merged commit f54297f into YaLTeR:main Aug 14, 2024
9 checks passed
@YaLTeR
Copy link
Owner

YaLTeR commented Aug 14, 2024

Thanks; let's do the cargo run fix in a separate PR

@sodiboo
Copy link
Contributor

sodiboo commented Oct 1, 2024

I was investigating this issue the other day. I got it down to a change made in Rust nightly 2024-05-18, and was going to bisect rustc but didn't quite get around to doing that yet

yoo turns out I don't need to do that anymore. it's almost certainly because of rust-lld. see blog post date: https://blog.rust-lang.org/2024/05/17/enabling-rust-lld-on-linux.html

thanks to @getchoo for pointing me in the right direction for that (unbeknownst to him)

this breaks builds with Fenix, such as our package at the time:

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.

3 participants