Implement the rest of the checks

[jtojnar]

Those mentioned in NixOS/ofborg#245 (comment)
and in my saved replies https://github.com/settings/replies

• debian-sources
  Salsa is better since it is a git repository – we can pin it to a commit and it will always be there. Sources might remove older debian releases after they are no longer supported.

• attr-ordering attribute-ordering: Add attribute ordering check #6
  We prefer having all the phases in the bottom, see https://discourse.nixos.org/t/document-attribute-ordering-in-package-expressions/4887.

• attr-typos Add 'attribute-typo' check #29
  Check for typos as suggested in https://discourse.nixos.org/t/create-development-environment-based-on-shell-nix/10035/3?u=jtojnar, can be based on attr-ordering overlay, just check each attribute is not in known attributes && (its lowercase is in lowercased known attributes || has levenshtein distance ≤ 4 to any known attribute)

• avoid-with
  with statement makes it harder to reason about code. Prefer using qualified names, let with inherit or narrow the scope of with as much as possible.

• cmake-meson
  Are you sure cmake is needed? Meson supports it for finding dependencies but typically, pkg-config will be sufficient.

• explain-deps
  Also could you add a comment to libraries, why they are needed during build time? For example gtk3 # needed for gtk-update-icon-cache.

• explicit-phases
  When hardcoded phases like buildPhase or installPhase invoking make are used (suggest makeFlags)

• flags-array-in-nix Add 'no-flags-array' check #30
  FlagsArray should not be used in Nix, it is a bash thing: *FlagsArray do not work as people expect them to NixOS/nixpkgs#112054 (comment)

• flags-no-spaces Add 'no-flags-array' check #30
  `makeFlags & co. will be intercalated with spaces and then split again in bash, resulting in wrong arguments being applied.

• missing-hooks
  When overriding installPhase, you should also provide runHook preInstall and runHook postInstall. Though, in this case, it is better to rely on the default installPhase.

• ld_library_path
  LD_LIBRARY_PATH should be avoided when necessary, since it can lead to weird and difficult to diagnose error when it loads a different version of library than the program is compiled against. Remember environment variables are inherited by child processes and LD_LIBRARY_PATH has higher priority than DT_RUNPATH ELF entry we use to locate libraries. For a concrete example when this occurs: If you have a URL hyperlink in the application and click it, it will open a browser child process, which will inherit these X libraries. If you use browser from a different channel, it will likely crash.

• multiple-lines
  Could you make the list one dependency per line? It will make diffs nicer to read. nixpkgs-fmt can help you with that.

• outputs ordering
  Output containing binaries should be generally first (i.e. bin or out).

• parallel meson/cmake
  enableParallelBuilding is enabled by default in meson and cmake packages.

• no-aliases
  Try to eval with allowAliases = false

• quote-urls Add 'no-uri-literals' check #28
  Unquoted URLs are being phased out: [RFC 0045] Deprecating unquoted URL syntax NixOS/rfcs#45

• rev-tag
  It is preferred to use a tag rev = "v${version}" when available. It will make updating easier in the future.

• build-tools-in-build-inputs
  When known build tool/setup hook is in buildInputs instead of nativeBuildInputs
  cmake, ninja, meson, autoconf, automake, libtool, intltool, gettext, appstream-glib, gobject-introspection, wrapGAppsHook, wrapQtAppsHook…

• missing-patch-comment Add missing-patch-comment check using rust and rnix #16
  When a patch is added to patches but no comment describing it is nearby.

• improbable-package
  When using improbable package variant (e.g. gtk3-x11 – here we could check if the source actually depends on this rather than gtk3)

• intree-patch
  When patch file is added to nixpkgs (suggest fetchpatch)

• vcs-naming-mismatch
  When fetching from git with rev=${commithash} but ${version} ≠ unstable-yyyy-mm-dd

  • unstable should be in version? treewide: split name into pname/version for unstable packages NixOS/nixpkgs#88023 (comment)

• verbose-git-rev
  When fetching from git with rev=refs/tags/${version} (replace with rev=${version})

• name-instead-of-pname
  When name="foo-${version}" is used. Suggest pname

• upstream-desktop-file
  One of the reasons desktop files were conceived was so that distros do not have to create launchers of their own and the effort can be done once upstream. Instead of using makeDesktopItem, consider opening an issue upstream about creating and installing a desktop file or sending a patch that does that.

  • Examples
    • in a qmake-based project: optional INSTALL_PATH qmake flag (on unix) jamulussoftware/jamulus#247

• dbus-xml-location
  D-Bus XML files should go to datadir not sysconfdir https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/76

• avoid-static-libraries
  Unless stdenv.targetPlatform.isStatic is true Make pkgsStatic set "static" argument to true NixOS/nixpkgs#96223

• maintainer-missing Add two simple checks from #1 #26
  Check that meta.maintainers is present

• license-missing Add two simple checks from #1 #26
  Check that meta.license is present

• platforms-missing
  Check that meta.platforms is present

• shellcheck
  Use shellcheck on phases: Crazy idea: run ShellCheck on scripts, mkDerivation, runCommand, etc. NixOS/nixpkgs#21166, https://github.com/Fuuzetsu/shellcheck-nix-attributes

• targeted-comment
  Comments should be co-located with their subject – not before patches, postPatch, … but inside, so that when more items are added, it does not look like the comment belongs to the whole attribute.

• redundant-hash-type
  Either use sha256 = "…"; or hash = "sha256-…";, not sha256 = "sha256-…";. Same for vendorSha256…

• more-specific-fetcher
  Suggest using fetchGitLab instead of fetchgit for GitLab domains.

• overzealous-patch-shebangs
  Patching shebangs indiscriminately in the whole source tree is not recommended. You may accidentally patch a run time script and then it will use build interpreter (instead of being fixed to host interpreter by patchShebangsAuto in fixupPhase).

[rmcgibbo]

Four of the checks mentioned here (pkg-config, maintainer-missing, license-missing, platforms-missing) seem very easy to implement, but you've specifically marked them for being new packages only.

Within the scope of nixpkgs-hammering, it's not obvious how we can know whether a package is new. However, within something like nixpkgs-review, that information is known. (I'm working on integrating nixpkgs-hammering into my fork of nixpkgs-review).

[rmcgibbo]

missing-patch-comment
When a patch is added to patches but no comment describing it is nearby.

This seems like a very good idea, but unless I'm mistaken, I don't think it can be done with the current approach. For checks that apply mainly on the source code or AST level and can't be performed during evaluation, it seems like perhaps using rnix-parser would be the best approach. Would you want to add that to this project, or would that be out of scope?

[jtojnar]

Four of the checks mentioned here (pkg-config, maintainer-missing, license-missing, platforms-missing) seem very easy to implement, but you've specifically marked them for being new packages only.

I guess this distinction does not really make sense. Most of the checks are for maintainer to consider but not currently required by a coding style. I guess I added this note before I figured out how to only check specific attributes.

And the pkg-config is now officially an alias: https://discourse.nixos.org/t/pkgconfig-alias-has-been-officially-aliased/11066 so we could just set config.allowAliases = false.

This seems like a very good idea, but unless I'm mistaken, I don't think it can be done with the current approach. For checks that apply mainly on the source code or AST level and can't be performed during evaluation, it seems like perhaps using rnix-parser would be the best approach. Would you want to add that to this project, or would that be out of scope?

Right, we will want to check AST at some point. I just did not look into the options yet. Want to know which of hnix, rnix, and tree-sitter fits the best.

[rmcgibbo]

I have a prototype of the patch comment check with rust / rnix that I’m working on. I’ll post it as a draft shortly.

[mweinelt]

verbose-git-rev
When fetching from git with rev=refs/tags/${version} (replace with rev=${version})

The verbose reference format prevents branch/tag confusion, when multiple refs with the same shortname (tags vs branches) exist. I think verbose is better, while we carry this ambiguity around in the fetchers.