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

jeroenvisser101 · 2021-06-16T22:37:49Z

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

This also adds GitHub actions, which should replace Travis.

jeroenvisser101 · 2021-06-16T22:41:39Z

native/meeseeks_html5ever_nif/src/lib.rs

@@ -126,20 +125,15 @@ enum ParserType {
    XmlDocument,
 }

-fn parse<'a>(parser_type: ParserType, env: Env<'a>, args: &[Term<'a>]) -> NifResult<Term<'a>> {
+fn parse<'a>(parser_type: ParserType, env: Env<'a>, document: Binary) -> NifResult<Term<'a>> {


This change seems to work fine, tests pass, but it drops the previous panic, for what I guess will be a rustler specific error?

Can you explain to me the change in signature?

~~Is that a change in Rustler v0.22? Can you point me to the appropriate documentation there if it is?~~

It was changed, but to be honest, the documentation is either hidden or not there, as the only way I figured out about this is by looking at others who had already updated.

The error without it is the one I mentioned before:

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

I'm very new to Rust, and don't really have a clue as to why this change is needed. I'm not sure if there's a way to keep Term and get it to work with Decoder, but if there is then maybe we can do that instead. No preference on my end here, just trying to get it to compile (which it does 😄)

That something called "args", the first element of which was the document that needed parsing, is getting replaced by the document that is being parsed makes me think there is going to be a mismatch there.

I will need time to review Rustler's changes and figure out what's going on and how best to address that issue.

This (and some other files in that PR) are the only references that change this as well, but I couldn't really find it in the changelog, but I may have missed it.

Ok, I confirmed that your approach (replacing the slice of terms that was args with the actual arg, in this case document) is the correct one.

That was the approach suggested here by @evnu.

I'm very new to Rust, and don't really have a clue as to why this change is needed.

The rustler changelog actually mentions this, but it is possibly too opaque.

NIFs can be derived from regular functions, if the arguments implement Decoder and the return type implements Encoder:

Before the upcoming rustler, args was a slice of Term which needed to be decoded. Each entry in args corresponds with one argument passed into the function. Now with rustler v0.22, the arguments are spelled out explicitly, just like in a regular function.

Do you have a suggestion to make this more obvious for the users of rustler? Where would you expect that information?

@evnu maybe some examples of the before and after could help. I figured this out by comparing libraries that had already upgraded from those that had not. Maybe UPGRADING.md file?

Maybe the changelog entry was fine, but like I said, I'm not familiar with rustler or rust, so someone else may have read Encoder/Decoder and knew immediately that those sliced params (?) would be replaced with it.

@evnu I saw the example in the changelog under the "NIFs can be derived from regular functions..." note but I wasn't sure if it related to this because the signatures were so different:

add<'a>(env: Env<'a>, args: &[Term<'a>]) -> Result<Term<'a>, Error>

vs

fn add(a: i64, b: i64) -> i64

An example that just showed args being replaced by each individual arg would have been clearer.

Maybe UPGRADING.md file?

That's a good idea, thanks!

jeroenvisser101 · 2021-06-16T22:45:21Z

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 · 2021-06-16T22:46:11Z

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 · 2021-06-16T22:52:12Z

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

As suggested in rusterlium/rustler#141

mischov · 2021-06-17T06:21:24Z

native/meeseeks_html5ever_nif/src/lib.rs


-fn on_load<'a>(_env: Env<'a>, _load_info: Term<'a>) -> bool {
+fn load(_env: Env, _load_info: Term) -> bool {


Curious what prompted the change from on_load to load?

Ah this was honestly only to align with what rustler.new would generate if ran today, it's not a required change

mischov · 2021-06-17T06:27:45Z

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 · 2021-06-17T12:57:28Z

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: rusterlium/rustler#326. I think this fix wasn't in this RC yet, so we should be able to revert it later on, I think

mischov · 2021-06-17T15:31:17Z

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

jeroenvisser101 · 2021-06-17T15:45:18Z

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 · 2021-06-17T15:52:16Z

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 · 2021-06-17T16:03:04Z

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

Thank you again.

jeroenvisser101 · 2021-06-18T01:33:01Z

We deployed it and it worked fine :)

Thanks for the help and your time 👍

jeroenvisser101 commented Jun 16, 2021

View reviewed changes

Update library implementation for rustler v0.22

36ccbd0

jeroenvisser101 force-pushed the jv-rustler-v0.22 branch from cb23760 to e1e1205 Compare June 16, 2021 22:42

jeroenvisser101 force-pushed the jv-rustler-v0.22 branch from e1e1205 to 36ccbd0 Compare June 16, 2021 22:51

Use cdylib crate type

3915843

As suggested in rusterlium/rustler#141

mischov mentioned this pull request Jun 17, 2021

Move to GitHub Actions #38

Merged

mischov reviewed Jun 17, 2021

View reviewed changes

Try to fix issue with compilation

fbe1a79

jeroenvisser101 mentioned this pull request Jun 17, 2021

Update rustler to v0.22.0-rc.1 mjaric/tds-encoding#1

Closed

mischov marked this pull request as ready for review June 17, 2021 15:55

mischov merged commit 6a6f10e into mischov:master Jun 17, 2021

mischov mentioned this pull request Jun 22, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jeroenvisser101 commented Jun 16, 2021

jeroenvisser101 Jun 16, 2021

mischov Jun 16, 2021 •

edited

Loading

jeroenvisser101 Jun 16, 2021 •

edited

Loading

mischov Jun 16, 2021

jeroenvisser101 Jun 16, 2021

mischov Jun 17, 2021

evnu Jun 17, 2021

jeroenvisser101 Jun 17, 2021

mischov Jun 17, 2021

evnu Jun 17, 2021

jeroenvisser101 commented Jun 16, 2021

mischov commented Jun 16, 2021

jeroenvisser101 commented Jun 16, 2021

mischov Jun 17, 2021

jeroenvisser101 Jun 17, 2021

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 17, 2021

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 17, 2021

mischov commented Jun 17, 2021 •

edited

Loading

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 18, 2021


		fn on_load<'a>(_env: Env<'a>, _load_info: Term<'a>) -> bool {
		fn load(_env: Env, _load_info: Term) -> bool {

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

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

Conversation

jeroenvisser101 commented Jun 16, 2021

Choose a reason for hiding this comment

mischov Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

jeroenvisser101 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeroenvisser101 commented Jun 16, 2021

mischov commented Jun 16, 2021

jeroenvisser101 commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 17, 2021

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 17, 2021

mischov commented Jun 17, 2021 • edited Loading

mischov commented Jun 17, 2021

jeroenvisser101 commented Jun 18, 2021

mischov Jun 16, 2021 •

edited

Loading

jeroenvisser101 Jun 16, 2021 •

edited

Loading

mischov commented Jun 17, 2021 •

edited

Loading