CONTRIBUTING: Add hotlinks to package and module reviewing guides, minor touchups

[pbsds]

Description of changes

I missed a detail myself, so let's improve discoverability.

Rendered:

• pkgs/README.md#reviewing-contributions
• nixos/README.md#reviewing-contributions

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[infinisil]

Unfortunately these only work locally in the file, changing the title will still break pages linking to it..

What should work, and I really think we should have that, is custom HTML anchors like

<a name="location"></a>

Can you try that and confirm whether it works?

[pbsds]

Is pandoc commonmark notation supported?

### File naming and organisation {#location}

[infinisil]

Unfortunately not

[pbsds]

Suggested change

	## How to backport pull requests<a name="how-to-backport-pull-requests"></a>
	## How to backport pull requests <a name="how-to-backport-pull-requests"></a>

When I add a space as suggested above then pandoc --to gfm adds a - at the end of the automatic id of the h2 tag. I assume pandoc behaves like github in this manner.

[pbsds]

If i add the anchor below the title, then it gets put inside the p block

[pbsds]

I did as suggested, but the results is not pretty to read in source. Perhaps a CI check that looks for dead links is the way to go?

EDIT: humble beginnings:

$ pandoc pkgs/README.md --from gfm --to html | htmlq a --attribute href | grep -E '\.md(#[a-z]+)?'
../CONTRIBUTING.md
./by-name/README.md
./by-name/README.md
./by-name/README.md
../CONTRIBUTING.md#commit-conventions
../CONTRIBUTING.md#file-naming
../CONTRIBUTING.md#commit-conventions
../CONTRIBUTING.md#how-to-backport-pull-requests
../CONTRIBUTING.md#how-to-backport-pull-requests

[infinisil]

It would also be great for third-party documents to be able to link to anchors, but I guess for now something like that would be better than nothing

[pbsds]

I've committed heinous crimes, but alas we do have a prototype:

{ pkgs ? import ./. {}
, lib ? pkgs.lib
}:

with pkgs;

let
  src = let
    toLink = path: {
      inherit path;
      name = lib.removePrefix "${toString ./.}/" (toString path);
    };
  in pkgs.linkFarm "nixpkgs-gfm-docs" (map toLink [
    ./CONTRIBUTING.md
    ./README.md
    ./doc/README.md
    ./lib/README.md
    ./lib/path/README.md
    ./maintainers/README.md
    ./nixos/README.md
    ./pkgs/README.md
    ./pkgs/by-name/README.md
    ./pkgs/test/nixpkgs-check-by-name/README.md
  ]);
in
  runCommand "test-nixpkgs-gfm-links" {
    nativeBuildInputs = [ htmlq pandoc ];
  } ''
    md2html() {
      local fname="$1" # relative to ${src}
      if test -f html-cache/"$fname"; then
        cat html-cache/"$fname"
      else
        mkdir -p "$(dirname html-cache/"$fname")"
        pandoc ${src}/"$fname" --from gfm --to html | tee html-cache/"$fname"
      fi
    }

    return_code=0

    shopt -s globstar
    for fname in ${src}/**/*.md; do
      fname="''${fname#${src}/}" # strip nix store prefix

      get_links() (
        set +o pipefail
        md2html "$fname" \
          | htmlq a --attribute href --base file:///"$fname" \
          | grep '^file:///' \
          | cut -d/ -f4- \
          | grep -E '\.md(#.*)?$' \
          | sort -u
      )

      while IFS=# read target anchor; do
        if ! test -f ${src}/"$target"; then
          >&2 echo "ERROR ($fname): resolved reference not found: $target"
          return_code=1
          continue
        fi

        test -n "$anchor" || continue

        if test -z "$(md2html "$target" | htmlq "#$anchor")"; then
          >&2 echo "ERROR ($fname): resolved reference not found: $target#$anchor"
          return_code=1
        fi
      done < <(get_links)

    done

    touch $out
    exit $return_code
  ''

Output:

test-nixpkgs-gfm-links> ERROR (CONTRIBUTING.md): resolved reference not found: CONTRIBUTING.md#backporting-changes
test-nixpkgs-gfm-links> ERROR (CONTRIBUTING.md): resolved reference not found: CONTRIBUTING.md#sec-package-naming
test-nixpkgs-gfm-links> ERROR (nixos/README.md): resolved reference not found: CONTRIBUTING.md#file-naming
test-nixpkgs-gfm-links> ERROR (pkgs/README.md): resolved reference not found: CONTRIBUTING.md#file-naming

The first two are genuine errors, and the last two are due to my code not handling name=... in addition to id=.... This could be fixed.

[pbsds]

I cleaned it up and pushed it as tests.nixpkgs-check-readme-links. I believe the way i phrase the src linkTree should avoids both any uneccesary rebuild, and globbing all of nixpkgs

@ofborg build tests.nixpkgs-check-readme-links

[infinisil]

Nice! Though just recently I was pointed to https://github.com/serokell/xrefcheck, which looks a bit nicer 😅

In any case, could you do that in a separate PR? This way we can merge this one with just the doc improvements/fixes

[infinisil]

Otherwise looking good!

[infinisil]

Nice thanks!