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

Allow for C-style dynamic libraries to be produced #171

Merged
merged 10 commits into from Jul 6, 2022
Merged

Allow for C-style dynamic libraries to be produced #171

merged 10 commits into from Jul 6, 2022

Conversation

gjtorikian
Copy link
Collaborator

I'd like to be able to use comrak for commonmarker, but before doing that this project needs to be able to generate a C-style dynamic library. This PR adds the crate-type necessary to do that.

@gjtorikian gjtorikian marked this pull request as draft December 18, 2020 14:19
@gjtorikian
Copy link
Collaborator Author

Actually some methods need to be exposed still (extern "C"), so consider this WIP.

@gjtorikian
Copy link
Collaborator Author

gjtorikian commented Dec 18, 2020

Before I continue I should ask @kivikakk whether this is even a desired change.

The main function is easy enough to expose, roughly:

#[no_mangle]
#[allow(unused)]
pub extern "C" fn c_markdown_to_html(md_char: *const c_char, options: &ComrakOptions) -> *const c_char {
    // Convert C string to Rust string
    let c_md = unsafe {
        assert!(!md_char.is_null());
        CStr::from_ptr(md_char)
    };
    let md = c_md.to_str().unwrap();

    let arena = Arena::new();
    let root = parse_document(&arena, md, options);
    let mut s = Vec::new();
    format_html(root, options, &mut s).unwrap();
    unsafe {
        CString::from_vec_unchecked(s).as_ptr()
    }
}

But I would need all the functions the current C library supports, including modifying nodes (append, prepend) and getting/setting attributes (fence_info, header_level=). Would you be okay with me exposing all of that within the lib?

@kivikakk
Copy link
Owner

Would you be okay with me exposing all of that within the lib?

Yes, absolutely! Thank you for starting this work. If we need to rejig some internals slightly for that to happen, that'd be fine too—it'd be lovely to have this usable through a C interface.

Some simple test harnesses around it will be necessary, but that could even be literally running the CommonMark spec through commonmarker with the Comrak backend.

@gjtorikian
Copy link
Collaborator Author

@kivikakk 👋 Hello!!! 👋 I'm back to finish this PR. 😆

In the time since I opened this PR, I've become marginally better at Rust FFI handling, so I've rewritten whatever garbage was previously here with some new thoughts.

I've added here a little Rust lib that exposes a few methods, defined in comrak.h. I've also added tests (via picotest) which show a proposal for how this API can be used. Happy to change any of these names/arguments/whatever—just dropping a basic outline of what I'm thinking about.

To be honest, the last time I tried doing this, I got horribly stuck on memory handling when exposing the Arena of nodes. If possible, I'd like to scope this PR to just basic option handling and direct Commonmark -> HTML rendering.

@kivikakk
Copy link
Owner

Hey Garen, so lovely to hear from you again 😊 This is awesome. I gave it a run (after overcoming my brief confusion at trying to cargo test the c-api stuff /o\) — it looks great, and the direction seems 💯. Very happy for this to go ahead! (And the smaller scope for this PR sounds good 👍)

@gjtorikian gjtorikian marked this pull request as ready for review July 5, 2022 21:48
@gjtorikian
Copy link
Collaborator Author

@kivikakk 👋 There are three tests failing oddly, which I will definitely address, but I believe this PR is ready for review in all other places!

@kivikakk
Copy link
Owner

kivikakk commented Jul 5, 2022

This is looking awesome. Just from a quick look, a stray </table> at the end of one, and I think the --width option only applies when outputting CommonMark. (On my way to an appointment, will fix up myself if you don't by the time I'm back!)

@kivikakk
Copy link
Owner

kivikakk commented Jul 6, 2022

I've pushed 0da84ed now 43382b2, which hopefully gets us all green on CI! ❤️

@kivikakk
Copy link
Owner

kivikakk commented Jul 6, 2022

Rebased onto main. If you like the look of it as is, please feel free to merge! 🚀

@gjtorikian
Copy link
Collaborator Author

Thank you! It looks great. 😄

@gjtorikian gjtorikian merged commit fb65615 into kivikakk:main Jul 6, 2022
@gjtorikian gjtorikian deleted the specify-lib branch July 6, 2022 13:51
@kivikakk
Copy link
Owner

kivikakk commented Jul 7, 2022

Awesome! Thank you so much & nicely done!

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

2 participants