lightning icon indicating copy to clipboard operation
lightning copied to clipboard

Remove mrkd

Open rustyrussell opened this issue 1 year ago • 13 comments

As requested by @wtogami ...

Changelog-None

rustyrussell avatar Jul 20 '22 12:07 rustyrussell

Just a word of caution: lowdown is only available in Ubuntu LTS starting from 22.04. So we might need to also add pointers to install it on older versions somehow.

cdecker avatar Jul 21 '22 11:07 cdecker

Just a word of caution: lowdown is only available in Ubuntu LTS starting from 22.04. So we might need to also add pointers to install it on older versions somehow.

It isn't available on Fedora at all yet but I will newly package it and all other build deps for that distro to handle offline builds without pip or poetry.

wtogami avatar Jul 22 '22 00:07 wtogami

It isn't available on Fedora at all yet but I will newly package it and all other build deps for that distro to handle offline builds without pip or poetry.

Mh! we really achieve the offline build by kicking off pip and poetry here? or are we just solve it for now? if rust enters as required dependencies (it already is a required dep because you can not change an RPC command without it) the offline problem is open again right? or there is some way to use cargo in an offline mode that I'm not aware of?

I'm more than a pro to kick off mrkd because the maintainer is slow and unresponsive, and we can always put lowdown as a submodule because we already build C, so should be not a big deal! (and we can always keep a fork if some bug exists, I think we have enough people with good knowledge of C to keep a tiny markdown parser alive :) )

vincenzopalazzo avatar Jul 24 '22 10:07 vincenzopalazzo

Mh! we really achieve the offline build by kicking off pip and poetry here? or are we just solve it for now? if rust enters as required dependencies (it already is a required dep because you can not change an RPC command without it) the offline problem is open again right? or there is some way to use cargo in an offline mode that I'm not aware of?

Distros had long packaged python modules to such a degree that many users never have used pip.

https://doc.rust-lang.org/cargo/commands/cargo-vendor.html The usual solution for offline rust builds is with cargo vendor.

I'm more than a pro to kick off mrkd because the maintainer is slow and unresponsive, and we can always put lowdown as a submodule because we already build C, so should be not a big deal! (and we can always keep a fork if some bug exists, I think we have enough people with good knowledge of C to keep a tiny markdown parser alive :) )

lowdown can easily be a tool packaged in distros. Building and running it as a build dependency is less desirable for reasons like it wouldn't work for cross-compiles.

wtogami avatar Jul 26 '22 00:07 wtogami

Manually inspected diff -u lightning-before/doc lightning-after/doc and found some oddities.

lightning-decodepay.7

Before

Screenshot from 2022-07-25 20-36-03

After

Screenshot from 2022-07-25 20-36-14

I prefer the more information dense bullets. But it mangled min_final_cltv_expiry in such a way that man misinterprets some _ literals as underline.

wtogami avatar Jul 26 '22 01:07 wtogami

image

Here's the diff -u of particularly the min_final_cltv_expiry example from above.

wtogami avatar Jul 26 '22 01:07 wtogami

These before and after shots show the typical difference from mrkd to lowdown.

Before

Screenshot from 2022-07-25 21-02-08

After

Screenshot from 2022-07-25 21-02-20

  • What bothers me about many of the lowdown formatted outputs is the 1st level bullets are not indented.
  • Various escaping changes make almost no difference except when it causes errors like in the above decodepay example. This page had no problems just the SEE ALSO is no longer bold.

wtogami avatar Jul 26 '22 02:07 wtogami

.gitignore

# Ignore generated files
devtools/features
doc/lightning*.[1578]

I ran into a confusing issue when switching between before and after branches where dependency tracking failed to recognize it needed to re-process the man pages. I then tried make clean and it still failed to redo these files. Perhaps make clean could also additionally delete the same files ignored by .gitignore?

wtogami avatar Jul 26 '22 02:07 wtogami

Here's the diff -u of the doc/ directory generated before and after this PR. https://fedorapeople.org/~wtogami/a/2022/e70729b04befe44d603370b96fbb10dc00bd342d...77f0388b6757bc6c7b9909521b0eadb0c55bd773-manpage.diff

~~Comparing before and after is tedious but we should also visually inspect the HTML formatted output as a sanity check so it won't be a surprise later when the docs website is regenerated.~~ edit: Oops we use sphinx to generate the documentation web pages. So we use lowdown for only man page generation? Then there isn't much remaining to inspect.

wtogami avatar Jul 26 '22 02:07 wtogami

For the record, clightning is now broken in nixpkgs and nix-bitcoin because mrkd is now broken in nixpkgs - https://github.com/NixOS/nixpkgs/blob/0b73b43a942d4f264036c74d03758194ba0b5855/pkgs/development/python-modules/mrkd/default.nix#L27-L28

prusnak avatar Aug 16 '22 21:08 prusnak

The above review found only cosmetic or minor formatting bugs. Build being broken is a bigger problem. For that reason maybe we should merge this before 0.12?

wtogami avatar Aug 16 '22 22:08 wtogami

I don't think this should go in now, at it risks breaking every other platform that now needs to figure out how to get lowdown, update their docs etc.

rustyrussell avatar Aug 17 '22 01:08 rustyrussell

How about pre-process the man pages, commit and skip that build step for the 0.12 release? That would be the least risky change while maximizing ease of deployment. Revert it immediately after 0.12 is cut.

wtogami avatar Aug 17 '22 01:08 wtogami

image

Here's the diff -u of particularly the min_final_cltv_expiry example from above.

OK, I've fixed this now.

rustyrussell avatar Sep 05 '22 21:09 rustyrussell

Rebased, and formatting fix. The extra blank line before sub-indents is a bit jarring, but seems the best we can do for now?

(See man -l doc/lightning-decode.7)

rustyrussell avatar Sep 05 '22 21:09 rustyrussell

Just a word of caution: lowdown is only available in Ubuntu LTS starting from 22.04. So we might need to also add pointers to install it on older versions somehow.

Latest draft here will build one internally if it's not found by configure. It seems fairly portable...

rustyrussell avatar Sep 06 '22 04:09 rustyrussell