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

build(nix): use nix-cargo-integration, make shell.nix use flake devshell #180

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

yusdacra
Copy link
Contributor

@yusdacra yusdacra commented Jun 8, 2021

This does a few changes:

  • use nix-cargo-integration to integrate with cargo instead of own rust expressions
  • use crate2nix to build the package instead of naersk (much more cache friendly)
  • make the shell.nix file use the flake devshell (thanks to flake-compat)
  • use local code, submodules will still be taken from git master branch

@yusdacra yusdacra force-pushed the build/nix-improvements branch from 7c2bfda to 8a28608 Compare June 8, 2021 15:32
flake.nix Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

pickfire commented Jun 8, 2021

Although I stopped using nix myself and not very good at it. I am a bit concerned about using nix-cargo-integration, seemed like @yusdacra is the maintainer, the project being quite new. @yusdacra What's your plan on maintaining the project and the status?

@yusdacra
Copy link
Contributor Author

yusdacra commented Jun 8, 2021

Although I stopped using nix myself and not very good at it. I am a bit concerned about using nix-cargo-integration, seemed like @yusdacra is the maintainer, the project being quite new. @yusdacra What's your plan on maintaining the project and the status?

The project is fairly stable now, I don't expect any big changes or anything (if flakes stay as-is that is), mostly only bug fixes and new features. Next thing I'm working on is adding more documentation to the code. I try to fix issues that pop up as fast as I can since I myself use it on many projects (or other projects). Hope that answers it :D

@pickfire
Copy link
Contributor

pickfire commented Jun 8, 2021

Let's see what @archseer says. It looks okay to me since it is able to make use of cachix.

@pickfire pickfire mentioned this pull request Jun 9, 2021
@archseer
Copy link
Member

archseer commented Jun 9, 2021

I'd like to see what @nrdxp and @gytis-ivaskevicius think about it, I tend to avoid wrappers since it's one less layer to go wrong. That said, it's my first Rust flake so I'm open to accepting it if other Nix users agree :)

@gytis-ivaskevicius
Copy link

gytis-ivaskevicius commented Jun 9, 2021

Here are my thoughts. I am personally not particularly supportive of this PR

use nix-cargo-integration to integrate with cargo instead of own rust expressions

What does this gain us? It adds more lines than it removes

use crate2nix to build the package instead of naersk (much more cache friendly)

Very nice, but I think this would be a nicer solution https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#importing-a-cargolock-file-importing-a-cargolock-file

make the shell.nix file use the flake devshell (thanks to flake-compat)

👍

@yusdacra
Copy link
Contributor Author

yusdacra commented Jun 9, 2021

use nix-cargo-integration to integrate with cargo instead of own rust expressions

What does this gain us? It adds more lines than it removes

I made nix-cargo-integration because of all of this boilerplate being used everywhere; what it does is move the boilerplate to a single repo so that it can be maintained and distributed more nicely. Other than that it also integrates with Cargo.toml using the *.metadata attributes, so it can be configured via TOML (heavily inspired by devshell), and does other stuff such as binary attribute detection in Cargo.toml and putting them as seperate app outputs in the flake outputs, better defaults and other niceties. I understand that it might seem like a big abstraction (and the current nix-cargo-integration code doesn't explain that well sadly).

Very nice, but I think this would be a nicer solution https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#importing-a-cargolock-file-importing-a-cargolock-file

That does not support automatic git vendoring (ie. using the revision hash in the lockfile and not needing a seperate hash) while (patched) crate2nix can do that (which is what nix-cargo-integration uses). It also does not do what crate2nix does, which is compiling each crate as a seperate derivation, which means it will have much better caching. Although this does need a more recent Nix version, as it can use attributes like submodules and allRefs of builtins.fetchGit, but if one is using flakes, they should already be recent enough.

@gytis-ivaskevicius
Copy link

gytis-ivaskevicius commented Jun 9, 2021

I have not tried it yet but I thought that this should take care of git stuff:

 cargoLock = {
    lockFile = ./Cargo.lock;
    outputHashes = {
      "finalfusion-0.14.0" = "17f4bsdzpcshwh74w5z119xjy2if6l2wgyjy56v621skr2r8y904";
    };
  }

https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#importing-a-cargolock-file-importing-a-cargolock-file

@yusdacra
Copy link
Contributor Author

yusdacra commented Jun 9, 2021

I have not tried it yet but I thought that this should take care of git stuff:

 cargoLock = {
    lockFile = ./Cargo.lock;
    outputHashes = {
      "finalfusion-0.14.0" = "17f4bsdzpcshwh74w5z119xjy2if6l2wgyjy56v621skr2r8y904";
    };
  }

https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#importing-a-cargolock-file-importing-a-cargolock-file

Yeah that's what I meant, with nix-cargo-integration you wouldn't need to specify hashes like that since it would be handled automatically for you. (using Cargo.lock to vendor deps instead of specifying a vendor hash is still a big step though, in terms of buildRustPackage)

@gytis-ivaskevicius
Copy link

I see, makes sense. Tho someone should really look into it and add support to nixpkgs. Makes me wonder why is it not there already 🤔

@nrdxp
Copy link
Contributor

nrdxp commented Jun 9, 2021

Do you have a solid plan for submodule support? If so, this may be a better solution than mine #190

@yusdacra
Copy link
Contributor Author

yusdacra commented Jun 9, 2021

Do you have a solid plan for submodule support? If so, this may be a better solution than mine #190

Same idea really. There really isn't a good solution since you can't either access .git, if that could be accessed it would be possible to get the HEAD of all submodules, but sadly we can't do that.

EDIT: sorry, accidentally closed

@yusdacra yusdacra closed this Jun 9, 2021
@yusdacra yusdacra reopened this Jun 9, 2021
@yusdacra yusdacra force-pushed the build/nix-improvements branch 4 times, most recently from 8ec3813 to efa5081 Compare June 11, 2021 17:21
@yusdacra
Copy link
Contributor Author

OK, this builds now. Would like to hear if it works for everyone else too.
And another thing, a Cachix cache should probably be setup, since it will be a huge benefit (even more if crate2nix is used which this PR does).

@yusdacra yusdacra force-pushed the build/nix-improvements branch 4 times, most recently from 09ff97b to ec99b39 Compare June 11, 2021 18:10
@yusdacra yusdacra requested a review from pickfire June 11, 2021 18:18
@yusdacra yusdacra force-pushed the build/nix-improvements branch from ec99b39 to 2a627cf Compare June 11, 2021 18:28
@nrdxp
Copy link
Contributor

nrdxp commented Jun 11, 2021

does this still build from github, or does it now work locally?

@yusdacra yusdacra force-pushed the build/nix-improvements branch from 2a627cf to dc35933 Compare June 11, 2021 18:36
@yusdacra
Copy link
Contributor Author

yusdacra commented Jun 11, 2021

does this still build from github, or does it now work locally?

Not right now, but i'm trying something that is much less hackier than symlinkJoin, so we might get local builds ;)

@yusdacra yusdacra force-pushed the build/nix-improvements branch from dc35933 to 24a6c55 Compare June 11, 2021 18:48
@yusdacra
Copy link
Contributor Author

Alright, just pushed something that should make it better. So now at least the local code will be used, submodules will still be taken from the git.

@nrdxp
Copy link
Contributor

nrdxp commented Jun 11, 2021

It looks like this can be fixed conceptually upstream. I tested the patch mentioned NixOS/nix#4423 (comment), and it actually built with root = self and no special hackery.

Additonally, this type of solution seems to be endorsed by the nix BDFL himself NixOS/nix#4423 (comment). So there is hope for an upstream solution. I'm looking into myself, but I'm not much of a C++ guru so I can't promise anything.

@yusdacra yusdacra force-pushed the build/nix-improvements branch 2 times, most recently from fa51916 to 0bdbcf0 Compare June 13, 2021 05:03
@yusdacra yusdacra force-pushed the build/nix-improvements branch from 0bdbcf0 to 6d84cd9 Compare June 13, 2021 05:04
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

I'm merging this for now since it makes shell.nix work on non-flake installs, we'll re-evaluate if it's necessary to keep later.

@archseer archseer merged commit a3f0150 into helix-editor:master Jun 13, 2021
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.

5 participants