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

ASYNC_start_job: more readable documentation for handling ASYNC* APIs #23941

Closed
wants to merge 1 commit into from

Conversation

tomato42
Copy link
Contributor

Recently I was trying to figure out how to use the OpenSSL APIs combined with the ASYNC multithreading and found the documentation hard to follow.

Added few more comments that would probably made it easier to the past me.

Checklist
  • documentation is added or updated

doc/man3/ASYNC_start_job.pod Outdated Show resolved Hide resolved
doc/man3/ASYNC_start_job.pod Show resolved Hide resolved
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: documentation The issue/pr deals with documentation (errors) tests: exempted The PR is exempt from requirements for testing labels Mar 22, 2024
@tomato42
Copy link
Contributor Author

About the failing CI check:

doc/man3/ASYNC_start_job.pod:1: Missing man section number (likely, 3) in L<select(2)>
doc/man3/ASYNC_start_job.pod:1: Missing man section number (likely, 3) in L<poll(2)>
doc/man3/ASYNC_start_job.pod:1: reference to non-existing select(2)
doc/man3/ASYNC_start_job.pod:1: reference to non-existing poll(2)

we can't link to basic C man pages?

@nhorman
Copy link
Contributor

nhorman commented Mar 22, 2024

According to this:
https://perldoc.perl.org/perlpod#Formatting-Codes

We should be able to. I wonder if successful linking is incumbent on the target man pages being installed? I would imagine that the ci images that do the doc-nits check have them all stripped

@nhorman
Copy link
Contributor

nhorman commented Mar 22, 2024

Oh, I found the problem, its twofold:

  1. find-doc-nits only checks for man pages in sections 1,3,5 and 7. We need to add section 2 to the check
  2. find-doc-nits assumes all links resolve to files in the openssl repo (excluding hyperlinks and such). since unix man pages don't we would need to find some way to make them appear in the tree, have find-doc-nits understand them as nroff pages (rather than pod files), and not processes them

@tomato42
Copy link
Contributor Author

Sounds like I shouldn't link to them then...

@nhorman
Copy link
Contributor

nhorman commented Mar 23, 2024

yeah, probably not, as the L<> tag assumes that the link target is included in the docs tree. We could potentially use this syntax:

L<select|http://external.link.to/man/page/you/want/to/target

There are several sites that provide canonical man pages in html form, but I'm not sure what the policy is on linking to external sources

Signed-off-by: Hubert Kario <hkario@redhat.com>
@tomato42
Copy link
Contributor Author

no-shared-macos (macos-14) failure looks unrelated: seems to be a race condition in the test case

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 26, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 27, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@nhorman
Copy link
Contributor

nhorman commented Apr 2, 2024

merged, thank you for your contribution

@nhorman nhorman closed this Apr 2, 2024
openssl-machine pushed a commit that referenced this pull request Apr 2, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #23941)
@tomato42 tomato42 deleted the async-docs branch April 2, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants