Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Makefile.inc1: Also cleanup (un)compressed manpages #1295

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
Loading
from
Open

Makefile.inc1: Also cleanup (un)compressed manpages #1295

wants to merge 2 commits into from

Conversation

ricardobranco777
Copy link
Contributor

@ricardobranco777 ricardobranco777 commented Jun 16, 2024 •
edited
Loading

Cleanup (un)compressed manpages in the check-old-files & delete-old-files targets.

Also cleanup manpages for arches not included in MAN_ARCH, if specified.

PR:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279792

@concussious
Copy link
Contributor

(I don't really understand where is the appropriate place to have this conversation so I'm posting both places -- guidance welcome)

Maybe that's a feature?

With compressed manual pages, you can install a locally modified manual page alongside the compressed ones. Man(1) chooses the uncompressed version first, and the compressed versions are still tracked and updated with the system, while leaving the locally modified manuals alone. There are other ways to do this, but this way is zero conf.

Nobody told me to do it this way, but I've been doing this for years.

If others agree that it's better to clean them up instead, I'm okay with that.

@ricardobranco777
Copy link
Contributor Author

With compressed manual pages, you can install a locally modified manual page alongside the compressed ones. Man(1) chooses the uncompressed version first, and the compressed versions are still tracked and updated with the system, while leaving the locally modified manuals alone. There are other ways to do this, but this way is zero conf.

Why would you have a locally modified manual page?

I enabled ZFS compression and having compressed manual pages is a waste.

@concussious
Copy link
Contributor

concussious commented Jun 17, 2024 •
edited
Loading

Why would you have a locally modified manual page?

hier(7): "A well-maintained installation will include a customized version of this document."

Best reason: man page developmemt and testing.

Corner reason: upstream and user are opinionated about doc in divergent ways.

Secret reason: keeps their playbook in the form of examples in custom manpages 😎 man foo is more elegant than m ~/notes/fx.foo.

I enabled ZFS compression and having compressed manual pages is a waste.

I have had this setup sometimes but that's pretty custom. Iirc without_mancompress is a src.conf setup.

I could manpath +/man/working just as easily as you could rm /.gz.

How much of a waste is it? Zfs compression knows to bypass compressed files.

Again, I don't object to changing it, but I do think it's important that this is mentioned as part of the discussion.

@ricardobranco777
Copy link
Contributor Author

Why would you have a locally modified manual page?

hier(7): "A well-maintained installation will include a customized version of this document."

Best reason: man page developmemt and testing.

This is the worst reason. Why use a staging manpage in production? You could manpath for that.

bsdimp
bsdimp reviewed Jun 17, 2024
View reviewed changes
Makefile.inc1
@@ -3522,6 +3537,21 @@ check-old-files: .PHONY
echo $${catpage}; \
fi; \
done | sort
.if ${MK_MANCOMPRESS} != "yes"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this could somehow just be on the inner if rather than having two identical calls copies of the loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this could somehow just be on the inner if rather than having two identical calls copies of the loop

The problem are the backslashes.

Copy link
Member

Choose a reason for hiding this comment

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

Let me play with something to overcome that

Copy link
Member

Choose a reason for hiding this comment

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

I've not found time to do this, so please don't let it block this. We/I can refine after the comment when I find time.

@concussious
Copy link
Contributor

Why use a staging manpage in production?

I test in production because I'm embarrassed when I proposed flawed changes in default behavior and scared of mutually unknown unknowns.

@ricardobranco777
Copy link
Contributor Author

Added support to also cleanup manpages for arches not included in MAN_ARCH, if specified.

@bsdimp
Copy link
Member

bsdimp commented Jul 8, 2024

I wonder... what do you think about making this a script in tools/build somewhere that we run. Then the repetition won't be a problem...

@ricardobranco777
Copy link
Contributor Author

I wonder... what do you think about making this a script in tools/build somewhere that we run. Then the repetition won't be a problem...

I did just that in a new commit. Tell me if you like this approach to improve it further and then squash and rebase.

bsdjhb
bsdjhb reviewed Jul 30, 2024
View reviewed changes
Copy link
Member

@bsdjhb bsdjhb left a comment

Choose a reason for hiding this comment

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

I'm less certain of a need for cleanup script vs doing it inline.

Makefile.inc1 Show resolved Hide resolved
Makefile.inc1
@@ -3497,6 +3497,23 @@ delete-old-files: .PHONY
rm ${RM_I} $${catpage} <&3; \
fi; \
done
.if ${MK_MANCOMPRESS} != "yes"
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment here would be:

# Remove compressed copies of uncompressed manpages.

Makefile.inc1 Show resolved Hide resolved
bsdjhb
bsdjhb reviewed Jul 30, 2024
View reviewed changes
Copy link
Member

@bsdjhb bsdjhb left a comment

Choose a reason for hiding this comment

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

Or at least, if we had a cleanup script, the cleanup script should do all the things including removing catpages, dealing with mismatched MAN_ARCH pages, etc. To that end, I would probably just leave the last commit off and focus on the first ones. I think the duplication is ok TBH.

@ricardobranco777
Copy link
Contributor Author

Or at least, if we had a cleanup script, the cleanup script should do all the things including removing catpages, dealing with mismatched MAN_ARCH pages, etc. To that end, I would probably just leave the last commit off and focus on the first ones. I think the duplication is ok TBH.

I applied your suggestions. Hopefully fixed now..

@concussious
Copy link
Contributor

concussious commented Jul 30, 2024 •
edited
Loading

IIUC @jrtc27's comment in the bug tracker, this is actually a bug fix. I would like to humbly request changes to commit msg:

Closes: 1295
MFC after: 3 days
Fixes: 460f173dad6e (Remove old documents)
Reported by: jrtc
Reviewed by: imp, jhb
Pull Request: https://github.com/freebsd/freebsd-src/pull/1295

EDIT: forgot some trailer details, sorry.
EDIT2: ok. Something (many things?) happened to 460f173 to make all this non-obvious. This is exactly why I really care about commit messages.

@ricardobranco777
Copy link
Contributor Author

IIUC @jrtc27's comment in the bug tracker, this is actually a bug fix. I would like to humbly request changes to commit msg:

We all care about commit messages. And I'll change it after:

  1. The code review is finished.
  2. Maybe I'm asked to squash 2 commits into one.
  3. Last but not least, know why the old commit fails.

I've been doing PR's before and know the drill.;)

@concussious
Copy link
Contributor

I've been doing PR's before and know the drill.;)

Sorry, I did not mean to imply otherwise. I've been enjoying following your work, which is very cool and well above my skill level!

@ricardobranco777
Copy link
Contributor Author

I've been doing PR's before and know the drill.;)

Sorry, I did not mean to imply otherwise. I've been enjoying following your work, which is very cool and well above my skill level!

Actually, I've been never asked to add the by: fields to the commit messages. Those have been added by the reviewers so far.

@ricardobranco777
Copy link
Contributor Author

We must have a conversation about the wastefulness of compressed manpages. They don't save that much space and the way we install them fills up space in ZFS snapshots by removing manpages and then installing them:

rm -f /usr/share/man/man9/callout_active.9 /usr/share/man/man9/callout_active.9.gz;  install -C -l h -o root -g wheel -m 444  /usr/share/man/man9/callout.9 /usr/share/man/man9/callout_active.9

@bsdimp
Copy link
Member

bsdimp commented Aug 2, 2024

DO NOT ADD PULL REQUEST: LINES TO YOUR COMMIT MESSAGES

OK. Please don't add these, as it slows me down when I'm landing the pull request. My landing software automatically adds them and I have to go in and remove the duplicates.

@concussious
Copy link
Contributor

I'm sorry for suggesting the incorrect information.

@bsdimp
Copy link
Member

bsdimp commented Sep 22, 2024

Looking at it again, this looks good enough for now. Is there some reason for @bapt to do the review?

@bsdimp bsdimp self-assigned this Oct 4, 2024
@ricardobranco777
Copy link
Contributor Author

I've been using this patch on STABLE-14 for weeks without issues.

@ricardobranco777
Copy link
Contributor Author

It's been months and this hasn't been merged. What happened?

@bsdimp
Copy link
Member

bsdimp commented Apr 6, 2025

For me, it's just no time.

@kevans91 kevans91 added the ready Seems ready to go, subject to final sanity check label Apr 22, 2025
@ricardobranco777
Copy link
Contributor Author

This PR has been working for me on 14 & 15. Can we merge at least for 15?

@bsdimp
Copy link
Member

bsdimp commented Jun 5, 2025

I plan on merging this on Friday this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsdimp bsdimp bsdimp left review comments

@bsdjhb bsdjhb bsdjhb approved these changes

@bapt bapt Awaiting requested review from bapt bapt is a code owner

Assignees

@bsdimp bsdimp

Labels
ready Seems ready to go, subject to final sanity check
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ricardobranco777 @concussious @bsdimp @bsdjhb @kevans91
Morty Proxy This is a proxified and sanitized view of the page, visit original site.