Better error for outside references (#97)

NixOS · Aug 29, 2024 · 480c5a7 · 480c5a7
2 parents 6eec7b1 + bed457e
commit 480c5a7
Show file tree

Hide file tree

Showing 24 changed files with 76 additions and 54 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "nixpkgs-check-by-name"
-version = "0.1.2"
+version = "0.1.3"
 edition = "2021"
 
 [dependencies]

diff --git a/src/main.rs b/src/main.rs
@@ -176,7 +176,7 @@ mod tests {
         test_nixpkgs(
             "case_sensitive",
             path,
-            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\n\
+            "- pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\n\
             This PR introduces the problems listed above. Please fix them before merging, \
             otherwise the base branch would break.\n",
         );

diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs
@@ -158,17 +158,17 @@ impl fmt::Display for NixpkgsProblem {
                     ShardErrorKind::ShardNonDir =>
                         write!(
                             f,
-                            "{relative_shard_path}: This is a file, but it should be a directory.",
+                            "- {relative_shard_path}: This is a file, but it should be a directory.",
                         ),
                     ShardErrorKind::InvalidShardName =>
                         write!(
                             f,
-                            "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
+                            "- {relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
                         ),
                     ShardErrorKind::CaseSensitiveDuplicate { first, second } =>
                         write!(
                             f,
-                            "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
+                            "- {relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
                         ),
                 }
             }
@@ -181,28 +181,28 @@ impl fmt::Display for NixpkgsProblem {
                         let relative_package_dir = structure::relative_dir_for_package(package_name);
                         write!(
                             f,
-                            "{relative_package_dir}: This path is a file, but it should be a directory.",
+                            "- {relative_package_dir}: This path is a file, but it should be a directory.",
                         )
                     }
                     PackageErrorKind::InvalidPackageName { invalid_package_name } =>
                         write!(
                             f,
-                            "{relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
+                            "- {relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
                         ),
                     PackageErrorKind::IncorrectShard { correct_relative_package_dir } =>
                         write!(
                             f,
-                            "{relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.",
+                            "- {relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.",
                         ),
                     PackageErrorKind::PackageNixNonExistent =>
                         write!(
                             f,
-                            "{relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
+                            "- {relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
                         ),
                     PackageErrorKind::PackageNixDir =>
                         write!(
                             f,
-                            "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
+                            "- {relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
                         ),
                 }
             }
@@ -215,25 +215,25 @@ impl fmt::Display for NixpkgsProblem {
                         let relative_package_file = structure::relative_file_for_package(attribute_name);
                         write!(
                             f,
-                            "pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}",
+                            "- pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}",
                         )
                     }
                     ByNameErrorKind::NonDerivation => {
                         let relative_package_file = structure::relative_file_for_package(attribute_name);
                         write!(
                             f,
-                            "pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation",
+                            "- pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation",
                         )
                     }
                     ByNameErrorKind::InternalCallPackageUsed =>
                         write!(
                             f,
-                            "pkgs.{attribute_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.",
+                            "- pkgs.{attribute_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.",
                         ),
                     ByNameErrorKind::CannotDetermineAttributeLocation =>
                         write!(
                             f,
-                            "pkgs.{attribute_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.",
+                            "- pkgs.{attribute_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.",
                         ),
                 }
             }
@@ -333,12 +333,12 @@ impl fmt::Display for NixpkgsProblem {
                     PathErrorKind::OutsideSymlink =>
                         write!(
                             f,
-                            "{relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.",
+                            "- {relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.",
                         ),
                     PathErrorKind::UnresolvableSymlink { io_error } =>
                         write!(
                             f,
-                            "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.",
+                            "- {relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.",
                         ),
                 }
             },
@@ -353,22 +353,30 @@ impl fmt::Display for NixpkgsProblem {
                     NixFileErrorKind::PathInterpolation =>
                         write!(
                             f,
-                            "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.",
+                            "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.",
                         ),
                     NixFileErrorKind::SearchPath =>
                         write!(
                             f,
-                            "{relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.",
+                            "- {relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.",
                         ),
                     NixFileErrorKind::OutsidePathReference =>
-                        write!(
+                        writedoc!(
                             f,
-                            "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.",
+                            "
+                            - {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.
+                              This is undesirable because it creates dependencies between internal paths, making it harder to reorganise Nixpkgs in the future.
+                              Alternatives include:
+                              - If you are creating a new version of a package with a common file between versions, consider following the recommendation in https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions.
+                              - If the path being referenced could be considered a stable interface with multiple uses, consider exposing it via a `pkgs` attribute, then taking it as a attribute argument in {PACKAGE_NIX_FILENAME}.
+                              - If the path being referenced is internal and has multiple uses, consider passing the file as an explicit `callPackage` argument in `pkgs/top-level/all-packages.nix`.
+                              - If the path being referenced is internal and will need to be modified independently of the original, consider copying it into the {relative_package_dir} directory.
+                            "
                         ),
                     NixFileErrorKind::UnresolvablePathReference { io_error } =>
                         write!(
                             f,
-                            "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.",
+                            "- {relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.",
                         ),
                 }
             },

diff --git a/tests/base-still-broken/expected b/tests/base-still-broken/expected
@@ -1,3 +1,3 @@
-pkgs/by-name/bar: This is a file, but it should be a directory.
+- pkgs/by-name/bar: This is a file, but it should be a directory.
 The base branch is broken and still has above problems with this PR, which need to be fixed first.
 Consider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.
diff --git a/tests/broken-autocall/expected b/tests/broken-autocall/expected
@@ -1,2 +1,2 @@
-pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
+- pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/incorrect-shard/expected b/tests/incorrect-shard/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
+- pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/internalCallPackage/expected b/tests/internalCallPackage/expected
@@ -1,2 +1,2 @@
-pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
+- pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/invalid-package-name/expected b/tests/invalid-package-name/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
+- pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/invalid-shard-name/expected b/tests/invalid-shard-name/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
+- pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/missing-package-nix/expected b/tests/missing-package-nix/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/fo/foo: Missing required "package.nix" file.
+- pkgs/by-name/fo/foo: Missing required "package.nix" file.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/multiple-failures/expected b/tests/multiple-failures/expected
@@ -1,14 +1,21 @@
-pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
-pkgs/by-name/A/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
-pkgs/by-name/A/fo@: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
-pkgs/by-name/A/fo@: Path package.nix is a symlink pointing to a path outside the directory of that package.
-pkgs/by-name/aa: This is a file, but it should be a directory.
-pkgs/by-name/ba/bar: This path is a file, but it should be a directory.
-pkgs/by-name/ba/baz: "package.nix" must be a file.
-pkgs/by-name/ba/foo: Incorrect directory location, should be pkgs/by-name/fo/foo instead.
-pkgs/by-name/ba/foo: File package.nix at line 4 contains the path expression "/bar" which cannot be resolved: No such file or directory (os error 2).
-pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "../." which may point outside the directory of that package.
-pkgs/by-name/ba/foo: File package.nix at line 6 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
-pkgs/by-name/ba/foo: File package.nix at line 7 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
-pkgs/by-name/fo/foo: Missing required "package.nix" file.
+- pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
+- pkgs/by-name/A/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
+- pkgs/by-name/A/fo@: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
+- pkgs/by-name/A/fo@: Path package.nix is a symlink pointing to a path outside the directory of that package.
+- pkgs/by-name/aa: This is a file, but it should be a directory.
+- pkgs/by-name/ba/bar: This path is a file, but it should be a directory.
+- pkgs/by-name/ba/baz: "package.nix" must be a file.
+- pkgs/by-name/ba/foo: Incorrect directory location, should be pkgs/by-name/fo/foo instead.
+- pkgs/by-name/ba/foo: File package.nix at line 4 contains the path expression "/bar" which cannot be resolved: No such file or directory (os error 2).
+- pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "../." which may point outside the directory of that package.
+  This is undesirable because it creates dependencies between internal paths, making it harder to reorganise Nixpkgs in the future.
+  Alternatives include:
+  - If you are creating a new version of a package with a common file between versions, consider following the recommendation in https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions.
+  - If the path being referenced could be considered a stable interface with multiple uses, consider exposing it via a `pkgs` attribute, then taking it as a attribute argument in package.nix.
+  - If the path being referenced is internal and has multiple uses, consider passing the file as an explicit `callPackage` argument in `pkgs/top-level/all-packages.nix`.
+  - If the path being referenced is internal and will need to be modified independently of the original, consider copying it into the pkgs/by-name/ba/foo directory.
+
+- pkgs/by-name/ba/foo: File package.nix at line 6 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
+- pkgs/by-name/ba/foo: File package.nix at line 7 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
+- pkgs/by-name/fo/foo: Missing required "package.nix" file.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/non-attrs/expected b/tests/non-attrs/expected
@@ -1,2 +1,2 @@
-pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+- pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/non-derivation/expected b/tests/non-derivation/expected
@@ -1,2 +1,2 @@
-pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+- pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/package-dir-is-file/expected b/tests/package-dir-is-file/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
+- pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/package-nix-dir/expected b/tests/package-nix-dir/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/fo/foo: "package.nix" must be a file.
+- pkgs/by-name/fo/foo: "package.nix" must be a file.
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/ref-absolute/expected b/tests/ref-absolute/expected
@@ -1,2 +1,2 @@
-pkgs/by-name/aa/aa: File package.nix at line 1 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
+- pkgs/by-name/aa/aa: File package.nix at line 1 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/tests/ref-escape/expected b/tests/ref-escape/expected
@@ -1,2 +1,9 @@
-pkgs/by-name/aa/aa: File package.nix at line 1 contains the path expression "../." which may point outside the directory of that package.
+- pkgs/by-name/aa/aa: File package.nix at line 1 contains the path expression "../." which may point outside the directory of that package.
+  This is undesirable because it creates dependencies between internal paths, making it harder to reorganise Nixpkgs in the future.
+  Alternatives include:
+  - If you are creating a new version of a package with a common file between versions, consider following the recommendation in https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions.
+  - If the path being referenced could be considered a stable interface with multiple uses, consider exposing it via a `pkgs` attribute, then taking it as a attribute argument in package.nix.
+  - If the path being referenced is internal and has multiple uses, consider passing the file as an explicit `callPackage` argument in `pkgs/top-level/all-packages.nix`.
+  - If the path being referenced is internal and will need to be modified independently of the original, consider copying it into the pkgs/by-name/aa/aa directory.
+
 This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.