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.

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

Feature/manpage #596

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Feature/manpage #596

merged 6 commits into from
Mar 18, 2024

Conversation

andrews05
Copy link
Collaborator

@andrews05 andrews05 commented Mar 5, 2024 •
edited
Loading

This PR adds a build script to generate a man page using clap_mangen, as per this example:
https://github.com/sondr3/clap-man-example/blob/main/build.rs

I'm not sure what to actually do with the man file from here, I guess it's up to the packaging process to do something with it?
See #69 (comment)

Note I couldn't see a way to include the DISPLAY chunk names from the constant as we did before. They're now just hardcoded into the help and will require manually updating if the list changes.

Closes #526

@andrews05
Copy link
Collaborator Author

Maybe we can use cargo-deb to make a deb package in the github workflow.

@andrews05 andrews05 mentioned this pull request Mar 16, 2024
Closed
@musicinmybrain
Copy link
Contributor

I'm not sure what to actually do with the man file from here, I guess it's up to the packaging process to do something with it? See #69 (comment)

Speaking as the current maintainer of the Fedora and EPEL packages, if the build process generates a man page then that’s all I need. I can easily install it.

@andrews05 andrews05 marked this pull request as ready for review March 16, 2024 23:56
@andrews05
Copy link
Collaborator Author

@musicinmybrain Excellent, that's what I was hoping.

@andrews05 andrews05 requested a review from AlexTMjugador March 17, 2024 02:27
@AlexTMjugador
Copy link
Collaborator

This is beautiful, great work! For reference, here's an excerpt of the rendered page:

Rendered man page

It was faster for me to push two extra small changes than explaining them on a PR review, but the summary of what I did is as follows:

  • I modified the build script to generate man page files to the generated/assets folder instead of target/assets. The rationale for this change is that we need to create files outside of OUT_DIR anyway, and OUT_DIR's relative location to target is purposefully underspecified. The previous logic could fail when using the --target for a fresh cargo build, and maybe if we do weird shenanigans to change the target directory, so it's best to leverage the fact that build scripts are executed from a consistent working directory and put stuff into a well-known directory that's ignored by Git.
  • I devised a way to bring back the generation of DISPLAY chunk names from the source code constant, so we don't need to hardcode them anymore 😄

andrews05 and others added 6 commits March 18, 2024 12:22
@andrews05 @AlexTMjugador
Separate cli command construction into new file
fab3a43
@andrews05 @AlexTMjugador
Add build script for man generation
58b3784
@andrews05 @AlexTMjugador
Downgrade clap_mangen
4109dd5
@andrews05 @AlexTMjugador
Specify assets for cargo-deb
64e1a04
@AlexTMjugador
Generate man pages to non-Cargo directory
d7f6757 
This is much less fragile than messing with `OUT_DIR`, whose location
relative to the `target` directory is purposefully underspecified.
@AlexTMjugador
Bring back generation of help strings from DISPLAY chunks constant
3ce88eb 
This is done by extracting such a constant to a separate module file,
outside of `StripChunks`.
@AlexTMjugador AlexTMjugador merged commit f4e631b into shssoichiro:master Mar 18, 2024
10 checks passed
@andrews05 andrews05 deleted the feature/manpage branch March 18, 2024 20:27
@andrews05
Copy link
Collaborator Author

Awesome, thanks @AlexTMjugador, your changes look great!

What do you think about changing the workflow to use cargo-deb for the linux builds?

@AlexTMjugador
Copy link
Collaborator

What do you think about changing the workflow to use cargo-deb for the linux builds?

I think that's a good and doable idea. We can begin distributing the generated .deb ourselves too, so even if we don't make it to the official Debian repositories anytime soon, people can still manage their OxiPNG install with the APT package manager.

@pereorga pereorga mentioned this pull request Apr 23, 2024
Closed
AlexTMjugador added a commit that referenced this pull request Nov 19, 2024
@AlexTMjugador
Move manpages generation to an xtask
4c310c1 
PR #596 brought forward automatic generation of Linux manual pages for
Oxipng, which is executed every time Oxipng is built. However, while
building manpages on every build is convenient for Oxipng development
and doing so didn't catch my attention initially, it introduces
noticeable inefficiencies for crates using Oxipng as a library: during
their build, Oxipng manpages are also built, even though most dependent
crates won't use such artifacts, as they are not considered part of the
public Oxipng crate API or even appropriate for non-human consumption.

Moreover, generating manpages depends on `clap`, which is a heavyweight
dependency: according to a fresh `cargo build --timings --release` on my
development workstation, its `clap_builder` dependency is the third most
time consuming unit to build, totalling 1.5 s (out of 11.7 s, or 12.8%).
And there is no way for dependent crates to turn this off:
[`build-dependencies` cannot be conditional on crate
features](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies).
Potentially using other `cfg` hacks to either enable or disable
manpage generation is unergonomic, if not outright disallowed. Besides
reducing their compilation time cost, dependent crates may also want to
trim the size of their dependency tree, avoiding unnecessary dependency
downloads in the process.

Therefore, a better solution to conditionally build manpages in a way
convenient for both Oxipng maintainers and downstream consumers is
needed. My proposal implemented in this PR is to leverage the
[`cargo-xtask`](https://github.com/matklad/cargo-xtask) convention to
define an auxiliary crate to move the manpage generation logic and
dependencies to, which is to be used exclusively by Oxipng maintainers
and not part of the `oxipng` crate published on `crates.io`. That way
Oxipng maintainers and packagers can still generate manpages at request
with ease, without any automation being noticeable to uninterested crate
consumers. And as a side benefit, Oxipng maintainers can also benefit
from slightly faster iteration times due to the lack of a build script
for the main crate.

The new `mangen` xtask can be run at any time with `cargo xtask mangen`.
The generated manpages are now available at `target/xtask/mangen/manpages`.
Existing deployment scripts were updated accordingly.
@AlexTMjugador AlexTMjugador mentioned this pull request Nov 19, 2024
Merged
AlexTMjugador added a commit that referenced this pull request Nov 24, 2024
@AlexTMjugador
Move manpages generation to an xtask (#645)
c81a863 
PR #596 brought forward automatic generation of Linux manual pages for
Oxipng, which is executed every time Oxipng is built. However, while
building manpages on every build is convenient for Oxipng development
and doing so didn't catch my attention initially, it introduces
noticeable inefficiencies for crates using Oxipng as a library: during
their build, Oxipng manpages are also built, even though most dependent
crates won't use such artifacts, as they are not considered part of the
public Oxipng crate API or even appropriate for non-human consumption.

Moreover, generating manpages depends on `clap`, which is a heavyweight
dependency: according to a fresh `cargo build --timings --release` on my
development workstation, its `clap_builder` dependency is the third most
time consuming unit to build, totalling 1.5 s (out of 11.7 s, or 12.8%).
And there is no way for dependent crates to turn this off:
[`build-dependencies` cannot be conditional on crate
features](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies).
Potentially using other `cfg` hacks to either enable or disable manpage
generation is unergonomic, if not outright disallowed. Besides reducing
their compilation time cost, dependent crates may also want to trim the
size of their dependency tree, avoiding unnecessary dependency downloads
in the process.

Therefore, a better solution to conditionally build manpages in a way
convenient for both Oxipng maintainers and downstream consumers is
needed. My proposal implemented in this PR is to leverage the
[`cargo-xtask`](https://github.com/matklad/cargo-xtask) convention to
define an auxiliary crate to move the manpage generation logic and
dependencies to, which is not part of the `oxipng` crate published on
`crates.io`. That way Oxipng maintainers and packagers can still
generate manpages at request with ease, without any automation being
noticeable to uninterested crate consumers. And as a side benefit,
Oxipng maintainers can also benefit from slightly faster iteration times
due to the lack of a build script for the main crate.

The new `mangen` xtask can be run at any time with `cargo xtask mangen`.
The generated manpages are now available at
`target/xtask/mangen/manpages`. Existing deployment scripts were updated
accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexTMjugador AlexTMjugador AlexTMjugador approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add man page for the command-line utility
3 participants
@andrews05 @musicinmybrain @AlexTMjugador