Add sphinx-builder `linkcheck`

[LecrisUT]

Depends on: #2483

There are a bunch of linkcheck failures, many of which come from the auto-generated pages. Not sure how these should be handled

[martinhoyer]

@happz looks like we have a few more broken links being generated.
No idea about this one ( spec/core: line 131) broken None -
Test docs, like /tests/execute/results-custom.fmf -> tests/execute/result/custom

Looking into how it's being generated.

[LecrisUT]

Oh wow, this is what it expanded for me:

* Verified by `/tests/core/adjust <https://github.com/LecrisUT/tmt/tree/feat/linkcheck/tests/core/adjust/main.fmf>`_ 

It seems the generation automatically puts in the git repo and branch, but then on a PR, the git repo can be either target or source depending on the checkout. I think that is where it trips up :thinking:

[happz]

@happz looks like we have a few more broken links being generated.

Yes, my https://github.com/teemtee/tmt/pull/2940 did not try to address everything. There's https://github.com/teemtee/tmt/issues/2868 for the links to tmt internals and tests as created by the template, which seems to be the vast majority of the remaining broken links.stories

No idea about this one ( spec/core: line 131) broken None - Test docs, like /tests/execute/results-custom.fmf -> tests/execute/result/custom

Looking into how it's being generated.

Check out https://github.com/teemtee/tmt/issues/2868#issuecomment-2109611146. I'd be happy to look into fixing this, since it's the code I wrote and left in less than ideal state, but if you feel like diving in, be my guest :)

Today I'd go with the idea mentioned in the last paragraph, i.e. moving the macro away from the template, making it a full-fledged Python code & expose it back to the template, mostly because it can be tested easily. I'd add a couple of unit tests to verify it's rendering links correctly - should be mostly Link on the input, str on the output, therefore easy to parameterize with a pile of test cases. Feel free to ping me if you hit some dumb piece of code.

Oh wow, this is what it expanded for me:

* Verified by `/tests/core/adjust <https://github.com/LecrisUT/tmt/tree/feat/linkcheck/tests/core/adjust/main.fmf>`_ 

It seems the generation automatically puts in the git repo and branch, but then on a PR, the git repo can be either target or source depending on the checkout. I think that is where it trips up 🤔

That's one issue for sure, there are more though, e.g. some links lead to main.fmf when the test is not defined in main.fmf and the file has a different name, and so on. Basically the macro tries to guess, and in some cases it's wrong.

[martinhoyer]

Thanks @happz!

if you feel like diving in, be my guest :)

I'll be afk for the rest of this week unfortunately. Also, I've familiarized myself with the templates a bit and think I get the general idea, but there is still plenty of things I'm seeing for the first time to catch-up with.

[happz]

With https://github.com/teemtee/tmt/pull/3001 applied, I get no broken links reported from the linkcheck builder.

[LecrisUT]

With #3001 applied, I get no broken links reported from the linkcheck builder.

Let me rebase on your commit first just to make sure it works for forks as well

[happz]

With #3001 applied, I get no broken links reported from the linkcheck builder.

Let me rebase on your commit first just to make sure it works for forks as well

It's probably not going to, not out of the box, there's still an issue with forks and/or SSH config. I had to apply this to unblock my local environment:

diff --git a/tmt/utils.py b/tmt/utils.py
index 596c66b9..01493215 100644
--- a/tmt/utils.py
+++ b/tmt/utils.py
@@ -4291,6 +4291,8 @@ def web_git_url(url: str, ref: str, path: Optional[Path] = None) -> str:
     if path:
         url += str(path)

+    url = url.replace('github:teemtee', 'https://github.com/teemtee').replace('feat/linkcheck', 'main')
+
     return url

[happz]

Right, and the branch is also wrong, it's all main and HEAD, but that's another problem to resolve. It works now, it won't work when PR starts pointing at new things.

[LecrisUT]

Hmm, but looking at the original generated links it actually looked fine:

* Verified by `/tests/core/adjust <https://github.com/LecrisUT/tmt/tree/feat/linkcheck/tests/core/adjust/main.fmf>`_ 

It points to a correct file, so should it actually have that replace?

[happz]

Hmm, but looking at the original generated links it actually looked fine:

* Verified by `/tests/core/adjust <https://github.com/LecrisUT/tmt/tree/feat/linkcheck/tests/core/adjust/main.fmf>`_ 

It points to a correct file, so should it actually have that replace?

Well, even better then :) Things did not work for me locally, hence the replace(), if you're fine with how they work in Github check, fewer things to worry about for me :)

it might have been the SSH config rather than the fork that was ruining my local work.

[happz]

Anyway, I don't know where the single broken link comes from - I can't reproduce it:

2024-06-10T08:13:19.5300673Z (       spec/core: line  131) broken    None - 

In my generated sources, it's this line:

* Verified by `/tests/core/adjust <github:teemtee/tmt/tree/docs-improve-rendering-of-story-links/tests/core/adjust/main.fmf>`__ 

[martinhoyer]

Anyway, I don't know where the single broken link comes from - I can't reproduce it:

It's driving me crazy.
btw, shouldn't we see test links ending with .fmf in the output as ok?

[LecrisUT]

Sorry, should have done this long ago, let's what exactly is being produced in github action: https://github.com/teemtee/tmt/actions/runs/9450580783/artifacts/1585890004

* Verified by `/tests/core/adjust <None>`__ 

It is literally None :sweat_smile:. Seems to be {{ url }} expansion

[martinhoyer]

Sorry, should have done this long ago, let's what exactly is being produced in github action: https://github.com/teemtee/tmt/actions/runs/9450580783/artifacts/1585890004 It is literally None 😅. Seems to be {{ url }} expansion

Thanks! Indeed. I'm confused why the doc build is successful. Perhaps we should double-check the build command arguments?

RTD: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html

[LecrisUT]

Oh, yeah, I didn't even realize the normal build actually worked perfectly fine. One difference is that hatch is used for the Github actions. Oh, and python version. Another hypothesis, it's actions/checkout

[LecrisUT]

Ok, I probably have a clue:

..
    [Debug] emit_tmt_object_links(link).object = /tests/core/adjust [Test]
    fmf_id.url = https://github.com/teemtee/tmt
    fmf_id.ref = None

It's probably that actions/checkout on PRs is pointing to the merge commit ^1 and it seems fmf_id.ref is not able to account for that. RTD uses source commit instead.

[happz]

Interesting. It seems to me that ref is also HEAD in RTD PR job, no idea why. At least HEAD is propagated into links in docs generated for a PR.

[LecrisUT]

Maybe, it's more simple than that, just the command failing at:

    branch = run(Command("git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"))

Maybe there are better commands than rev-parse to get what is basically:

$ git checkout --progress --force refs/remotes/pull/2489/merge

The difference with RTD is that it uses:

git fetch origin --force --prune --prune-tags --depth 50 pull/2489/head:external-2489

(Note the : making it a named branch, vs the GH action)

---

Here are 2 alternatives that could be used:

$ git show-ref --head --no-head HEAD
1198e878e700cc184bdcc3757c4641ca409a5c38 refs/remotes/pull/2489/merge
$ git for-each-ref --format="%(refname)" --points-at HEAD
refs/remotes/pull/2489/merge

Edit: actually just the later is more robust. Here's how it looks in more general if there are tags and other branches pointing to it:

$ git for-each-ref --format="%(refname)" --points-at HEAD
refs/heads/feat/linkcheck
refs/heads/some-random-other-branch
refs/remotes/LecrisUT/feat/linkcheck
refs/tags/i-am-a-tag

A bit annoying that we cannot distinguish if it's one or another :/

[LecrisUT]

@happz checkout 94fc226ca5d757ff63052119477fc57712068152 and b9d84acab0ced825e8e4b62ce131b4add431f6f0 if you've got time. This almost works, just need to sanitize web_git_url a bit more to be able to read and navigate ref=refs/remotes/pull/N/{merge,head} (and equivalent for gitlab) and translate it to PR-like url. However, I don't seem to find a tree url to navigate a PR :/. Technically a2eadc43d8345a6af4e31b0c8f9dfc01ef06b96e can also work

Lol, this actually works :))

* Verified by `/tests/core/adjust <[None](https://github.com/teemtee/tmt/tree/1.33.0-43-ga2eadc4/tests/core/adjust/main.fmf)>`__ 

[thrix]

@LecrisUT we talked about this PR today, could you maybe rebase it and write down the current state, or make it a draft if still in works pls?

[martinhoyer]

Any ideas why it failed @LecrisUT @happz ? It seems to be this line:

(       spec/core: line  131) broken    None - 

, which if I understand it correctly should be https://github.com/teemtee/tmt/tree/HEAD/tests/core/adjust/main.fmf link, which works fine on rtd latest.

EDIT: hmm, tmt --context repo=https://custom.url/fedora-38-x86_64 run ... being a few lines above it is quite sus.

[LecrisUT]

The failure is because it is run on a fork, these are resolved in #3108

[martinhoyer]

The failure is because it is run on a fork, these are resolved in #3108

If it's because of fork, wouldn't it break more links than just one?

[LecrisUT]

I don't quite remember, but there were a bunch of broken links inside the rst generated file, but it so happened that linkcheck aggregated the report or something, don't quite remember.

On RTD it showed fine because it uses a different git checkout approach and it doesn't fail here: https://github.com/teemtee/tmt/blob/5fbbd89f8be22fdbb7efd7c7044ea0dd4d077ca7/tmt/utils/init.py#L4318-L4325

But git checkout in GH actions checksout the merge commitand that's where it breaks. Check the log of the checkout action to replicate locally.

[happz]

/packit build

[happz]

/packit build

[happz]

Hmm, blocked by https://github.com/teemtee/tmt/pull/3108 :/

[LecrisUT]

Hmm, blocked by #3108 :/

Or maybe superseded-by since that one contains the commit from here

[psss]

Or maybe superseded-by since that one contains the commit from here

Let's keep the linkcheck change here and remove it from #3108.

[psss]

/packit build