mischov / meeseeks_html5ever

Meeseeks-specific NIF binding of html5ever using Rustler.
Apache License 2.0
10 stars 15 forks source link

Update library implementation for rustler v0.22 (contd.) #37

Closed jeroenvisser101 closed 3 years ago

jeroenvisser101 commented 3 years ago

Continuation of #35, fixes issues related to a botched upgrade, and completes the upgrade.

This also adds GitHub actions, which should replace Travis.

jeroenvisser101 commented 3 years ago

This should fix #36, but the implementation there is a bit more advanced. If you want me to add caching and move formatting check to a different job, I'm happy to do so :)

mischov commented 3 years ago

Sorry, but could I get you to remove the Github Actions bits. That's work that should be in a separate branch off master with its own PR.

I am in the process of adding the Github Actions stuff myself and I'd like to do it incrementally (remove travis, add workflow similar to what travis last tested, upgrade the versions tested). Appreciate you trying to save me work, though.

jeroenvisser101 commented 3 years ago

For sure, no worries, I rebased and dropped the commit with Github actions. If you want me to add it in a separate PR anyway, let me know, happy to help

mischov commented 3 years ago

With the exception of that load vs on_load stuff (and I'm mostly just curious what prompted you to make that change) everything looks good to me.

I do want to take a moment and thank you for figuring all this stuff out- from what I've seen not a lot of libraries have moved to the Rustler v0.22 stuff yet so it looks like you were blazing a bit of a trail there.

jeroenvisser101 commented 3 years ago

I do want to take a moment and thank you for figuring all this stuff out- from what I've seen not a lot of libraries have moved to the Rustler v0.22 stuff yet so it looks like you were blazing a bit of a trail there.

Thanks 👍 I've only found 3 or so other packages that have updated, and those were very helpful to get a feel for what was changed.

I made 2 additional changes, one is to include the priv folder, which was required to get it to build in CI/docker: https://github.com/rusterlium/rustler/issues/326. I think this fix wasn't in this RC yet, so we should be able to revert it later on, I think

mischov commented 3 years ago

@jeroenvisser101 Do you think this is ready to merge or are you still testing?

jeroenvisser101 commented 3 years ago

I think it's ready, but the change from dylib to cdylib may not have been needed, although it is now the default. We're planning to deploy this to prod later today, but we've tested our use of meeseeks and all is working correctly.

mischov commented 3 years ago

I think the change to cdylib is ok.

The Rust docs say this about cdylib:

This is used when compiling a dynamic library to be loaded from another language.

That sounds a lot like our use case, though I'm not sure if it comes with any downsides.

mischov commented 3 years ago

Let me know if any problems pop up once you deploy to prod.

Thank you again.

jeroenvisser101 commented 3 years ago

We deployed it and it worked fine :)

Thanks for the help and your time 👍