Introduce nix flake

[untio11]

This flake captures the external dependencies for this project (rust toolchain, additional cargo packages) in addition to providing build instructions to build this package through the nix package manager.

An example of using this flake as an input to another one:

{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs/nixos-unstable";
    firestore-emulator = {
      # Currently using `/nixify` branch, but that part can be left out when merged with master:
      url = "github:skunkteam/rust-firestore-emulator/nixify";
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };
  outputs =
    {
      nixpkgs,
      firestore-emulator,
      ...
    }:
    {
      devShells.aarch64-darwin.default =
        let
          system = "aarch64-darwin";
          pkgs = import nixpkgs { inherit system; };
        in
        pkgs.mkShell {
          name = "devEnv";
          # Puts `firestore-emulator` in path:
          packages = [ firestore-emulator.defaultPackage.${system} ];
        };
    };
}

Or running firestore-emulator directly from the flake:

nix run github:skunkteam/rust-firestore-emulator/nixify -- --host-port 127.0.0.1:8080

[untio11]

/gemini review

[pavadeli]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Nix flake to manage the project's dependencies and build environment, which is a great improvement for ensuring reproducibility. The implementation is solid, but I've found a few issues that need addressing. There's a critical issue in the flake.nix patchPhase that will break the build, and a high-severity logic error in the justfile helper script for updating submodules. I've also noted that a required Rust flag (tokio_unstable) seems to be missing from the new Nix environment, and there's a minor code duplication in flake.nix that could be refactored for better maintainability. Overall, great work on moving to Nix, and with these fixes, it should be ready to merge.

[pavadeli]

Code Review

This pull request introduces a Nix flake to manage the project's dependencies and build environment, which is a great improvement for ensuring reproducibility. The implementation is solid, but I've found a few issues that need addressing. There's a critical issue in the flake.nix patchPhase that will break the build, and a high-severity logic error in the justfile helper script for updating submodules. I've also noted that a required Rust flag (tokio_unstable) seems to be missing from the new Nix environment, and there's a minor code duplication in flake.nix that could be refactored for better maintainability. Overall, great work on moving to Nix, and with these fixes, it should be ready to merge.

@gemini-code-assist, please create comments and suggestions, those are missing from your review.

[gemini-code-assist]

Hello @pavadeli,

Thank you for reaching out and clarifying your request. My apologies for the initial high-level summary. You are absolutely right; a detailed review with specific comments and code suggestions is much more helpful. I've now gone through the pull request with a fine-tooth comb, focusing on the issues I highlighted previously, and have provided actionable feedback below.

EDIT @pavadeli : I've removed incorrect suggestions, the rest is for @untio11 to consider.

---

Detailed Code Review

Here's a breakdown of the issues identified, along with their severity and suggested solutions:

• Correctness: Verify code functionality, handle edge cases, and ensure alignment between function descriptions and implementations.
• Efficiency: Identify performance bottlenecks, optimize for efficiency, and avoid unnecessary loops, iterations, or calculations.
• Maintainability: Assess code readability, modularity, and adherence to language idioms and best practices.

---

1. Critical: Incorrect patchPhase in flake.nix

EDIT: Incorrect assumptions

Description: The patchPhase in flake.nix attempts to replace the relative path "include:" in crates/googleapis/build.rs with the absolute path to the googleapis flake input. However, the googleapis input points to the root of the googleapis repository, and the actual include files are located within a subdirectory (specifically google/api). The current sed command will result in an incorrect path, causing the build to fail.

Severity: critical

File: flake.nix

Suggestion: You need to append the correct subdirectory path (/google/api) to the googleapis variable in the sed command to point to the actual location of the include files within the fetched googleapis repository.

--- a/flake.nix
+++ b/flake.nix
@@ -106,7 +106,7 @@
           # code before building.
           patchPhase = ''
             runHook prePatch
-            sed -i 's:"include:":"${googleapis}:g' crates/googleapis/build.rs
+            sed -i 's:"include:":"${googleapis}/google/api:g' crates/googleapis/build.rs
             runHook postPatch
           ';
         };

2. High: Logic Error in justfile Submodule Update

EDIT: really confused LLM (funny actually)

Description: The update-submodule recipe in justfile calls update-flake-input before updating the git submodule itself. This means update-flake-input will read the old commit hash of the submodule and update flake.nix with it, even though git submodule update --remote will subsequently fetch the new commit. This leads to flake.nix being out of sync with the actual submodule state.

Severity: high

File: justfile

Suggestion: Reverse the order of operations in the update-submodule recipe. First, update the git submodule, and then update the flake.nix with the newly fetched commit hash.

--- a/justfile
+++ b/justfile
@@ -87,10 +87,10 @@
 # Update the googleapis submodule to their latest commit and patch the commit hash in flake.nix.
 # Also staging changes in git to ensure things stay in sync. 
 update-submodule: && update-flake-input
-    #!/usr/bin/env sh
-    git submodule update --remote
-    git add './crates/googleapis/include'
+    #!/usr/bin/env sh
+    git submodule update --remote
+    git add './crates/googleapis/include'
 
 # Just patches the current commit hash into flake.nix
 update-flake-input:
     #!/usr/bin/env sh

Correction: My apologies, the just syntax && means the second recipe runs after the first one completes. The issue is that update-flake-input is called as a prerequisite, meaning it runs before the main update-submodule commands. The fix is to move the update-flake-input call to after the git submodule update --remote command within the update-submodule recipe itself, and remove it as a prerequisite.

--- a/justfile
+++ b/justfile
@@ -87,10 +87,10 @@
 # Update the googleapis submodule to their latest commit and patch the commit hash in flake.nix.
 # Also staging changes in git to ensure things stay in sync. 
 update-submodule:
     #!/usr/bin/env sh
     git submodule update --remote
     git add './crates/googleapis/include'
+    just update-flake-input
 
 # Just patches the current commit hash into flake.nix
 update-flake-input:
     #!/usr/bin/env sh
     # Get the commit hash of the current version of the submodule:
     # The git command sometimes returns +${commit_hash}, so use sed to strip the '+' if it's there

3. High: Missing tokio_unstable Rust Flag

EDIT: Only if we ever consider adding the `console` feature to the build defined in nix. Otherwise not necessary

Description: The .cargo/config.toml file, which previously set the tokio_unstable rustflag, has been removed. This flag is often necessary for using certain unstable features in the Tokio runtime. Without it, builds might fail or behave unexpectedly if such features are still in use by the project.

Severity: high

File: flake.nix

Suggestion: To ensure the tokio_unstable flag is applied during the build process within the Nix environment, you should set the RUSTFLAGS environment variable for the defaultPackage derivation. This ensures consistency with the previous build setup.

--- a/flake.nix
+++ b/flake.nix
@@ -90,6 +90,7 @@
         # `nix run . -- ARGS` runs the firestore-emulator binary with the provided arguments.
         # For example: `nix run . -- --host-port 127.0.0.1:8080` to start the emulator on localhost.
         defaultPackage = rust-builder.buildPackage {
+          RUSTFLAGS = "--cfg tokio_unstable";
           src = ./.;
           # naersk can't deal with `version = { workspace = true}` for the root package, so extract it
           # manually:

4. Low: Code Duplication in flake.nix

Description: The buildInputs for devShells.default and nativeBuildInputs for defaultPackage contain an identical set of core dependencies (openssl, pkg-config, rust-toolchain, protobuf). This duplication can make the file harder to maintain, as changes to these common dependencies would need to be applied in two places.

Severity: low

File: flake.nix

Suggestion: Extract these common dependencies into a shared variable within the let block. This improves maintainability and readability by centralizing the definition of these inputs.

--- a/flake.nix
+++ b/flake.nix
@@ -55,6 +55,13 @@
           cargo = rust-toolchain;
           rustc = rust-toolchain;
         };
+        # Common build inputs for both the development shell and the package build.
+        commonBuildInputs = with pkgs; [
+          openssl
+          pkg-config
+          rust-toolchain
+          protobuf
+        ];
       in
       {
         # Define the development environment with the necessary dependencies and rust toolchain:
@@ -65,11 +72,7 @@

           # Packages necessary for building the firestore-emulator crate. Made available in the
           # shell's PATH.
-          buildInputs = with pkgs; [
-            openssl
-            pkg-config
-            rust-toolchain
-            protobuf
-          ];
+          buildInputs = commonBuildInputs;
           # Other project dependencies to be made avaible in PATH:
           packages = with pkgs; [
             # Nix language server and formatter.
@@ -95,11 +98,7 @@
           src = ./.;
           # naersk can't deal with `version = { workspace = true}` for the root package, so extract it
           # manually:
-          version = with builtins; (fromTOML (readFile ./Cargo.toml)).workspace.package.version;
-          nativeBuildInputs = with pkgs; [
-            protobuf
-            openssl
-            pkg-config
-            rust-toolchain
-          ];
+          version = with builtins; (fromTOML (readFile ./Cargo.toml)).workspace.package.version;
+          nativeBuildInputs = commonBuildInputs;
           # Workaround - build.rs refers to git submodule path `./crates/googleapis/include`, but this
           # doesn't get copied over to the nix store for some reason. This patch replaces the reference
           # to local directory `include` by the absolute path to the nix store in the build.rs source

---

I hope this detailed feedback is more helpful for addressing the identified issues. Please let me know if you have any further questions or need clarification on these points.

[untio11]

Clippy check is failing on ./crates/fifo-rwlock/src/lib.rs, but not locally. I suspect because the github action is using dtolnay/rust-toolchain@stable and the latest stable rust version is 1.89, while locally we're on 1.87.

Ideally, I'd have github actions use the toolchain from the flake. There's this action that basically loads the flake devshell. See the Why section for relevant rust examples.

Edit: indeed, after updating the rust version to 1.89.0 (via nix flake update rust-overlay), cargo clippy --workspace produces two warnings, one of which is the one that causes the github action to fail:

warning: this `if` statement can be collapsed
   --> crates/emulator-database/src/database/query.rs:551:9
    |
551 | /         if let Some(filter) = &self.filter {
552 | |             if !filter.eval(doc)? {
553 | |                 return Ok(false);
554 | |             }
555 | |         }
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
    = note: `#[warn(clippy::collapsible_if)]` on by default
help: collapse nested if block
    |
551 ~         if let Some(filter) = &self.filter
552 ~             && !filter.eval(doc)? {
553 |                 return Ok(false);
554 ~             }
    |

warning: hiding a lifetime that's elided elsewhere is confusing
  --> crates/emulator-database/src/database/document.rs:59:30
   |
59 |     pub(crate) async fn read(&self) -> Result<DocumentContentsReadGuard> {
   |                              ^^^^^            ------------------------- the same lifetime is hidden here
   |                              |
   |                              the lifetime is elided here
   |
   = help: the same lifetime is referred to in inconsistent ways, making the signature confusing
   = note: `#[warn(mismatched_lifetime_syntaxes)]` on by default
help: use `'_` for type paths
   |
59 |     pub(crate) async fn read(&self) -> Result<DocumentContentsReadGuard<'_>> {
   |   

[pavadeli]

LGTM