mischov / meeseeks_html5ever

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

Add support for OTP 24, upgrade rustler to v0.22 #35

Closed jeroenvisser101 closed 3 years ago

jeroenvisser101 commented 3 years ago

This upgrades rustler to v0.22.0-rc.1 to allow compilation under OTP 24.

Also addresses #34.

mischov commented 3 years ago

Do you know what the release timeline is for v0.22? I generally won't use a release candidate as a dependency.

jeroenvisser101 commented 3 years ago

Not at all, from what I've read, the original author is a bit busy right now. We could release this as an -rc.1 too, and upgrade accordingly?

mischov commented 3 years ago

I appreciate your assistance getting Rustler upgraded and the fix for #34 in, I will merge this as is.

I asked about the timeline for release in the issue you linked to, and I'll wait on making any decision about releasing until I hear back there. Generally speaking I am against releasing as an rc because then I might also need to release Meeseeks as an rc, etc, etc.

In the meantime I think you'll be able to override the version of this library Meeseeks uses with a github dependency in your mix.exs.

That should look something like:

{:meeseeks_html5ever, git: "https://github.com/mischov/meeseeks_html5ever.git", ref: "...", override: true}

I don't use git dependencies a lot so something might not work with that example.

jeroenvisser101 commented 3 years ago

@mischov thanks, although I wasn't completely ready with it yet. I seem to have upgraded the wrong dependency, and while it works if I update the correct one, it throws more warnings (rustler_atoms! and init! was changed too), but when changing these, I ran into another issue that is a bit outside of my very limited rust knowledge:

error[E0277]: the trait bound `&[rustler::Term<'a>]: Decoder<'_>` is not satisfied
   --> src/lib.rs:177:1
    |
177 | #[rustler::nif(schedule = "DirtyCpu")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Decoder<'_>` is not implemented for `&[rustler::Term<'a>]`
    |
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `&[rustler::Term<'a>]: Decoder<'_>` is not satisfied
   --> src/lib.rs:182:1
    |
182 | #[rustler::nif(schedule = "DirtyCpu")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Decoder<'_>` is not implemented for `&[rustler::Term<'a>]`
    |
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

https://github.com/mediapress-ltd/meeseeks_html5ever/commit/ee36785d7e28be55daf6583ccbfb62deb7a026ee

jeroenvisser101 commented 3 years ago

My best guess is that this is what the commented-out section "term_to_configs" does, but I'm not sure, any chance you can push me in the right direction?

jeroenvisser101 commented 3 years ago

I don't use git dependencies a lot so something might not work with that example.

We do, and you can actually reference a github repo too: github: "owner/repo", branch: "...", it works quite well :)

mischov commented 3 years ago

My bad. I should have tested. This also highlights that Travis isn't doing anything and I need to carry through on the upgrade to github actions.

I would, however, suggest either not opening the PR before the work is ready or marking it with a [WIP] tag in the title and/or description if you have to create it for some reason.

jeroenvisser101 commented 3 years ago

I would, however, suggest either not opening the PR before the work is ready or marking it with a [WIP] tag in the title and/or description if you have to create it for some reason.

Yeah my bad, I should've opened it as a draft. Thing is, tests were passing, weirdly enough, maybe because it didn't trigger a recompile?

Anyway, I think I've fixed it, let me do a couple more tests and I'll open a new PR. Sorry for the hassle!

jeroenvisser101 commented 3 years ago

@mischov I've added GitHub actions, will open a new PR if that's ok :)

mischov commented 3 years ago

@acru-jdp It sounds like you're on the wrong commit maybe, PR #37 fixed the Rustler version as you described.

https://github.com/mischov/meeseeks_html5ever/blob/6a6f10ec80a8cdaab0044938031c30db296ed2bc/native/meeseeks_html5ever_nif/Cargo.toml#L12-L14

acru-jdp commented 3 years ago

Hi

Yes, I realised that immediately after I had posted my comment. I pointed to the subsequent version and it worked fine.

Regards

John

On Tue, 22 Jun 2021, 18:24 Mischov, @.***> wrote:

@acru-jdp https://github.com/acru-jdp It sounds like you're on the wrong commit maybe, PR #37 https://github.com/mischov/meeseeks_html5ever/pull/37 fixed the Rustler version as you described.

https://github.com/mischov/meeseeks_html5ever/blob/6a6f10ec80a8cdaab0044938031c30db296ed2bc/native/meeseeks_html5ever_nif/Cargo.toml#L12-L14

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mischov/meeseeks_html5ever/pull/35#issuecomment-866182513, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANNHBMJ7GNZRU6XXCMJBZUDTUDBLPANCNFSM462H4KLA .