Support dependencies with slashes in their names

When a dependency gets fetched from Git, it's possible for its name to
contain slashes - e.g. because the dependency's branch name contains
one:

```
something-something = { git = "...", branch = "JIRA-123/dev" }
```

Currently, because we don't sanitize those slashes, trying to compile
such `Cargo.toml` / `Cargo.lock` would fail - e.g. given a dependency
from branch `JIRA-123/dev`, we'd try to extract it into path
`JIRA-123/dev` instead of something flat such as `JIRA-123_dev`.

This commit fixes this issue by sanitizing all the `nkey`s - basically,
for all dependency paths, we replace `/` with `_`.

The choice of underscore is arbitrary, just seemed like a good fit.

build.nix

@@ -94,8 +94,17 @@ let
 
       unpack() {
         toml=$1; nkey=$2
+
+        # If a dependency gets fetched from Git, it's possible that its name
+        # will contain slashes (since Git allows for slashes in branch names).
+        #
+        # To properly handle those kind of dependencies, we have to sanitize
+        # their names first - in this case by replacing `/` with `_`.
+        nkey=''${nkey/\//_}
+
         # Most filesystems have a maximum filename length of 255
         dest="$out/$(echo "$nkey" | head -c 255)"
+
         if [ -d "$dest" ]; then
           log "Crate was already unpacked at $dest"
         else
@@ -132,6 +141,7 @@ let
 
         success=0
         tomls=$(find $checkout -name Cargo.toml)
+
         while read -r toml; do
           pname=$(cat $toml \
             | sed -n -e '/\[package\]/,$p' \
@@ -335,24 +345,21 @@ let
       runHook postConfigure
     '';
 
-    buildPhase =
+    buildPhase = ''
+      runHook preBuild
+      export SOURCE_DATE_EPOCH=1
 
-      ''
-        runHook preBuild
-        export SOURCE_DATE_EPOCH=1
+      cargo_ec=0
+      logRun ${cargoBuild} || cargo_ec="$?"
 
-        cargo_ec=0
-        logRun ${cargoBuild} || cargo_ec="$?"
+      if [ "$cargo_ec" -ne "0" ]; then
+        cat "$cargo_build_output_json" | jq -cMr 'select(.message.rendered != null) | .message.rendered'
+        log "cargo returned with exit code $cargo_ec, exiting"
+        exit "$cargo_ec"
+      fi
 
-        if [ "$cargo_ec" -ne "0" ]
-        then
-          cat "$cargo_build_output_json" | jq -cMr 'select(.message.rendered != null) | .message.rendered'
-          log "cargo returned with exit code $cargo_ec, exiting"
-          exit "$cargo_ec"
-        fi
-
-        runHook postBuild
-      '';
+      runHook postBuild
+    '';
 
     checkPhase = ''
       runHook preCheck
@@ -363,7 +370,6 @@ let
       runHook postCheck
     '';
 
-
     docPhase = lib.optionalString doDoc ''
       runHook preDoc
       export SOURCE_DATE_EPOCH=1
@@ -456,6 +462,7 @@ let
 
         runHook postInstall
       '';
+
     passthru = {
       # Handy for debugging
       inherit builtDependencies;

builtins/default.nix

@@ -12,7 +12,26 @@ rec
 
   readTOML = usePure: f:
     if usePure then
-      builtins.fromTOML (builtins.readFile f)
+      builtins.fromTOML (
+        # Safety: We invoke `unsafeDiscardStringContext` _after_ reading the
+        # file, so the derivation either had been already realized or `readFile`
+        # just realized it.
+        #
+        # Abstract: Discarding the context allows for users to provide `src`s
+        # that are dynamically generated and contain references to other
+        # derivations (e.g. via `pkgs.runCommand`); that's a pretty rare use
+        # case, but we need it - if nothing else - for our tests.
+        #
+        # Had we not discarded the context, and had user passed some string with
+        # one, `builtins.fromTOML` would fail with:
+        #
+        # ```
+        # the string '...' is not allowed to refer to a store path
+        # ```
+        builtins.unsafeDiscardStringContext (
+          builtins.readFile f
+        )
+      )
     else
       builtins.fromJSON (
         builtins.readFile (

test.nix

@@ -72,7 +72,6 @@ let
   fastTests =
     rec
     {
-
       readme = pkgs.runCommand "readme-gen" {}
         ''
           cat ${./README.tpl.md} > $out
@@ -174,9 +173,51 @@ let
       git-dep = naersk.buildPackage {
         doCheck = true;
         src = ./test/git-dep;
+      };
+
+      git-dep-by-branch = naersk.buildPackage {
+        doCheck = true;
+        src = ./test/git-dep-by-branch;
         cargoOptions = (opts: opts ++ [ "--locked" ]);
       };
 
+      git-dep-by-branch-with-slash =
+        let
+          dep = pkgs.runCommand "dep" {
+            buildInputs = [ pkgs.git ];
+          } ''
+            mkdir $out
+            cd $out
+            cp -ar ${./test/git-dep-by-branch-with-slash/dep}/* .
+
+            git init --initial-branch=with/slash
+            git add .
+            git config user.email 'someone'
+            git config user.name 'someone'
+            git commit -am 'Initial commit'
+          '';
+
+          app = pkgs.runCommand "app" {
+            buildInputs = [ pkgs.git ];
+          } ''
+            mkdir $out
+            cd $out
+            cp -ar ${./test/git-dep-by-branch-with-slash/app}/* .
+
+            depPath="${dep}"
+            depRev=$(cd ${dep} && git rev-parse HEAD)
+
+            sed "s:\$depPath:$depPath:" -is Cargo.*
+            sed "s:\$depRev:$depRev:" -is Cargo.*
+          '';
+
+        in
+        naersk.buildPackage {
+          doCheck = true;
+          src = app;
+          cargoOptions = (opts: opts ++ [ "--locked" ]);
+        };
+
       git-dep-by-tag = naersk.buildPackage {
         doCheck = true;
         src = ./test/git-dep-by-tag;

test/git-dep-by-branch-with-slash/README.md

@@ -0,0 +1,21 @@
+# Abstract
+
+This test ensures that we correctly handle dependencies with slashes in their
+names (as it requires some extra care around unpacking them).
+
+# Test
+
+`app` depends on `dep`, for which our Nix test-runner dynamically creates a Git
+repository with a branch called `with/slash`; naersk then builds `app`, and if
+the compilation succeeds, then everything must be working correctly.
+
+For this test to exist, we rely on one trick though:
+
+- Cargo doesn't support relative Git paths (such as `dep = { git = "file:../dep" }`),
+  but it does support _absolute_ paths - that's why `app`'s `Cargo.toml` and
+  `Cargo.lock` refer to `dep` via `$depPath` and `$depRev`; those variables are
+  substituted through Nix code.
+
+A bit unfortunately, that trick also means that this test cannot be run locally
+as it is - you'd have to open `app/Cargo.toml` and adjust `$depPath` to be an 
+actual, absolute path to `dep` on your machine.

test/git-dep-by-branch-with-slash/app/Cargo.lock

@@ -0,0 +1,15 @@
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+version = 3
+
+[[package]]
+name = "app"
+version = "0.1.0"
+dependencies = [
+ "dep",
+]
+
+[[package]]
+name = "dep"
+version = "0.1.0"
+source = "git+file://$depPath?branch=with/slash#$depRev"

test/git-dep-by-branch-with-slash/app/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "app"
+version = "0.1.0"
+edition = "2018"
+
+[dependencies]
+dep = { git = "file://$depPath", branch = "with/slash" }

test/git-dep-by-branch-with-slash/app/src/main.rs

@@ -0,0 +1,9 @@
+fn main() {
+    println!("Hello, world!");
+}
+
+#[cfg(test)]
+#[test]
+fn test() {
+    assert_eq!("with/slash", dep::branch());
+}

test/git-dep-by-branch-with-slash/dep/Cargo.toml

@@ -0,0 +1,4 @@
+[package]
+name = "dep"
+version = "0.1.0"
+edition = "2018"

test/git-dep-by-branch-with-slash/dep/src/lib.rs

@@ -0,0 +1,3 @@
+pub fn branch() -> &'static str {
+    "with/slash"
+}