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

Feature/manpage #596

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Conversation

andrews05
Copy link
Collaborator

@andrews05 andrews05 commented Mar 5, 2024

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.

@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.

@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
This is much less fragile than messing with `OUT_DIR`, whose location
relative to the `target` directory is purposefully underspecified.
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add man page for the command-line utility
3 participants