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

Fix README.md to work with Doxygen release 1.10.0 #1775

Merged
merged 4 commits into from
May 3, 2024
Merged

Conversation

praveksharma
Copy link
Member

Undoes changes from #1769 and re-introduces changes from #1715 to satisfy Doxygen release 1.10.0.

GitHub markdown provides a few extension in addition to the standard Markdown spec; a valid GitHub Markdown file isn't necessarily a valid Markdown file (see this comment by a Doxygen contributer). Changing #linuxmacos to either #linuxmacOS or #linux/macOS worked for me locally; I chose #linuxmacOS because that seems to keep the link intact at the moment.

CI containers currently use Doxygen 1.9.2, openquantumsafe/ci-containers#76 updates them to use Doxygen 1.10.0.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@Martyrshot
Copy link
Member

Looks good to me. :) Thanks for this

Copy link
Member

@SWilson4 SWilson4 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 taking care of this, @praveksharma!

@praveksharma praveksharma marked this pull request as ready for review April 30, 2024 13:53
@praveksharma
Copy link
Member Author

@Martyrshot I forgot to mark the draft PR to ready for review, sorry about that. Merging is blocked till the changes get another approval.

@dstebila
Copy link
Member

If we were to rename the section title from Linux/macOS to Linux and macOS would that result in a shortcode that is compatible with both Doxygen and Github?

@praveksharma
Copy link
Member Author

If we were to rename the section title from Linux/macOS to Linux and macOS would that result in a shortcode that is compatible with both Doxygen and Github?

I considered that. Since both #linuxmacOS and #linux/macOS (I didn't state this explicitly but #linux/macos does not work) I think the issue is the upper case letters in macOS and not the /.

@dstebila
Copy link
Member

dstebila commented Apr 30, 2024

If we were to rename the section title from Linux/macOS to Linux and macOS would that result in a shortcode that is compatible with both Doxygen and Github?

I considered that. Since both #linuxmacOS and #linux/macOS (I didn't state this explicitly but #linux/macos does not work) I think the issue is the upper case letters in macOS and not the /.

Okay... would Linux/Mac or Linux and Mac work?

@praveksharma
Copy link
Member Author

Okay... would Linux/Mac or Linux and Mac work?

Either would work, I ended up going with Linux and Mac since that seems to better follow GitHub Markdown conventions (I could be wrong but I don't think the GitHub Markdown spec or the GitHub markdown formatting documentation explicitly address how section links handle non-alphabetic characters and uppercase alphabets).

@praveksharma
Copy link
Member Author

I pushed some more changes: the CI needed to be tweaked a little once the CI containers started using the updated images from openquantumsafe/ci-containers#76. CI now uses scripts/run_doxygen.sh (which does some formatting to make GitHub markdown work with Doxygen) instead of calling Doxygen directly. I'm not why the older way earlier but doesn't anymore; for what it's worth, the new way of using scripts/run_doxygen.sh follows how ninja gen_docs generates documentation.

@praveksharma praveksharma requested a review from SWilson4 May 1, 2024 12:57
Copy link
Member

@Martyrshot Martyrshot left a comment

Choose a reason for hiding this comment

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

Seems good to me, but I would wait for @SWilson4 @dstebila, and @baentsch to take a look. :)

# The default value is: DOXYGEN.
# This tag requires that the tag MARKDOWN_SUPPORT is set to YES.

MARKDOWN_ID_STYLE = DOXYGEN
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems to suggest a new feature controlling how anchors for headers are generated for Markdown files. Would this have solved our problem?

Copy link
Member Author

@praveksharma praveksharma May 3, 2024

Choose a reason for hiding this comment

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

I think it might. I'll merge this so PRs with red CI can rebase to main and create a new PR to address configuring Doxygen more appropriately if that is applicable.

@praveksharma praveksharma merged commit a23046f into main May 3, 2024
81 checks passed
SWilson4 pushed a commit that referenced this pull request Jun 5, 2024
* fix link in README.md

Signed-off-by: Pravek Sharma <[email protected]>

* simplify linux and mac link in README.md

Signed-off-by: Pravek Sharma <[email protected]>

* update Doxyfile

Signed-off-by: Pravek Sharma <[email protected]>

* update CI to use /scripts/run_doxygen.sh

Signed-off-by: Pravek Sharma <[email protected]>

---------

Signed-off-by: Pravek Sharma <[email protected]>
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.

4 participants