Nix flake

[nicholaschiasson]

Description

• Adding initial nix flake capable of building the code
• Checking Cargo.lock into version control

Testing

N/A since there is no code change.

I simply ran nix run locally to test that it is working.

Checklist

Things need to be done before this Pull Request can be merged.

• Review if we want to expand upon dev shell, adding extra tooling for local devel/opment for those who wish to use nix
• Review pinned SHA for fastnbt dependency and if this is necessary (not preferred since there is a manual step introduced in updating the flake in case we ever want to upgrade this dependency).
• Review version in buildRustPackage, which is currently simply pulled from ./pumpkin/Cargo.toml. This is probably fine, except that it could be misleading about the outputs being built, masquerading the output as a proper release, when the build is coming from a feature branch. Maybe better would be to derive version somehow from git ref at HEAD or something.

[nicholaschiasson]

For fastnbt dep, I understand now the reason it is necessary to pin the SHA in this flake is because the dependency is specified in a few Cargo.tomls using a git url. Is this because we need features not yet available in an official release (2.5.0), that are only in fastnbt master branch? And as a side question, is this a blocker before pumpkin itself can also have a stable release?

With this change, I have checked the Cargo.lock into version control, since that is the recommendation for rust projects that output binaries. The flake itself depends on the lock file as well, so it kind of makes the Cargo.lock file necessary. If that is problematic, then this change may be inappropriate for this repo. However, I think it will also help here. And for new contributors as well, since we guarantee people are getting the same versions of dependencies after a fresh repo clone.

If we ever wish to update the fastnbt dependency, the flake file will need to be updated too to reflect the new hash (tedious), or if we can pin against a new fastnbt release from the crates registry, we can remove the explicit pinning from the flake. That would be best because then there would be no hard coded versions in there and the file would rarely/never require further modification.

[Snowiiii]

We are currently moving to an own NBT Crate. Should be done soon. Only some issues with arrays when deserializing.
#100

[Snowiiii]

I decided to partially move to our own NBT crate due to a Problem with Deserialization when having Arrays. Until the Problem is not solved we still need to use fastnbt for reading anvil chunks

[IogaMaster]

I +1 this

[Eveeifyeve]

This shell could be cleaned up by not repackaging pumpkin instead letting the user build it themselves and utilising the rust-toolchain.toml.

flake.nix

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
    flake-parts.url = "github:hercules-ci/flake-parts";
    flake-compat.url = "https://flakehub.com/f/edolstra/flake-compat/1.tar.gz";
    rust-overlay = {
      url = "github:oxalica/rust-overlay";
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };

  outputs = inputs@{ nixpkgs, flake-parts, ... }:
    flake-parts.lib.mkFlake {inherit inputs;} {
      systems = nixpkgs.lib.systems.flakeExposed;
      perSystem = {
        lib,
        pkgs,
        system,
        config,
        ...
      }: 
      {
        _module.args.pkgs = import nixpkgs {
          inherit system;
          overlays = [
            (import inputs.rust-overlay)
          ];
        };

        devShells.default = with pkgs; let 
        toolchain =  break ((pkgs.rust-bin.fromRustupToolchainFile ./rust-toolchain.toml).override {
          extensions = [ "rust-analyzer" "rust-src" ];
          targets = []; # Targets that you want to add support
        }); 
        in mkShell
        {
          nativeBuildInputs = with pkgs; [
            toolchain
            openssl
            pkgs-config
          ] ++ lib.optionals stdenv.isDarwin [
            apple-sdk
          ];
        };
      };
    };
}

shell.nix (for non flakes)

(import
  (
    let
      lock = builtins.fromJSON (builtins.readFile ./flake.lock);
      nodeName = lock.nodes.root.inputs.flake-compat;
    in
    fetchTarball {
      url = lock.nodes.${nodeName}.locked.url or "https://github.com/edolstra/flake-compat/archive/${lock.nodes.${nodeName}.locked.rev}.tar.gz";
      sha256 = lock.nodes.${nodeName}.locked.narHash;
    }
  )
  { src = ./.; }
).shellNIx

[nicholaschiasson]

This shell could be cleaned up by not repackaging pumpkin instead letting the user build it themselves and utilising the rust-toolchain.toml.

This flake is not only a dev shell. I'm using the package feature of flakes to support using nix as the build system for those who desire it. This way, this repo could be pointed to as a flake package from other flakes or simply built with nix build, built and run with nix run, etc. Sorry, but I disagree with removing that configuration the way you suggest.

Also, my flake is already using rust-toolchain.toml. Maybe you just overlooked it?

shell.nix is a welcome addition though, I can add that later when I have some time.

[Eveeifyeve]

This shell could be cleaned up by not repackaging pumpkin instead letting the user build it themselves and utilising the rust-toolchain.toml.

This flake is not only a dev shell. I'm using the package feature of flakes to support using nix as the build system for those who desire it. This way, this repo could be pointed to as a flake package from other flakes or simply built with nix build, built and run with nix run, etc. Sorry, but I disagree with removing that configuration the way you suggest.

I feel like the package should be located at nixpkgs instead of here it doesn't make sense to build a package here, as when your cloning this repo you want to develop pumpkin not coming here to package pumpkin when that could be done in the nixpkgs side I disagree there shouldn't be a package in this repo when the package could be located at nixpkgs and you just use include it in the devshell here but if you want to build your local changes it should be a simple cargo build --release then cargo run --release to test your changes.

Also, my flake is already using rust-toolchain.toml. Maybe you just overlooked it?

Yes but it doesn’t override what you only need if there is a change that is made to the rust-toolchain.toml you wouldn’t need unnecessary dependencies.

[nicholaschiasson]

I agree nixpkgs should distribute the package. Obviously that depends on achieving a stable release at some point, so it cannot be worked on just yet anyway. But can you explain the harm in defining a flake package as well? That's arguably the core feature and incentive of using nix flakes. It's not just for defining a dev shell.

[Eveeifyeve]

I agree nixpkgs should distribute the package. Obviously that depends on achieving a stable release at some point, so it cannot be worked on just yet anyway. But can you explain the harm in defining a flake package as well? That's arguably the core feature and incentive of using nix flakes. It's not just for defining a dev shell.

I mean your just repeating yourself there is no harm.

[DataM0del]

Remove cargo.lock 🤮

[Eveeifyeve]

Not to be rude or anything @DataM0del, I don't think you have done research before making the review comments above this comment. Before making anymore review comments, could you please dedicate your time to do some research please and thanks.