swsnr / mdcat

cat for markdown
https://crates.io/crates/mdcat
Mozilla Public License 2.0
2.15k stars 64 forks source link

Consider dropping the `release_max_level_warn` feature from `tracing` dependency #242

Closed flavio closed 1 year ago

flavio commented 1 year ago

Hello, I'm one of the maintainers of kubewarden, I'm reaching out because we are using mdcat as a library inside of our kwctl command line utility.

mdcat is doing an amazing job at rendering markdown, this is the best solution we found out there. Congratulations! :heart: This is why we are doing something kinda dirty, we're consuming the mdcat crate to implement the kwctl inspect command (source code here).

I recently discovered that all our tracing::info output was now shown when kwctl was compiled in release mode. After some spelunking I found the mdcat dependency is causing that because of this line:

https://github.com/swsnr/mdcat/blob/28a2d2579959fe9472bbea012fb6c199c9b65807/Cargo.toml#L30

I was wondering if you could drop the release_max_level_warn feature from the tracing dependency. I've seen that inside of your main.rs you're actually disabling any kind of trace event:

https://github.com/swsnr/mdcat/blob/28a2d2579959fe9472bbea012fb6c199c9b65807/src/bin/mdcat/main.rs#L164-L170

Shouldn't that be enough? I tried patching the Cargo.toml and from some quick tests everything seemed fine (as in, no trace event was printed).

Another possible solution, a better one IMHO, would be to move mdcat libraries to a dedicated crate that could then be consumed by third parties like us. What do you think about that? :pleading_face:

Again, thanks a lot for the awesome job you did with this project!

swsnr commented 1 year ago

Thank you for your kind words šŸ™‚

I'm not sure I would not like to disable that feature; mdcat does a lot of internal tracing in the rendering code to help me understand rendering states, and I'm not sure if it's a good idea to have all that in a release build šŸ¤·šŸæ

But I'd happily consider a pull request which splits out the library code, i.e. lib.rs and downwards, into a separate say pulldown-cmark-tty crate or so, and sets up all the cargo workspace stuff, also for Github actions and cargo release. I might do this myself at some point, but it will take a while, since mdcat is a very side project for me.


Also, I have only a coarse idea what kubewarden is, but your website looks very professional and talks about "policy" and "admission", so I presume it's some enterprise-level security-related thing, and I feel like I should have things straight: mdcat comes with absolutely no warranty. Any bugs or security issues you find in mdcat in your project are yours to fix, and any kind of DCO, SBOM or whatever bureaucracy that sometimes goes along with enterprise-level open source things is yours to do, and you will likely have to wait long time for me to review any pull requests. Do not expect me to do anything here.

If you expect me to do even just basic maintenance here, I'd prefer if you didn't use mdcat šŸ™‚

I'm sorry for being blunt, more so, if my guestimate about kubewarden is entirely wrong šŸ˜‡ but I think it's important to clarify what you can expect from me (nothing really šŸ˜‡ ) if you're using mdcat in a project at enterprise level.

flavio commented 1 year ago

I'm not sure I would not like to disable that feature; mdcat does a lot of internal tracing in the rendering code to help me understand rendering states, and I'm not sure if it's a good idea to have all that in a release build šŸ¤·šŸæ

In theory, filtering the trace/debub/info statement using a log level doesn't harm the performance of the final binary. The compilation feature you're using is kinda of a last resort. All of that according to the information provided ~6 months ago on this reddit comment by one of the maintainers of tracing.

I don't think mdcat needs to squeeze all the juice out of the CPU, I think it would perform fine also without using this flag.

Just to be clear, I don't want to push you into doing that. I'm just thinking out loud.

But I'd happily consider a pull request which splits out the library code, i.e. lib.rs and downwards, into a separate say pulldown-cmark-tty crate or so, and sets up all the cargo workspace stuff, also for Github actions and cargo release. I might do this myself at some point, but it will take a while, since mdcat is a very side project for me.

I totally get it. I can set aside some time to provide a PR that does these changes. Given this is a big change, I wanted to be sure you would be fine with this idea before starting to implement that. It's never good to show up with a big amount of invasive changes without announcing yourself :)

I'm sorry for being blunt, more so, if my guestimate about kubewarden is entirely wrong innocent but I think it's important to clarify what you can expect from me (nothing really innocent ) if you're using mdcat in a project at enterprise level.

Again, I totally get it. I do have some spare time open source projects too, I feel your pain and I understand your concerns.

However, in our case the usage of mdcat is well defined and definitely not security critical. Just to give you the full picture, we use mdcat to render the Markdown README that is embedded into our policies.

swsnr commented 1 year ago

Thanks for providing some background on the effect of this feature. I think you're right; mdcat doesn't need it.

I've pushed a commit which removes the feature, and another one which uses default-features = false for all dependencies, just to avoid another feature getting in your way.

I'll make a new release over the next days.

Again, I totally get it. I do have some spare time open source projects too, I feel your pain and I understand your concerns.

Thanks for understanding ā¤ļø

flavio commented 1 year ago

I've pushed a commit which removes the feature, and another one which uses default-features = false for all dependencies, just to avoid another feature getting in your way.

I'll make a new release over the next days.

Wow, thanks a lot!

swsnr commented 1 year ago

1.1.1 is out.

swsnr commented 1 year ago

@flavio Just wanted to make you aware that I've split out the rendering code into a new pulldown-cmark-mdcat crate, and moved out a bunch of heavy dependencies (reqwest) and guarded other heavy dependencies behind features (resvg, image).

You'll need to replace the mdcat with pulldown-cmark-mdcat, and building with default-features = false, features = ["regex-fancy"] should give you a considerably lighter build (at the cost of reduced image support, no SVG support, and no HTTP support for images).

I'll release this a 2.0.0 soonish.

flavio commented 1 year ago

Thanks! I'll wait for mdcat 2.0.0 to be out, so that I can learn how to use the new crate by looking at its source code

swsnr commented 1 year ago

It's out already.