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

Add API for version and date #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

codesections
Copy link
Member

This PR adds the API for version and date as discussed in #26. Because the current date would be the date that the man page is generated (which might be much later than the date it was authored given that the docs contemplate runtime generation of the man page) this PR accepts the date as a user-specified string.

As noted in the comments, the code is slightly less elegant than would be ideal because the roff-rs API is not designed to include version and date info in the .TH element (which is where it needs to be placed). I plan to make a PR to that crate eventually but wanted man to have this functionality in the meantime.

@yoshuawuyts
Copy link
Collaborator

Because the current date would be the date that the man page is generated (which might be much later than the date it was authored given that the docs contemplate runtime generation of the man page) this PR accepts the date as a user-specified string.

I guess we're using this crate differently. I've intended for this crate to be used from build.rs files, statically, to generate man pages that can be distributed with a package. To my knowledge it's not possible to provide man pages as part of the binary. I'd love it if there were a way tho!

I think it would be reasonable to default by default to the current date, and allow people to override it using .date() with a string.

@codesections
Copy link
Member Author

Hmmm, please correct me if I'm wrong, but wouldn't that build process still result in the wrong current date most of the time? For example, if someone installs your package with cargo install or by cloning the GitHub repo and running cargo build --release, then the build.rs file would be triggered and the man page would be re-built with the current date, no?

That wouldn't happen if someone installs a pre-built binary, but that seems like it isn't a guarantee (though it would be better for more rust utilities to be packaged in various package repositories, it still seems inevitable that a lot will be installed with cargo install at least in the short term).

@codesections
Copy link
Member Author

Here's an idea that I'd appreciate your thoughts on:

We could have the date default to querying the crates.io API and getting the updated_at value. (See https://crates.io/api/v1/crates/man). We could have a fallback of the current date in cases where the user can't reach crates.io (e.g., no Internet access).

The reason this is appealing is to handle the cargo install use case—the time we can be sure that the users can access crates.io is when they are in the process of installing the package with cargo install!

On the other hand, this is starting to feel like a bit of over-engineering, so if you'd prefer to go a different route I'd definitely understand.

@yoshuawuyts
Copy link
Collaborator

We could have the date default to querying the crates.io API and getting the updated_at value. (See https://crates.io/api/v1/crates/man).

I don't think this is feasible because of the amount of dependencies that would require pulling in would be non-trivial. In essence it means we now either need to compile libcurl or reqwest on every prebuild, and possible include them in the resulting binary. That means we'll take big hit in terms of compilation times, and possibly also binary size just so we can add a date to the bottom of a man page.

A much more efficient solution that would work on both versions would be to use a macro or const fn that resolves at compile time containing the date the crate was compiled on.

@codesections
Copy link
Member Author

I don't think [querying the crates.io API] is feasible because of the amount of dependencies that would require pulling in would be non-trivial.

Agreed, that makes perfect sense. man builds super fast right now (~2 sec for release on my laptop) and keeping it lean seems key to getting it more widely adopted/improving documentation in the Rust community. I withdraw the suggesting of querying crates.io :)

A much more efficient solution that would work on both versions would be to use a macro or const fn that resolves at compile time containing the date the crate was compiled on.
(emphasis added)

Hmm, I'm still a bit confused about how you're imagining man being used where that would be the date we'd want in the man page. I put an example in #33 of how I was imagining people using man—that is, generating a man page as part of their compile process in a build.rs file. But if they do that, then any user of their crate who runs cargo install will re-compile the crate and re-generate the man page—which would mean that the date on the man page would be the date at which the crate was installed by that user, not the date when it was last updated by the maintainer.

Am I missing something here?

@yoshuawuyts
Copy link
Collaborator

But if they do that, then any user of their crate who runs cargo install will re-compile the crate and re-generate the man page.

This only applies to cargo install, and not OS distributions. I don't think we should focus too much on what cargo install can offer us beyond a prototyping / testing, as the Cargo team seems to treat it similarly. I think we're better off focusing on integrating with package distributors, as outlined in https://github.com/rust-lang-nursery/cli-wg/issues/8.

@codesections
Copy link
Member Author

This only applies to cargo install, and not OS distributions.

Well, it doesn't just apply to cargo install—it also applies to packages distributed with a makefile that builds the package from source or to source-only distributions (e.g., Gentoo). I agree that it wouldn't apply to most OS distributions, though.

I don't think we should focus too much on what cargo install can offer us beyond a prototyping / testing, as the Cargo team seems to treat it similarly. I think we're better off focusing on integrating with package distributors, as outlined in rust-lang-nursery/cli-wg#8.

I'm a little worried that we're at risk of letting the perfect be the enemy of the good-enough here. I agree that, in an ideal world, developers would distribute their programs through OS package managers and those tools would take care of installing the man page in the appropriate location/users wouldn't be compiling from source that often. However, I think we're currently very far from that ideal world crates.io lists 616 crates in the "Command Line Utilities" category, and my sense is that the vast majority of them are (currently) available primarily through cargo install. As a random sample, I checked the packages on page 10, and there was one available as a .deb, a handful available as an AUR package—and nearly all mentioned installing with cargo install.

So, as much as I'd like everyone to install with package managers—and plan to contribute to projects that make that easier—my view is that we should build man to support the current reality that a lot of CLI apps are distributed primarily through cargo (especially when they are getting started, which is hopefully when they'd add man as a dependency).

Just my 2¢, though, and I'm happy to work on a PR implementing the other approach if that's the direction you want to go with the crate.

(One final note, though—even if a program is distributed through a package manager, it's sometimes (often?) packaged by someone other than the developer. In that case, the third-party packager would compile the source code, and that could take place significantly after the developer last edited the code. If the date in the man page updates on every compilation, then the man page would reflect when the program was last packaged, not when it was last updated.)

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.

2 participants