The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29965 (Lint: support running individual rust linters and improve subtree exclusion by davidgumberg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Without #30025 (or alternative fix) the new action being added here will fail.
It takes ~11s to run:

• example pass
• example fail

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24698006837}

utACK 9d2a8a8

with or without the nits

Thanks @maflcko , did a final force-push for that silly nit: git range-diff 9d2a8a8...82c5f95

Sorry to invalidate the ACK

utACK 82c5f95

Thanks!

crACK 82c5f95

I tested by manually breaking relative and absolute offline links and verifying that the lint runner catches them.

Some notes:

• At present, mlc does not check for anchor link validity, but likely will in the future. (Support anchor link targets becheran/mlc#31)

  For example:

  -Verification of [scripted diffs](/doc/developer-notes.md#scripted-diffs).
  +Verification of [scripted diffs](/doc/developer-notes.md#nonsense-link).

  mlc will not complain if you apply this diff

• I have opened a PR (Move error summary output to stdout becheran/mlc#87) to remove the extraneous stderr output mlc produces when we suppress stdout:

  The following links could not be resolved:

• I have some reservations about the growth of software that is encouraged or required to set up a development environment. mlc seems well maintained, and I don't think it would be better to shift the maintenance burden of markdown link checking to this repo, but it gives me some pause that mlc's entire dependency tree becomes a common attack surface for bitcoin developers.

  That being said, this holds for all of the existing linter dependencies, and this dependency is optional and I am not an expert in security.

  I am not sure if this PR is an appropriate or productive venue for this discussion, but I just wanted to give mention to this.

• That being said, this holds for all of the existing linter dependencies, and this dependency is optional and I am not an expert in security.

Right, the pull here does not change that. I'd say, it is best to only run the linters locally in a sandbox, or at least skip the optional dependencies.

re-utACK 4b7d984

reACK 4b7d984

Looks great!

Tested with the following diff:

diff --git a/test/lint/README.md b/test/lint/README.md
index 49ed8356c3..47ac472aa6 100644
--- a/test/lint/README.md
+++ b/test/lint/README.md
@@ -36,7 +36,7 @@ Then you can use:
 | [`lint-shell.py`](/test/lint/lint-shell.py) | [ShellCheck](https://github.com/koalaman/shellcheck)
-| [`lint-spelling.py`](/test/lint/lint-spelling.py) | [codespell](https://github.com/codespell-project/codespell)
+| [`lint-spelling.py`](/test/lint/lint-spling.py) | [codespell](https://github.com/codespell-project/codespell)
 | markdown link check | [mlc](https://github.com/becheran/mlc)

$ cargo run
# [...]
One or more markdown links are broken.

Relative links are preferred (but not required) as jumping to file works natively within Emacs.

Markdown link errors found:
[Err ] ./test/lint/README.md (39, 3) => /test/lint/lint-spling.py - Target filename not found.

                
^---- ⚠️  Failure generated from markdown hyperlink check!

After conducting Guix builds, lint_markdown starts complaining about docs in guix-build-* subdirectories.

After conducting Guix builds, lint_markdown starts complaining about docs in guix-build-* subdirectories.

Oh I didn't consider this directory! I think it should be possible to add /guix-build-* to the ignored files.

After conducting Guix builds, lint_markdown starts complaining about docs in guix-build-* subdirectories.

Oh I didn't consider this directory! I think it should be possible to add /guix-build-* to the ignored files.

This was what I tried before writing my comment :)

It doesn't work, unfortunately.

Ah, that is unfortunate. I will try and find another option...

I think an upstream PR may be the best approach here, So I opened becheran/mlc#89

Not sure if globbing is the right fix. Conceptually, no folder or file should be scanned that is not tracked in git.

Though, I wonder if there is an easy way to ask git to print all files/folders that are "dirty", ignoring the gitignore. An alternative may be to teach mlc to read understand the gitignore files. However, that will also be incomplete, because devs may add random other folders themselves.

So for now the only solution would be to call mlc in a loop over git ls-files -- '*.md'. Maybe a better upsteam fix would be to teach mlc to iterate over all given files, instead of only over one?

Not sure if globbing is the right fix. Conceptually, no folder or file should be scanned that is not tracked in git.

Though, I wonder if there is an easy way to ask git to print all files/folders that are "dirty", ignoring the gitignore. An alternative may be to teach mlc to read understand the gitignore files. However, that will also be incomplete, because devs may add random other folders themselves.

So for now the only solution would be to call mlc in a loop over git ls-files -- '*.md'. Maybe a better upsteam fix would be to teach mlc to iterate over all given files, instead of only over one?

This was a. bit of a pain (and prob not worth it!) but seems to work: becheran/mlc@master...willcl-ark:mlc:git-aware

Passing the glob symbols to the subcommand in Rust was ... tricky, or a skill issue. So I went for a script which matches md (and html, which is the other thing this tool seems to do) against git check-ignore. Feels a bit hacky, but should result in the same as git ls-files -- **/* I think?

@maflcko do you think this hits the correct files? I think check-ignore will nicely include dirty files etc.

@maflcko do you think this hits the correct files? I think check-ignore will nicely include dirty files etc.

Ah sorry. I guess I was using the wrong words. With "dirty" I meant folders/files that should be ignored, but are not according to the gitignore. Maybe "untracked" is a better word, which is also used by git status. For example, a python venv or a generated untracked markdown file should not be included in the check.

git ls-files is also used in ./test/lint/lint-python.py, for reference. So using git ls-files -- '*.md' and passing the full array to mlc may be the best approach for us?

git ls-files is also used in ./test/lint/lint-python.py, for reference. So using git ls-files -- '*.md' and passing the full array to mlc may be the best approach for us?

Thanks, I changed the approach in https://github.com/willcl-ark/mlc/tree/git-aware to use git status --ignored *.md and append to the ignored paths, as adding to that felt like the path of least resistance.

I will investigate if I can have a --git flag only check the result of git ls-files -- '*.md', as I feel this makes even more sense than having a --gitignore flag exclude (all) git-ignored files... Most folks using the tool are going to want to check:

i) a single file or
ii) all files tracked by git

...I think