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

feat: MDBook linkcheck #192

Merged
merged 16 commits into from
Feb 5, 2025
Merged

feat: MDBook linkcheck #192

merged 16 commits into from
Feb 5, 2025

Conversation

youyyytrok
Copy link
Contributor

Hi! This pull request addresses the following changes:

  • Fix broken links: Updated outdated or broken URLs in ghash.rs and gcm.rs to use the correct and secure NIST links.
  • Formatting improvements: Resolved minor formatting inconsistencies in aes/README.md for better readability.

@youyyytrok
Copy link
Contributor Author

@Autoparallel Hi! I closed my previous PR #189 and opened a new one with the latest changes. Please review it.

@Autoparallel
Copy link
Contributor

Thanks @youyyytrok let me review!

Copy link
Contributor

@Autoparallel Autoparallel left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes!

In order to keep the contribution quality high, I want to ask a favor of you. Would you mind making a few more edits? Let me suggest some low hanging fruit:

If you visit: https://ronkathon.pluto.xyz you will find the landing page has a blurb on sumcheck that is a bit out of place. I believe this is also just the repo's README. Do you think you can reorganize this so that the landing page / README more adequately explains the project and keeps the specific material located in its relevant module/section/chapter?

I don't believe this would be too much more work. Hopefully you don't mind this ask!

@Autoparallel
Copy link
Contributor

I also can propose this: #198

@youyyytrok
Copy link
Contributor Author

Hi @Autoparallel !
I’ve added a link checker to the CI, but I used a different tool, namely lychee, instead of the MDBook you mentioned. If it’s important, I can switch to the one you described in the issue #198 .

Also, I couldn’t resolve the issue with the link https://www.cryptopp.com/wiki/XChaCha20, which is timing out.
image
I couldn’t find anything to replace it with. Could you help with this?

@Autoparallel
Copy link
Contributor

@youyyytrok that link seems broken to me too. Interesting...

Perhaps you can find an alternative link for ChaCha20? I know there's some good ones out there. Give me a bit, let me run the CI here and see what we get.

Copy link
Contributor

@Autoparallel Autoparallel left a comment

Choose a reason for hiding this comment

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

Here is this mdbook-linkcheck

Maybe we can add a workflow for this and set up the configuration for the book?

There is also just mdbook test that we can run in CI? Can you perhaps look into these two options as well?

I really appreciate the help here, by the way!

Comment on lines 3 to 6
on:
push:
branches:
- "**"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
on:
push:
branches:
- "**"
on:
pull_request:
types: [opened, synchronize, reopened]
push:
branches: [main]

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider this to match other workflows we have!

- name: Check links
uses: lycheeverse/lychee-action@v2
with:
args: --no-progress --verbose "**/README.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can have this walk through all the different markdown files we have? Then we can check links throughout the book too?

@youyyytrok
Copy link
Contributor Author

Hi! @Autoparallel Pls check🐳

@youyyytrok
Copy link
Contributor Author

I couldn't find a similar alternative to the original site. Perhaps using a link from the Web Archive would work for you.
If not, here are other options, though they don't provide information as concise and essential as the original site:

@Autoparallel
Copy link
Contributor

@youyyytrok nice! Just curious, do we need lychee for checking links if we have mdbook-linkcheck?

Supposing not, then we need to make sure mdbook-linkcheck is ran on PRs into main. So perhaps we have a book-check workflow or something for this?

Is there a reason you prefer lychee?

@Autoparallel
Copy link
Contributor

I couldn't find a similar alternative to the original site. Perhaps using a link from the Web Archive would work for you. If not, here are other options, though they don't provide information as concise and essential as the original site:

The web archive link is fine in my opinion!

I'm noticing two other links fail -- ones that go to our releases page on github. I deleted these since this crate is on crates.io. Perhaps we direct there instead?

@youyyytrok
Copy link
Contributor Author

@youyyytrok nice! Just curious, do we need lychee for checking links if we have mdbook-linkcheck?

Supposing not, then we need to make sure mdbook-linkcheck is ran on PRs into main. So perhaps we have a book-check workflow or something for this?

Is there a reason you prefer lychee?

I'm more used to working with it, and it’s more convenient for checking links. If you don’t need it, I can remove it from CI.

@youyyytrok
Copy link
Contributor Author

I couldn't find a similar alternative to the original site. Perhaps using a link from the Web Archive would work for you. If not, here are other options, though they don't provide information as concise and essential as the original site:

The web archive link is fine in my opinion!

I'm noticing two other links fail -- ones that go to our releases page on github. I deleted these since this crate is on crates.io. Perhaps we direct there instead?

Alright, then I'll replace the XChaCha link with the one from the web archive

Yes, I noticed that too but couldn’t find a way to fix it. When generating the changelog, the script doesn’t add a dot after "v," which makes the link invalid. But if we add the dot, the link works. For example:

@Autoparallel
Copy link
Contributor

We should just ignore having links to github based releases all together since we aren't doing them anymore. As for lychee, i don't mind keeping it in CI, but on the note of dependency minimization, if we can just use mdbook-linkcheck that seems simpler to me.

I'm happy to help here and get this across the finish line. I appreciate your work!

@youyyytrok
Copy link
Contributor Author

Hi @Autoparallel ! Pls check my work

@Autoparallel
Copy link
Contributor

@youyyytrok Nice work!

I have done the following:

  • added a new workflow mdbook-test.yaml which runs the mdbook test and mdbook linkcheck.
  • ran mdbook test locally after seeing this fail in CI and started fixing a few problems.

With that said, there are quite a few number of broken things in the mdbook! Now, this is not really your problem at all. I apologize for dragging this on as long as it has -- you've put in great effort and been kind even with all my shenanigans.

With that said, let me give you an option here: carry on and help fix the broken items in the mdbook, or we can merge this with a failing workflow and open an issue to fix all these problems.

Do you have a preference? In the latter case, we'd get this merged today and you are by no means being asked to continue fixing the book problems although I'd be happy to work with you to do that if you are interested!

@youyyytrok
Copy link
Contributor Author

@Autoparallel Thank you for the feedback! Unfortunately, I can't allocate additional time to address all the issues in the mdbook. I think it would be best to merge the current changes and document the remaining problems in an issue for future work, as you suggested.

Thank you for your understanding! 🐳

@Autoparallel
Copy link
Contributor

@youyyytrok no worries! I made some changes on top to address the problems.

Will merge soon.

@Autoparallel Autoparallel changed the title fix: 404's link and formatting updates feat: MDBook linkcheck Feb 5, 2025
@Autoparallel Autoparallel merged commit fece68a into pluto:main Feb 5, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants