scripts: link check compatibility and docs

[jorgeorpinel]

Compatibility: it doesn't run out-of-the-box because pcregrep is not usually on the OS.

• Not sure about MacOS (but I remember sed was a problem there)
• On Ubuntu I had to sudo apt install pcregrep
• For Windows, I got it from https://sourceforge.net/projects/gnuwin32/files/pcre/, installed and put in PATH.
  sed will likely be a problem in some Windows terminals too, but it can also be obtained from GNUWin (http://sourceforge.net/projects/gnuwin32/files/sed).

By "and docs" I mean we should probably at least write the above info and how to use these commands in the README.

[jorgeorpinel]

Cc @casperdcl

[shcheklein]

It sounds too complicated if we run it locally on pre-commit. We should not require installing stuff with apt (or even worse on Windows) for the link checker.

[jorgeorpinel]

Sure, I guess I'm just suggesting we add this info to the README, at least 🙂
...so maybe this is a duplicate or can be merged into #1125

[shcheklein]

no, I think it's a bug to require this in the first place. We should not require more than yarn on most reasonable platforms.

[jorgeorpinel]

Got it. Also note it seems to have another bug as noted in #1000 (comment):

It's outputting

find: ‘/.../dvc.org/pages/’: No such file or directory

every time now. I think it has to be updated given the new project structure.

[shcheklein]

find: ‘/.../dvc.org/pages/’: No such file or directory

can we do a quick fix for this?

[jorgeorpinel]

You're right, that's pretty simple ha. OK fixed in 9665edc (#1124)

[casperdcl]

Compatibility: it doesn't run out-of-the-box because pcregrep is not usually on the OS [...] Not sure about MacOS (but I remember sed was a problem there) [...] likely be a problem in some Windows terminals too

and

[...] sounds too complicated if we run it locally on pre-commit. We should not require installing stuff with apt (or even worse on Windows) for the link checker [...] it's a bug to require this in the first place. We should not require more than yarn on most reasonable platforms.

link-check was never intended to be run locally. We had discussions about this originally and there were several reasons:

• pcregrep really is required. Most regex implementations (including python's own as well as any I could find via yarn) lack the features required to make nested bracket detection (as with markdown links) work flawlessly
• it can take a few minutes to check all links (which is why it's best done in the cloud via a cron job)
• ergo the scripts were written in bash, which itself could be an issue on e.g. Windows

Compared to the hassle of finding work-arounds, I don't think it's too much of an issue to wait for small diffs to be checked by CI rather than installing & running locally.

By "and docs" I mean we should probably at least write the above info and how to use these commands in the README.

... maybe. I think just documenting in code is enough purely for the purpose of maintenance of CI scripts.

The relevant lines are:

dvc.org/.circleci/config.yml

Line 35 in 7d259fb

sudo apt-get install pcregrep

dvc.org/package.json

Lines 18 to 19 in 7d259fb

	"link-check": "./scripts/link-check-git-all.sh",
	"link-check-diff": "./scripts/link-check-git-diff.sh"

using:

• https://github.com/iterative/dvc.org/blob/master/scripts/link-check-git-all.sh
• https://github.com/iterative/dvc.org/blob/master/scripts/link-check-git-diff.sh

both of which point to:

dvc.org/scripts/link-check.sh

Lines 1 to 6 in 7d259fb

	#!/usr/bin/env bash
	# Check HTTP status codes of links in the given files.
	# Success: 2xx, Errors: 4xx/5xx, Warnings: anything else.
	# Redirects (3xx) are followed.
	# Usage:
	# link-check.sh [<files>]

[shcheklein]

@jorgeorpinel let's remove it from docs for now. Since there is not intention to support it locally for now, let's not bother documenting a lot of stuff.

@casperdcl may be we can write a comment in the script itself about its dependencies and this script is purely for CI to run?

[jorgeorpinel]

let's remove it from docs for now

Removed in 4bf9eea (for #1160)

[casperdcl]

Also worth looking at the original PR #958.

E.g.: #958 (comment) states:

[prcregrep is] one of the reasons why this is a CI-only check by default rather than a pre-commit hook... no guarantee that it'll run on dev machines

[shcheklein]

Closing this. I think it's resolved more or less. Decided to keep it CI focused mostly.