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

invoke man(1) when requesting help #53

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Conversation

cvengler
Copy link
Member

This commit invokes the man(1) command with the rosenpass manual page, when invoking help.

In the future, we should handle the case, that a rosenpass manual page is not installed, somewhat better. My idea would be to render the manual page to an HTML version regularly and provide a link to it inside the error message.

@koraa
Copy link
Member

koraa commented Apr 15, 2023

That's great! Could we include a pre-render of the page and output that as a fallback?

@cvengler
Copy link
Member Author

That's great! Could we include a pre-render of the page and output that as a fallback?

Sure thing! :-)
Manual pages can be compiled into ASCII (e.g. groff -Tascii rosenpass.1 or mandoc -Tascii rosenpass.1). The only "problem" I see with this, would be to add somewhat of a build dependency to cargo, which invokes the command above, so that we can include_str!() it.

My alternative idea is outlined in #55. Simply provide an online version of the manual and output a link to it, in case it is not found. This will keep build dependencies small.

@koraa
Copy link
Member

koraa commented Apr 16, 2023

@emilengler

Manual pages can be compiled into ASCII (e.g. groff -Tascii rosenpass.1 or mandoc -Tascii rosenpass.1). The only "problem" I see with this, would be to add somewhat of a build dependency to cargo, which invokes the command above, so that we can include_str!() it.

Sounds great! Are there any drawbacks to that? I think the build should not fail if groff or mandoc are unavailable, and we might add a crate feature to turn including the manpage off explicitly…

@cvengler
Copy link
Member Author

@koraa I have done some changes: First and foremost, I have added a build script, that tries to compile the manual page into ASCII. Afterwards, I have adjusted the help function to invoke man and output the built-in manual, if this fails.

@koraa
Copy link
Member

koraa commented Apr 16, 2023

@emilengler Thank you! Looks good!
@wucke13 How bad is adding a build.rs from a packaging standpoint?

@cvengler
Copy link
Member Author

Oh by the way: I think, that mandoc should now become a part of the flake...

@cvengler
Copy link
Member Author

Done :-)

@koraa
Copy link
Member

koraa commented Apr 16, 2023

Done :-)

Thats awesome! Thank you, lets wait for @wucke13 to give an opinion about the compatibility of this!

@koraa
Copy link
Member

koraa commented Apr 21, 2023

@wucke13 Are the issues causing all PRs to fail part of the PRs or are they part of the gh actions issue we had before?

I think this PR is mostly ready except for your input about the impact of having a build.rs…

@wucke13
Copy link

wucke13 commented Apr 21, 2023

I think this time the CI is legitimately doing what it is supposed to, finding a small bug 😄

   > error: environment variable `ROSENPASS_MAN` not defined
   >    --> src/main.rs:296:26
   >     |
   > 296 |     eprint!(include_str!(env!("ROSENPASS_MAN")));
   >     |                          ^^^^^^^^^^^^^^^^^^^^^
   >     |
   >     = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)
   >
   > error: could not compile `rosenpass` due to previous error

@cvengler
Copy link
Member Author

Is it possible, that the CI omits the build.rs file? Because if build.rs is run, it will (to my knowledge) always set the envvar.

@cvengler
Copy link
Member Author

Nevermind, I've just got an idea while writing this. I need a rubber ducky

@cvengler
Copy link
Member Author

Hopefully fixed in 12e8bb1.

@cvengler cvengler force-pushed the invoke-man branch 2 times, most recently from 40de278 to b2384fc Compare April 22, 2023 11:03
@cvengler
Copy link
Member Author

Ready for merge in my opinion.

This commit invokes `man(1)` when requesting help and emits the built-in
manual, if the manual page is not found on the system.
@cvengler
Copy link
Member Author

Merge conflict solved.

@cvengler
Copy link
Member Author

Something fishy is going with the aarch64 CIs. It looks like a dependency broke on this architecture...

@koraa
Copy link
Member

koraa commented Apr 26, 2023

Sorry for the delay merging this; I'll check with @wucke13 on his opinion regarding the build.rs

@koraa
Copy link
Member

koraa commented Apr 26, 2023

Lets get this damn thing merged unless @wucke13 sees issues with using a build.rs!

@emilengler did you get a t-shirt by the way?

@koraa koraa mentioned this pull request Apr 26, 2023
@cvengler
Copy link
Member Author

Lets get this damn thing merged unless @wucke13 sees issues with using a build.rs!

@emilengler did you get a t-shirt by the way?

Not yet, but I will be at the release party next Friday =)

@wucke13 wucke13 merged commit d5b2a94 into rosenpass:main Apr 27, 2023
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.

None yet

3 participants