Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sphinx-builder linkcheck #2489

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Add sphinx-builder linkcheck #2489

merged 1 commit into from
Sep 19, 2024

Conversation

LecrisUT
Copy link
Contributor

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

@LecrisUT LecrisUT changed the title ci: sphinx: Add linkcheck Add sphinx-builder linkcheck Apr 15, 2024
@LecrisUT LecrisUT mentioned this pull request May 7, 2024
5 tasks
@happz happz added this to the 1.34 milestone May 8, 2024
@happz happz added documentation Improvements or additions to documentation test coverage Improvements or additions to test coverage of tmt itself labels May 8, 2024
happz added a commit that referenced this pull request May 17, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in template when rendering them; these should be
resolved by the work related to
#2868.
happz added a commit that referenced this pull request May 21, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in template when rendering them; these should be
resolved by the work related to
#2868.
happz added a commit that referenced this pull request May 28, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in template when rendering them; these should be
resolved by the work related to
#2868.
happz added a commit that referenced this pull request May 28, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in template when rendering them; these should be
resolved by the work related to
#2868.
happz added a commit that referenced this pull request May 28, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in a template when rendering them; these should be
resolved by the work related to
#2868.

Co-authored-by: Miroslav Vadkerti <[email protected]>
happz added a commit that referenced this pull request Jun 2, 2024
Related to #2489. This PR does not
fix all invalid links, but the rest should be caused by incorrect
handling of links in a template when rendering them; these should be
resolved by the work related to
#2868.

Co-authored-by: Miroslav Vadkerti <[email protected]>
@martinhoyer
Copy link
Collaborator

@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
Copy link
Contributor Author

LecrisUT commented Jun 4, 2024

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 🤔

@happz
Copy link
Collaborator

happz commented Jun 4, 2024

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

Yes, my #2940 did not try to address everything. There's #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 #2868 (comment). 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
Copy link
Collaborator

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
Copy link
Collaborator

happz commented Jun 10, 2024

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

@LecrisUT
Copy link
Contributor Author

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

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Aug 1, 2024

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

@martinhoyer
Copy link
Collaborator

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
Copy link
Contributor Author

LecrisUT commented Aug 1, 2024

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:

tmt/tmt/utils/__init__.py

Lines 4318 to 4325 in 5fbbd89

ref = run(Command("git", "rev-parse", "--abbrev-ref", "HEAD"))
if ref != fmf_id.default_branch or always_get_ref:
fmf_id.ref = ref
else:
# Note that it is a valid configuration without having a default
# branch here. Consumers of returned fmf_id object should check
# the fmf_id contains everything they need.
fmf_id.ref = None

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.

@thrix thrix self-requested a review August 1, 2024 13:04
Copy link
Collaborator

@thrix thrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@happz
Copy link
Collaborator

happz commented Aug 5, 2024

/packit build

@happz happz added the ci | full test Pull request is ready for the full test execution label Sep 3, 2024
@happz
Copy link
Collaborator

happz commented Sep 3, 2024

/packit build

@happz
Copy link
Collaborator

happz commented Sep 3, 2024

Hmm, blocked by #3108 :/

@happz happz added the status | blocked The merging of PR is blocked on some other issue label Sep 3, 2024
@LecrisUT
Copy link
Contributor Author

LecrisUT commented Sep 3, 2024

Hmm, blocked by #3108 :/

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

@martinhoyer martinhoyer modified the milestones: 1.36, 1.37 Sep 3, 2024
@psss
Copy link
Collaborator

psss commented Sep 17, 2024

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 psss added status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. and removed status | blocked The merging of PR is blocked on some other issue labels Sep 19, 2024
Signed-off-by: Cristian Le <[email protected]>
@psss
Copy link
Collaborator

psss commented Sep 19, 2024

/packit build

@psss
Copy link
Collaborator

psss commented Sep 19, 2024

The extended unit tests failure will be fixed by #3227. Remaining failure is irrelevant.

@psss psss merged commit 8245c47 into teemtee:main Sep 19, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution documentation Improvements or additions to documentation status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. test coverage Improvements or additions to test coverage of tmt itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants