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

[CI:DOCS] Refactor common options in man pages #15174

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

edsantiago
Copy link
Member

podman-create and -run have many options in common. To date,
these are copy-pasted and haphazardly maintained.

Solution: add an include mechanism, '@@option foo', such
that multiple md source files can fetch from one common file.

This is a Phase One commit, a very small subset of what's
possible. Purpose of this commit is ease of review. If this
passes review, much more (trickier stuff) will be forthcoming.

Signed-off-by: Ed Santiago [email protected]

Man page refactoring: add include mechanism for common options

podman-create and -run have many options in common. To date,
these are copy-pasted and haphazardly maintained.

Solution: add an include mechanism, '@@option foo', such
that multiple md source files can fetch from one common file.

This is a Phase One commit, a very small subset of what's
possible. Purpose of this commit is ease of review. If this
passes review, much more (trickier stuff) will be forthcoming.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago added the kind/documentation Categorizes issue or PR as related to documentation. label Aug 3, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2022
@edsantiago
Copy link
Member Author

@containers/podman-maintainers this is a small subset of #14931. The purpose of this commit is that 14931 is impossible to review -- I had to make way too many manual edits to the existing inconsistent mess. With this commit, I want you to focus on the MECHANISM: the tool for including options. I've written it in Python to make it easy to add as a step to our readthedocs process.

This subset of options was autogenerated by a pair of scripts I wrote for finding identical options; in this specific case, the descriptions, too, are identical. The whole goal right now is primum non nocere, confirming that this approach does no harm. To prove that, y'all can review the results via:

$ git co main; make clean; make docs; mv docs/build/man{,.main}
$ git co pr/15174 (or whatever you call this branch); make clean; make docs; diff -ur docs/build/man{.main,} 

That proves that there is zero difference between the man pages on main and the man pages generated with this code.

$ for i in docs/source/markdown/*.in;do f=${i%.in};git show main:$f | cdif -u - <(grep -v 'included file' $f);done

That proves that the only differences between the original .md files and the generated ones is whitespace. (See code comment for explanation).

Once (if) you're happy that this approach works, then please focus on the Makefile changes and the script itself.

If this merges, I will proceed with #14931 which adds a lot more cleanup but will be trickier to review because our man pages have diverged from each other. That will then become a wording-and-phrasing PR instead of a complicated-new-mechanism PR.

Comment on lines +429 to +436
# This does a bunch of filtering needed for man pages:
# 1. Strip markdown link targets like '[podman(1)](podman.1.md)'
# to just '[podman(1)]', because man pages have no link mechanism;
# 2. Then remove the brackets: '[podman(1)]' -> 'podman(1)';
# 3. Then do the same for all other markdown links,
# like '[cgroups(7)](https://.....)' -> just 'cgroups(7)';
# 4. Remove HTML-ish stuff like '<sup>..</sup>' and '<a>..</a>'
# 5. Replace "\" (backslash) at EOL with two spaces (no idea why)
Copy link
Member Author

Choose a reason for hiding this comment

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

Completely unrelated to this PR, but I had to sleuth that incomprehensible mess (for an earlier sed-based iteration of this work) and never, ever want to do that again.

@baude
Copy link
Member

baude commented Aug 3, 2022

lgtm

@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2022
@openshift-ci openshift-ci bot merged commit 549974d into containers:main Aug 3, 2022
@edsantiago edsantiago deleted the docs_dedup_phase1 branch August 3, 2022 20:31
@edsantiago
Copy link
Member Author

Gulp - I hadn't expected this to merge so quickly. Now, who here groks readthedocs? It looks like the build failed, probably because there's no "make docs" step (so podman-create.md and -run.md` don't get created). Who has the privs and know-how to add "make docs"?

@Luap99
Copy link
Member

Luap99 commented Aug 3, 2022

I can take a look tomorrow, just make sure to not backport this to v4.2

@edsantiago
Copy link
Member Author

Even easier than make docs: just run hack/markdown-preprocess. It will identify all *.md.in files and convert them to .md. (I forgot I added that).

@vrothberg
Copy link
Member

I reverted the commit in #15186 as the PR does not pass the OSX Cross job. Currently CI is red and reverting will buy us some time.

@gbraad
Copy link
Member

gbraad commented Aug 4, 2022

Rebasing our PRs as we saw: Makefile:133: invalid 'override' directive

@vrothberg
Copy link
Member

@gbraad, let's wait before rebasing on #15186. @Luap99 is looking into a proper fix. I just took the shortcut :^)

@gbraad
Copy link
Member

gbraad commented Aug 4, 2022

I understand. It seems 'override' in the text is interpreted as a statement?

edsantiago added a commit to edsantiago/libpod that referenced this pull request Aug 9, 2022
Followup to containers#15174. These are the options that are easy(ish)
to review: those that have only drifted slightly, and need
only minor tweaks to bring back to sanity. For the most part,
I went with the text in podman-run because that was cleaned up
in containers#5192 way back in 2020. These diffs primarily consist of
using '**' (star star) instead of backticks, plus other
formatting and punctuation changes.

This PR also adds a README in the options dir, and a new
convention: <<container text...|pod text...>> which tries
to do the right thing based on whether the man page name
includes "-pod-" or not. Since that's kind of hairy code,
I've also added a test suite for it.

Finally, since this is impossible to review by normal means,
I'm temporarily committing hack/markdown-preprocess-review,
a script that will diff option-by-option. I will remove it
once we finish this cleanup, but be advised that there are
still 130+ options left to examine, and some of those are
going to be really hard to reunite.

Review script usage: simply run it (you need to have 'diffuse'
installed). It isn't exactly obvious, but it shouldn't take more
than a minute to figure out. The rightmost column (zzz-chosen.md)
is the "winner", the actual content that will be used henceforth.
You really want an ultrawide screen here.

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants