Skip to content

Fix nix build#26270

Merged
P1n3appl3 merged 8 commits intozed-industries:mainfrom
P1n3appl3:nix
Mar 10, 2025
Merged

Fix nix build#26270
P1n3appl3 merged 8 commits intozed-industries:mainfrom
P1n3appl3:nix

Conversation

@P1n3appl3
Copy link
Contributor

This PR includes lots of small fixes to get our build.nix and shell.nix back to a working state.

I've tested this by running cargo run (inside the devshell) and nix run on x86 nixos and arm64 darwin machines. I'd appreciate it if others could test building inside the devshell to double-check that it's not just working because I happen to have some system-level packages installed, as well as seeing if it works on other platforms (non-nixos linux, arm linux, x86 darwin).

I couldn't get the full test suite (cargo nextest run --workspace) passing in the devshell on darwin, but they are all passing on nixos. nixpkgs disables some of our tests that apparently fail or are flakey on hydra, but they don't know why. I'm going to punt on debugging those for now, especially given that they seem to be working for me. I'm also unsure of whether we actually want the nix checkPhase to run the full test suite (it's currently not passing --workspace) given that we have separate CI that should enforce that those pass on all PRs.

Here's an overview of the changes made:

  • Fix our generate-licenses script
    • Relaxes the cargo-about version requirement slightly so it doesn't try to install an older binary when the nixpkgs one is newer than our requirement
    • Add a workaround for this cargo-about issue obviating the need for the patching done in the nixpkgs package
      • @niklaskorz would you be down to go update the package to use ALLOW_MISSING_LICENSES in nixpkgs?
      • I'm still not sure what the long term solution is there, as you mentioned it seems like cargo about just isn't set up to handle deps being vendored like that.
    • Set the new --frozen flag to avoid network access/mutating the lockfile
  • Use dynamic webrtc lib from nixpkgs, and fixes up the build script in webrtc-sys that hardcodes it to be statically linked.
  • Use inputsFrom in shell.nix and avoid duplicating everything from build.nix
  • Add a temporary workaround for an upstream crane bug.
  • Fix shebangs in our script dir to not hard-code /bin/bash

There are still a bunch of issues that aren't resolved here, I'll make a tracking issue for those and try to land this first just to get back to an unbroken state. Eventually among other things I'd like to use a libgit2 from staticPkgs and musl cross compilation to build the remote server under nix, and then add that as a separate flake output and include it in the shell's inputsFrom list.

Thanks @niklaskorz, @GaetanLepage, @bbigras and all the other nixpkgs maintainers that have kept the zed-editor package working and up to date! I seriously considered just making our flake overrideAttrs the package in nixpkgs given how well maintained it is.

Thanks @WeetHet for your volunteer maintinance of this flake. I referenced #24953 while working on these fixes, and I'd love to collaborate on adding some of those pieces like treefmt and a github action. If you're interested I'd really appreciate some help debugging why crane's buildDepsOnly isn't working for us. I'm assuming it'd make our nix build times go way down from the improved dep caching if we could get it working.

Thanks @rrbutani for all the help on this PR 💙.

Release Notes:

  • N/A

P1n3appl3 and others added 6 commits March 5, 2025 03:20
Co-authored-by: Rahul Butani <rrbutani@users.noreply.github.com>
Uses the new `--frozen` flag to avoid attempting to fetch or update
versions during the build.

Also changes from a hardcoded version to a semver constraint to avoid
downloading an extra `cargo-about` when a slightly newer one is present.

There are still issues due to `fetchCargoVendor` putting deps in a different
place than is expected by zed-licenses.toml. See #19971 for details.
- fixed source filtering
- use zed's cargo-bundle fork
- use dynamic linking for livekit-webrtc

Co-authored-by: Rahul Butani <rr.butani@gmail.com>
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Mar 7, 2025
@zed-industries-bot
Copy link
Contributor

zed-industries-bot commented Mar 7, 2025

Messages
📖

This PR includes links to the following GitHub Issues: #19971, #ipetkov/crane#808
If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against 30c3fd3

@niklaskorz
Copy link
Contributor

niklaskorz commented Mar 7, 2025

We'd love replacing our nixpkgs generate-licenses patch with a simple env var, thank you! If you're interested, we also have a small Matrix room for Zed on Nix(OS) where we discuss the package maintenance, and would be glad to extend that to upstream maintainers. You can contact me on Matrix if you'd like to join.

I love supporting a decade old bash version 🙃
@jansol
Copy link
Collaborator

jansol commented Mar 7, 2025

seriously considered just making our flake overrideAttrs the package in nixpkgs

Honestly I'd love having the nixpkgs package in a state where that is a workable option, it would make maintenance so much more centralized. What problems were there with that? I guess nixpkgs might be a bit slow to update for how fast zed moves at times.

@WeetHet
Copy link
Contributor

WeetHet commented Mar 7, 2025

Honestly I'd love having the nixpkgs package in a state where that is a workable option, it would make maintenance so much more centralized. What problems were there with that? I guess nixpkgs might be a bit slow to update for how fast zed moves at times.

I've done that in #22825 ultimately coming to a conclusion that as nixpkgs tracks stable zed and therefore can't be updated before the 2 week release window it would be less sustainable that having the package here

Ultimately, the build process of zed seems surprisingly fragile up to the point that building in debug mode in nix breaks it, so I expect actually getting CI to work quickly enough to be included in every PR to be an incredibly painful debugging experience

@P1n3appl3
Copy link
Contributor Author

P1n3appl3 commented Mar 8, 2025

I came to similar conclusions, while re-using the definition from nixpkgs would reduce duplication and possibly save some effort, there's several reasons why I believe it's still worth maintaining our own:

  • Having control over the entire build definition is helpful for us zed maintainers.
    • Having the build all in one place makes it easier to reason about.
    • If we wanted to make a big change (like this PR tbh), it'd be even messier to do as an override.
    • The process of us making a build change, writing an override to support it in our nix build, making a nixpkgs PR to upstream it, and then updating our flake and removing the override, would be much higher friction than just making the change in this repo all at once and letting nixpkgs pick up our change at their own pace.
  • There are cases where we have different priorities than upstream nixpkgs which warrants some divergence imo.
    • Using crane over buildRustPackage can improve build times which could end up being important if we run the nix build in CI.
    • Crane enables things like a separate rustdoc output which I'm interested in setting up but doesn't make much sense for nixpkgs.
    • We might not agree with nixpkgs on certain aspects of our build, such as whether to statically link a webrtc library or build the remote server with musl.
    • We're happy maintaining and packaging a forked tool or dep for our build, but nixpkgs (understandably) tends to prefer avoiding that even if it means some patches/hacks.
    • Nixpkgs already puts zed in an FHSenv to improve support for extensions downloading arbitrary binaries out of the box. I'm not sure whether we want to take this approach with our package.

Of course I plan to keep an eye on the nixpkgs package. Those maintainers are much more knowledgeable about nix than any of us at zed, and some of the fixes in this PR are straight from their solutions. I'll also try to make sure it's easy for them to integrate changes we make to our build, for example by adding knobs to disable things that aren't working under nix so they can avoid patching when possible.

@P1n3appl3 P1n3appl3 merged commit 4a7c84f into zed-industries:main Mar 10, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants