util/find-doc-nits: more precise option and function name checker #10073

levitte · 2019-10-02T11:24:00Z

The checks for our uses of B< and I< for options, and possibly
function names, was over-reaching quite a bit.

So we fine-tune it a bit:

by only checking for options in man1 pages, and only in SYNOPSIS
and *OPTIONS sections.
by only checking for function names in man3 pages.

The man1 option checker has the additional check that options found in
*OPTIONS are also found in SYNOPSIS and vice versa.

In all cases, this also handles options and function names with
additional markup, such as B<-I<cipher>> and B<sk_I<TYPE>_push>.

The checks for our uses of 'B<' and 'I<' for options, and possibly function names, was over-reaching quite a bit. So we fine-tune it a bit: - by only checking for options in man1 pages, and only in SYNOPSIS and *OPTIONS sections. - by only checking for function names in man3 pages. The man1 option checker has the additional check that options found in *OPTIONS are also found in SYNOPSIS andd vice versa. In all cases, this also handles options and function names with additional markup, such as 'B<-I<cipher>>' and 'B<sk_I<TYPE>_push>'.

levitte · 2019-10-02T11:25:05Z

Note that Travis will fail, because there is plenty of incomplete option documentation. That's actually a good thing, it shows that this script does its job right.

richsalz

hey, i even still understood my script after these changes :)

util/find-doc-nits

richsalz · 2019-10-02T12:32:16Z

And it would be good to see this merged soon, since all builds are currently broken.

richsalz · 2019-10-02T12:34:15Z

Note that Travis will fail, because there is plenty of incomplete option documentation. That's actually a good thing, it shows that this script does its job right.

do we want to do that? there are a LOT of undocumented flags. breaking all travis jobs until they're fixed doesn't seem quite right.

And BTW, look at the "-c" flag to find-doc-nits. Either remove that, or disable the reporting you do, we should only do it once.

mattcaswell · 2019-10-02T12:45:45Z

do we want to do that? there are a LOT of undocumented flags. breaking all travis jobs until they're fixed doesn't seem quite right.

I agree - we should get travis fixed asap.

levitte · 2019-10-02T12:51:35Z

[sigh] I have a separate PR where I'm fixing up doc/man1. I'm sure you've noticed.

richsalz · 2019-10-02T12:52:57Z

please don't lose the last line of my #10073 (comment)

levitte · 2019-10-02T13:08:06Z

Note that Travis will fail, because there is plenty of incomplete option documentation. That's actually a good thing, it shows that this script does its job right.

do we want to do that? there are a LOT of undocumented flags. breaking all travis jobs until they're fixed doesn't seem quite right.

My check does something different, it detects inconsistencies between SYNOPSIS and OPTIONS, i.e. if an option is given in one of them but not the other. So this has absolutely nothing to do with options not being documented at all.

richsalz · 2019-10-02T13:16:56Z

My check does something different

Got it, thanks. I still think that it is very wrong to check in code that knowingly breaks CI builds. OpenSSL has enough instances of doing it by mistake.

mattcaswell · 2019-10-02T13:25:06Z

I still think that it is very wrong to check in code that knowingly breaks CI builds.

I agree - we should avoid doing that.

Does #10065 address the new issues that this PR finds?

richsalz · 2019-10-02T13:30:45Z

No. They were broken by #10041, which broke find-doc-nits handling some new syntax. This PR fixes that issue, but then continues to break the build by adding a new type of error, the synopsis/options inconsistency mentioned above.

levitte · 2019-10-02T13:33:56Z

I have no idea what @richsalz is saying "No" to here. #10065 is indeed intended to fix what is found by this version of the script. I do not want to have discussions about the script and discussions about the manual pages mixed in the same PR, hence this separate one for the script.

richsalz · 2019-10-02T13:37:05Z

Does merging this PR leave doc-nits builds still broken? Yes, because of the new check you added which you are planning on fixing in another PR.

mattcaswell · 2019-10-02T13:38:15Z

So, IIUC this means that this PR is dependent on #10065. Only when both this PR and #10065 are merged will we have a clean travis again.

richsalz · 2019-10-02T13:40:01Z

What I have done in other PR's, and what I think should be done here, is that the errors at lines 318 and 323 should be commented out and enabled in #10065.

Fix the bugs/nits, and enhance find-doc-nits to report issues so it doesn't happen again, in the same PR.

mattcaswell · 2019-10-02T13:41:38Z

What I have done in other PR's, and what I think should be done here, is that the errors at lines 318 and 323 should be commented out and enabled in #10065.

That seems like a sensible approach.

levitte · 2019-10-02T13:48:10Z

I find it incredibly interesting that y'all don't want a script that is designed to show where our documentation is wrong, to actually show where the documentation is wrong (or at the very least, inconsistent).

A major reason to have it in a separate PR is for demonstration purposes, i.e. demonstrate that it actually does the work it purports to do. If I would put it in #10065, you would have no way to see that without doing extra work.

But tell you what, when the changes in this script look acceptable, I can always rebase #10065 on top of this, and we can all see the effect it will have there. Deal?

richsalz · 2019-10-02T13:55:50Z

What I described is what I have always done for doc nits before. We have never needed "proof" that it works" You can get that by making one commit enable the doc-nits changes, and then a separate commit include the fixes. You can put "if $debug" tests around the errors here, and then enable them for production in the other PR.

I am only a contributor, but I find it horrible to knowingly check in code that breaks the build.

mattcaswell · 2019-10-02T13:56:50Z

I find it incredibly interesting that y'all don't want a script that is designed to show where our documentation is wrong, to actually show where the documentation is wrong (or at the very least, inconsistent).

We want a fixed travis build more. Only then do we want to see more errors.

levitte · 2019-10-02T14:10:46Z

That way, we only get to see that it can approve of what's there, but not that it can find faults that are there, at least in the PR that changes it.

But like I said, when you have no more issues with the changes here, I will rebase #10065 on top if it, and final approval can happen there.

mattcaswell · 2019-10-02T14:15:31Z

But like I said, when you have no more issues with the changes here, I will rebase #10065 on top if it, and final approval can happen there.

I would much rather get this in asap, and not have it wait on #10065. This is breaking travis now and we need to get it fixed.

levitte · 2019-10-02T15:14:55Z

There were still some malformed options, even thought I removed the SYNOPSIS<->OPTIONS inconsistency check (I'll add it back in another PR), so there are a few affected man-pages.

mattcaswell · 2019-10-02T15:19:38Z

Why the inclusion of version.sh?

doc/man1/openssl-verify.pod

util/find-doc-nits

levitte · 2019-10-02T15:28:43Z

Why the inclusion of version.sh?

Mistake. I have no idea how it ended up in here. Fixed.

levitte · 2019-10-02T15:37:53Z

I think I've answered all outstanding issues

levitte · 2019-10-02T16:31:07Z

CIs are happy. Are you?

richsalz · 2019-10-02T16:33:50Z

I am happy. :) Thanks for listening.

levitte · 2019-10-02T16:36:59Z

I'm glad you are ;-). I hope someone with approval power is as well ...

mattcaswell

LGTM

levitte · 2019-10-02T17:26:48Z

Merged.

705128b util/find-doc-nits: more precise option and function name checker
8dc57d7 doc/man1: fix malformed options

The checks for our uses of 'B<' and 'I<' for options, and possibly function names, was over-reaching quite a bit. So we fine-tune it a bit: - by only checking for options in man1 pages, and only in SYNOPSIS and *OPTIONS sections. - by only checking for function names in man3 pages. The man1 option checker has the additional check that options found in *OPTIONS are also found in SYNOPSIS andd vice versa. In all cases, this also handles options and function names with additional markup, such as 'B<-I<cipher>>' and 'B<sk_I<TYPE>_push>'. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10073)

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10073)

levitte added the branch: master Applies to master branch label Oct 2, 2019

richsalz suggested changes Oct 2, 2019

View reviewed changes

agnosticdev mentioned this pull request Oct 2, 2019

Update dgst.c to show a list of message digests #9912

Closed

fixup! util/find-doc-nits: more precise option and function name checker

31b8f7e

fixup! util/find-doc-nits: more precise option and function name checker

a2df811

fixup! util/find-doc-nits: more precise option and function name checker

eddf7d7

levitte added 2 commits October 2, 2019 17:12

fixup! util/find-doc-nits: more precise option and function name checker

a1a270d

doc/man1: fix malformed options

cccdb04

richsalz reviewed Oct 2, 2019

View reviewed changes

doc/man1/openssl-verify.pod Outdated Show resolved Hide resolved

richsalz reviewed Oct 2, 2019

View reviewed changes

util/find-doc-nits Outdated Show resolved Hide resolved

fixup! util/find-doc-nits: more precise option and function name checker

3f12f11

levitte added 2 commits October 2, 2019 17:35

fixup! util/find-doc-nits: more precise option and function name checker

53d1615

fixup! doc/man1: fix malformed options

e8e2698

levitte mentioned this pull request Oct 2, 2019

WIP: util/find-doc-nits: enhance to find option inconsistencies between sections #10076

Closed

mattcaswell approved these changes Oct 2, 2019

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Oct 2, 2019

levitte closed this Oct 2, 2019

levitte added a commit that referenced this pull request Oct 2, 2019

doc/man1: fix malformed options

8dc57d7

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10073)

richsalz mentioned this pull request Oct 3, 2019

Add UK/US warnings and fix them #10079

Closed

Uh oh!

util/find-doc-nits: more precise option and function name checker #10073

util/find-doc-nits: more precise option and function name checker #10073

Uh oh!

Conversation

levitte commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

mattcaswell commented Oct 2, 2019

Uh oh!

Uh oh!

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

richsalz commented Oct 2, 2019

Uh oh!

levitte commented Oct 2, 2019

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Oct 2, 2019 •

edited

Loading