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

Replace hoedown markdown parser with CMark #714

Merged
merged 7 commits into from Jan 13, 2023
Merged

Replace hoedown markdown parser with CMark #714

merged 7 commits into from Jan 13, 2023

Conversation

redstrate
Copy link
Contributor

@redstrate redstrate commented Jan 6, 2023

This is a draft for patches to replace the old, crusty and buggy hoedown markdown library - with CMark.

Benefits

  • Less resource management (no more hoedown.h needed to wrap api calls)
  • Already used in production by other big projects like KDE and GNOME
  • Maintained

CMark also supports raw HTML blocks, so stuff like mod pages are no longer broken and have to be worked around (#713):
image

Since we don't need a resource wrapper, and less quirks because hoedown is terrible - it also means less lines of code worrying about markdown rendering :-)

Tasks

  • Check everywhere where markdown is shown and make sure there aren't any rendering problems.
  • Vendor/import CMark (see question below)
  • Remove hoedown vendored source
  • Check around to see if I'm missing any quirks that were previously used with hoedown, I think I zapped all of them though.

Questions

  • Should we vendor in cmark considering it's in a lot of distribution repositories already (unlike hoedown/sundown?)
    It's a small library anyway, so it doesn't matter either way.
  • How should we handle the markdownToHTML function? I haven't touched the codebase in a while, so I'm not sure if you want anything more than a static global function.

@DioEgizio
Copy link
Member

Should we vendor in cmark considering it's in a lot of distribution repositories already (unlike hoedown/sundown?)
It's a small library anyway, so it doesn't matter either way.

Do like zlib/quazip/ghc-filesystem etc and check if it's installed in the system, dynamically link if found and if it isn't found or Launcher_FORCE_BUNDLED_LIBS is on statically link

@DioEgizio
Copy link
Member

DioEgizio commented Jan 6, 2023

Since modrinth uses GitHub flavoured markdown, might make more sense to use cmark-gfm (GitHub's cmark fork) instead

https://github.com/github/cmark-gfm

@DioEgizio
Copy link
Member

Should we vendor in cmark considering it's in a lot of distribution repositories already (unlike hoedown/sundown?)
It's a small library anyway, so it doesn't matter either way.

Do like zlib/quazip/ghc-filesystem etc and check if it's installed in the system, dynamically link if found and if it isn't found or Launcher_FORCE_BUNDLED_LIBS is on statically link

Forgor to say it but cmark-gfm is also commonly packaged (see https://repology.org/project/cmark-gfm/versions ), so yeah do what I said there ^

@Scrumplex
Copy link
Member

Should we vendor in cmark considering it's in a lot of distribution repositories already (unlike hoedown/sundown?)
It's a small library anyway, so it doesn't matter either way.

We probably need this at the very least for our MSVC Windows builds.

I have pushed a commit adding support for system installs of CMark as well as falling back to a submodule, if required.

I have also looked at cmark-gfm, but it looks like it doesn't provide CMake files to allow finding it using find_package. Might need to use pkgconfig for that.

@redstrate
Copy link
Contributor Author

I have also looked at cmark-gfm, but it looks like it doesn't provide CMake files to allow finding it using find_package. Might need to use pkgconfig for that.

Erk, that's annoying :-) But if we want to switch to GFM for whatever reason - it shouldn't change anything API wise so that's something we can put off doing until later. It's not like I encountered anything in the changelogs or mod pages I've viewed that requires GH extensions

@DioEgizio
Copy link
Member

DioEgizio commented Jan 7, 2023

I guess not having gfm extensions is not a big issue for now, but flatpak build for some reason is failing

@flowln flowln added the enhancement New feature or request label Jan 7, 2023
@flowln flowln added this to the 7.0 milestone Jan 7, 2023
@redstrate
Copy link
Contributor Author

I guess not having gfm extensions is not a big issue for now, but flatpak build for some reason is failing

The fix should be simple, I just need to add cmark to the flatpak manifest. I'll be adding a commit to fix that :-)

@redstrate
Copy link
Contributor Author

Also, is there a standard format for writing new license headers? I see a mix of some of the old license headers from the MultiMC/PolyMC days but also SPDX. I'm guessing I should mark it GPLv3 with SPDX.

launcher/Markdown.h Outdated Show resolved Hide resolved
launcher/Markdown.h Outdated Show resolved Hide resolved
@redstrate redstrate marked this pull request as ready for review January 7, 2023 20:39
@redstrate
Copy link
Contributor Author

I also added an additional fix, I noticed that Modrinth pages specifically has extra line breaks. These are now stripped (Curseforge and other mod platforms I tested don't suffer from this issue):

image

How it looked before:

image

@redstrate
Copy link
Contributor Author

I also tested every place I could (with the exception of the updater, I don't know how to test that easily). The commits are all signed, messages fixed up and ready for review now :-)

@redstrate redstrate changed the title WIP: Replace hoedown with CMark Replace hoedown markdown parser with CMark Jan 7, 2023
@DioEgizio
Copy link
Member

just found a mod using gfm stuff

https://modrinth.com/mod/immediatelyfast

uses tables

image

@redstrate
Copy link
Contributor Author

redstrate commented Jan 8, 2023 via email

@redstrate
Copy link
Contributor Author

redstrate commented Jan 11, 2023

I got around to working on this again, but looking through cmark gfm is nothing but pain.

Not only are they a fork of cmark - that part is fine - but they don't include important commits like github/cmark-gfm@c8d5be9 which fixes CMake find_package as shown above. I could make a PR upstream to fix that though.

Also the availability of cmark-gfm pales in comparison to regular cmark. Not that this is a problem if we include it as a submodule, this only matters for linux packaging.

And to include the relevant GitHub extensions (notably tables) I would have to expand the markdownToHtml function because there's no simple way to add extensions through the cmark API. Not that big of a deal, but annoying.

Okay, so let's just accept all of those problems and just take the plunge:

/usr/include/cmark-gfm-core-extensions.h:10:10: fatal error: config.h: No such file or directory
   10 | #include "config.h" // for bool
      |          ^~~~~~~~~~
compilation terminated.

No way, it includes a private header in a public API, so even if I wanted to include cmark-gfm support I couldn't :-( Oh there's an open PR for that but it hasn't been merged yet. Since there's been no activity, Nix and Ghostwriter had to implement workarounds for this bug, Nix PR here and ghostwriter commit here respectively.

Is there that many mods that use tables in their READMEs? It's a shame that they don't render, but honestly cmark-gfm is a baffling library. We could vendor cmark-gfm and then apply our own patches, but I think that's a bad idea for a lot of reasons (and ideally, this would be fixed upstream).

I think this PR is in a good state right now and we can look at cmark-gfm support later, because with just plain cmark our markdown is much better than before :-) Perfection cannot be the enemy of good, etc

@DioEgizio
Copy link
Member

Yeah let's just use normal cmark then

COPYING.md Outdated Show resolved Hide resolved
flatpak/org.prismlauncher.PrismLauncher.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

libraries/README.md Show resolved Hide resolved
@DioEgizio
Copy link
Member

DioEgizio commented Jan 12, 2023

When you rebase develop with signoff:

@redstrate
Copy link
Contributor Author

Oops, somehow a bunch of unrelated commits made it's way into here :S

redstrate and others added 6 commits January 12, 2023 10:08
Signed-off-by: Joshua Goins <josh@redstrate.com>
Signed-off-by: Joshua Goins <josh@redstrate.com>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Joshua Goins <josh@redstrate.com>
Signed-off-by: Joshua Goins <josh@redstrate.com>
this way we can just dynamically link it on that build instead of building it ourselves and statically linking it

Signed-off-by: DioEgizio <83089242+DioEgizio@users.noreply.github.com>
@flowln flowln merged commit b937d33 into PrismLauncher:develop Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants