Merge pull request #33 from willbush/feauter/consistent-relative-paths

Consistent relative paths to expression file

src/eval.rs

@@ -344,9 +344,12 @@ fn by_name_override(
     location: Location,
     relative_location_file: RelativePathBuf,
 ) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
+    let expected_package_path = structure::relative_file_for_package(attribute_name);
+
     let to_problem = |kind| {
         NixpkgsProblem::ByNameOverride(ByNameOverrideError {
             package_name: attribute_name.to_owned(),
+            expected_package_path: expected_package_path.to_owned(),
             file: relative_location_file,
             line: location.line,
             column: location.column,
@@ -364,14 +367,11 @@ fn by_name_override(
         (false, Some(_)) => to_problem(ByNameOverrideErrorKind::NonToplevelCallPackage).into(),
         // Something like `<attr> = pkgs.callPackage ...`
         (true, Some(syntactic_call_package)) => {
-            if let Some(actual_package_file) = syntactic_call_package.relative_path {
-                let expected_package_file = structure::relative_file_for_package(attribute_name);
-
-                if actual_package_file != expected_package_file {
+            if let Some(actual_package_path) = syntactic_call_package.relative_path {
+                if actual_package_path != expected_package_path {
                     // Wrong path
                     to_problem(ByNameOverrideErrorKind::WrongCallPackagePath {
-                        actual_path: actual_package_file,
-                        expected_path: expected_package_file,
+                        actual_path: actual_package_path,
                     })
                     .into()
                 } else {

src/nixpkgs_problem.rs

@@ -77,6 +77,7 @@ pub enum ByNameErrorKind {
 #[derive(Clone)]
 pub struct ByNameOverrideError {
     pub package_name: String,
+    pub expected_package_path: RelativePathBuf,
     pub file: RelativePathBuf,
     pub line: usize,
     pub column: usize,
@@ -88,10 +89,7 @@ pub struct ByNameOverrideError {
 pub enum ByNameOverrideErrorKind {
     NonSyntacticCallPackage,
     NonToplevelCallPackage,
-    WrongCallPackagePath {
-        actual_path: RelativePathBuf,
-        expected_path: RelativePathBuf,
-    },
+    WrongCallPackagePath { actual_path: RelativePathBuf },
     EmptyArgument,
     NonPath,
 }
@@ -234,47 +232,49 @@ impl fmt::Display for NixpkgsProblem {
             }
             NixpkgsProblem::ByNameOverride(ByNameOverrideError {
                 package_name,
+                expected_package_path,
                 file,
                 line,
                 column,
                 definition,
                 kind,
             }) => {
                 let relative_package_dir = structure::relative_dir_for_package(package_name);
-                let relative_package_file = structure::relative_file_for_package(package_name);
+                let expected_path_expr = create_path_expr(file, expected_package_path);
                 let indented_definition = indent_definition(*column, definition.clone());
 
                 match kind {
-                    ByNameOverrideErrorKind::NonSyntacticCallPackage =>
+                    ByNameOverrideErrorKind::NonSyntacticCallPackage => {
+
                         writedoc!(
                             f,
                             "
                             - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                                {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
+                                {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
                               However, in this PR, it isn't defined that way. See the definition in {file}:{line}
 
                             {indented_definition}
                             ",
-                        ),
+                        )
+                    }
                     ByNameOverrideErrorKind::NonToplevelCallPackage =>
                         writedoc!(
                             f,
                             "
                             - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                                {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
+                                {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
                               However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}:
 
                             {indented_definition}
                             ",
                         ),
-                    ByNameOverrideErrorKind::WrongCallPackagePath { actual_path, expected_path } => {
-                        let expected_path_expr = create_path_expr(file, expected_path);
+                    ByNameOverrideErrorKind::WrongCallPackagePath { actual_path } => {
                         let actual_path_expr = create_path_expr(file, actual_path);
-                        writedoc! {
+                        writedoc!(
                             f,
                             "
                             - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
@@ -285,15 +285,15 @@ impl fmt::Display for NixpkgsProblem {
 
                                 {package_name} = callPackage {actual_path_expr} {{ /* ... */ }};
                             ",
-                        }
+                        )
                     }
                     ByNameOverrideErrorKind::EmptyArgument =>
                         writedoc!(
                             f,
                             "
                             - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                                {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
+                                {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
                               However, in this PR, the second argument is empty. See the definition in {file}:{line}:
 
@@ -308,7 +308,7 @@ impl fmt::Display for NixpkgsProblem {
                             "
                             - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                                {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
+                                {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
                               However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}:
 

tests/alt-callPackage/expected

@@ -1,8 +1,8 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    foo = callPackage ./../by-name/fo/foo/package.nix { /* ... */ };
 
-  However, in this PR, a different `callPackage` is used. See the definition in all-packages.nix:5:
+  However, in this PR, a different `callPackage` is used. See the definition in pkgs/top-level/all-packages.nix:5:
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
 

tests/internalCallPackage/pkgs/top-level/all-packages.nix

@@ -0,0 +1,4 @@
+self: super: {
+  foo = self._internalCallByNamePackageFile ./../../foo.nix;
+  bar = self._internalCallByNamePackageFile ./../../foo.nix;
+}

tests/manual-definition/base/all-packages.nix → tests/manual-definition/base/pkgs/top-level/all-packages.nix

@@ -1,7 +1,7 @@
 self: super: {
   nonAttributeSet = null;
   nonCallPackage = self.someDrv;
-  internalCallByName = self._internalCallByNamePackageFile ./some-pkg.nix;
+  internalCallByName = self._internalCallByNamePackageFile ./../../some-pkg.nix;
   nonDerivation = self.callPackage ({ }: { }) { };
 
   onlyMove = self.callPackage ({ someDrv }: someDrv) { };

tests/manual-definition/expected

@@ -1,20 +1,20 @@
 - Because pkgs/by-name/no/noEval exists, the attribute `pkgs.noEval` must be defined like
 
-    noEval = callPackage ./pkgs/by-name/no/noEval/package.nix { /* ... */ };
+    noEval = callPackage ./../by-name/no/noEval/package.nix { /* ... */ };
 
-  However, in this PR, the second argument is empty. See the definition in all-packages.nix:9:
+  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:9:
 
-    noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+    noEval = self.callPackage ./../by-name/no/noEval/package.nix { };
 
   Such a definition is provided automatically and therefore not necessary. Please remove it.
 
 - Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
 
-    onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
+    onlyMove = callPackage ./../by-name/on/onlyMove/package.nix { /* ... */ };
 
-  However, in this PR, the second argument is empty. See the definition in all-packages.nix:7:
+  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:7:
 
-    onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
+    onlyMove = self.callPackage ./../by-name/on/onlyMove/package.nix { };
 
   Such a definition is provided automatically and therefore not necessary. Please remove it.
 

tests/manual-definition/all-packages.nix → tests/manual-definition/pkgs/top-level/all-packages.nix

@@ -4,7 +4,7 @@ self: super: {
   internalCallByName = self.callPackage ({ someDrv }: someDrv) { };
   nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
 
-  onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
+  onlyMove = self.callPackage ./../by-name/on/onlyMove/package.nix { };
 
-  noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+  noEval = self.callPackage ./../by-name/no/noEval/package.nix { };
 }

tests/mock-nixpkgs.nix

@@ -81,8 +81,8 @@ let
 
   # A list optionally containing the `all-packages.nix` file from the test case as an overlay
   optionalAllPackagesOverlay =
-    if builtins.pathExists (root + "/all-packages.nix") then
-      [ (import (root + "/all-packages.nix")) ]
+    if builtins.pathExists (root + "/pkgs/top-level/all-packages.nix") then
+      [ (import (root + "/pkgs/top-level/all-packages.nix")) ]
     else
       [ ];
 

tests/move-to-non-by-name/expected

@@ -1,13 +1,13 @@
-- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in all-packages.nix.
+- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in pkgs/top-level/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in all-packages.nix.
+- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in pkgs/top-level/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in all-packages.nix.
+- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in pkgs/top-level/all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
-- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in all-packages.nix.
+- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in pkgs/top-level/all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/move-to-non-by-name/all-packages.nix → tests/move-to-non-by-name/pkgs/top-level/all-packages.nix

@@ -1,10 +1,10 @@
 self: super: {
   foo1 = self.callPackage ({ someDrv }: someDrv) { };
-  foo2 = self.callPackage ./without-config.nix { };
+  foo2 = self.callPackage ./../../without-config.nix { };
   foo3 = self.callPackage ({ someDrv, enableFoo }: someDrv) {
     enableFoo = null;
   };
-  foo4 = self.callPackage ./with-config.nix {
+  foo4 = self.callPackage ./../../with-config.nix {
     enableFoo = null;
   };
 }

tests/new-package-non-by-name/expected

@@ -1,21 +1,21 @@
 - Attribute `pkgs.new1` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new1/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in pkgs/top-level/all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new2` is a new top-level package using `pkgs.callPackage ./without-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new2/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in pkgs/top-level/all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new3` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new3/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in pkgs/top-level/all-packages.nix is still needed.
 
 - Attribute `pkgs.new4` is a new top-level package using `pkgs.callPackage ./with-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new4/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in pkgs/top-level/all-packages.nix is still needed.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/new-package-non-by-name/all-packages.nix → tests/new-package-non-by-name/pkgs/top-level/all-packages.nix

@@ -1,11 +1,11 @@
 self: super: {
   before = self.callPackage ({ someDrv }: someDrv) { };
   new1 = self.callPackage ({ someDrv }: someDrv) { };
-  new2 = self.callPackage ./without-config.nix { };
+  new2 = self.callPackage ./../../without-config.nix { };
   new3 = self.callPackage ({ someDrv, enableNew }: someDrv) {
     enableNew = null;
   };
-  new4 = self.callPackage ./with-config.nix {
+  new4 = self.callPackage ./../../with-config.nix {
     enableNew = null;
   };
 }

tests/non-syntactical-callPackage-by-name/expected

@@ -1,8 +1,8 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    foo = callPackage ./../by-name/fo/foo/package.nix { /* ... */ };
 
-  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:4
+  However, in this PR, it isn't defined that way. See the definition in pkgs/top-level/all-packages.nix:4
 
     foo = self.bar;
 

tests/non-syntactical-callPackage-by-name/pkgs/top-level/all-packages.nix

@@ -0,0 +1,6 @@
+self: super: {
+
+  bar = (x: x) self.callPackage ./../by-name/fo/foo/package.nix { someFlag = true; };
+  foo = self.bar;
+
+}

tests/override-different-file/expected

@@ -1,9 +1,9 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./../by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in all-packages.nix:2:
+  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in pkgs/top-level/all-packages.nix:2:
 
-    nonDerivation = callPackage ./someDrv.nix { /* ... */ };
+    nonDerivation = callPackage ./../../someDrv.nix { /* ... */ };
 
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

tests/override-different-file/pkgs/top-level/all-packages.nix

@@ -0,0 +1,3 @@
+self: super: {
+  nonDerivation = self.callPackage ./../../someDrv.nix { };
+}

tests/override-empty-arg-gradual/base/pkgs/top-level/all-packages.nix

@@ -0,0 +1,3 @@
+self: super: {
+  nonDerivation = self.callPackage ./../by-name/no/nonDerivation/package.nix { };
+}

tests/override-empty-arg-gradual/pkgs/top-level/all-packages.nix

@@ -0,0 +1,3 @@
+self: super: {
+  nonDerivation = self.callPackage ./../by-name/no/nonDerivation/package.nix { };
+}

tests/override-empty-arg/expected

@@ -1,10 +1,10 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./../by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
+  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:2:
 
-    nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
+    nonDerivation = self.callPackage ./../by-name/no/nonDerivation/package.nix { };
 
   Such a definition is provided automatically and therefore not necessary. Please remove it.
 

tests/override-empty-arg/pkgs/top-level/all-packages.nix

@@ -0,0 +1,3 @@
+self: super: {
+  nonDerivation = self.callPackage ./../by-name/no/nonDerivation/package.nix { };
+}

tests/override-no-call-package/expected

@@ -1,8 +1,8 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./../by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:2
+  However, in this PR, it isn't defined that way. See the definition in pkgs/top-level/all-packages.nix:2
 
     nonDerivation = self.someDrv;
 

tests/override-no-file/expected

@@ -1,8 +1,8 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./../by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:2:
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in pkgs/top-level/all-packages.nix:2:
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };