Skip to content
This repository has been archived by the owner on Nov 12, 2022. It is now read-only.

Clean up the glue #484

Open
nox opened this issue Oct 11, 2019 · 8 comments
Open

Clean up the glue #484

nox opened this issue Oct 11, 2019 · 8 comments

Comments

@nox
Copy link
Contributor

nox commented Oct 11, 2019

So this crate has a jsglue.cpp file, yet another one, with more stuff than the one in servo/mozjs, but mostly duplicated stuff. It has corresponding bindings in glue.rs, but this file is not generated by bindgen during the build, it's just sitting there in the repo, never regenerated, so it may not even be in sync anymore. The instructions to build it are a comment with hardcoded paths in the otherwise 8 years old file gen.py.

We then wrap those with additional Rust methods, through macro calls generated in glue_wrappers.in (and jsapi_wrappers.in because, why not?). Those files are generated by the shell script generate_wrappers.sh, which is just a sed script in disguise.

Those two .in files aren't generated during build-time either, and the jsapi one is generated from the generated bindings from servo/mozjs_sys, but the script is hardcoded to read the debug bindings, so who knows if that's correct when building in release mode…

How did we let things become that out of hand? What is actually relevant, and what is not?

@nox
Copy link
Contributor Author

nox commented Oct 11, 2019

Cc @jdm @asajeffrey

@nox nox transferred this issue from servo/mozjs Oct 11, 2019
@asajeffrey
Copy link
Member

Well the big picture "why have a glue.cpp file at all" is because bindgen has a hard time with C++ APIs, especially on windows where the ABI is 🤷‍♂️. Since jsapi is very keen on C++ idioms we then have to put them back into C-like function calls that bindgen is happier with.

That said, the split between the mozjs_sys crate and the mozjs crate is rather arbitrary, and probably in need of a spring clean.

IIRC the .in files are there to bridge between mozjs_sys's Handle (which doesn't take a lifetime) and mozjs's (which does). Yes, those files are checked in, which is odd since we don't check in the result of running bindgen.

So yes, it looks like we have let some technical debt accumulate here.

@nox
Copy link
Contributor Author

nox commented Oct 11, 2019

IIRC the .in files are there to bridge between mozjs_sys's Handle (which doesn't take a lifetime) and mozjs's (which does). Yes, those files are checked in, which is odd since we don't check in the result of running bindgen.

I feel like this script using grep and sed and generating macro calls is a failed experiment, and the technical debt wouldn't exist if we finally reached the conclusion that those bridges and safe wrappers (e.g. those additional lifetimes) just cannot be automated.

Would it really be that horrible if we just wrote such safe wrappers on demand, whenever we need a new API, rather than trying to mechanise that stuff weirdly?

@jdm
Copy link
Member

jdm commented Oct 11, 2019

My experience updating spidermonkey with the checked in wrapper files is that the compiler complains when there are invalid wrappers, so there is not actually a risk of drift.

@asajeffrey
Copy link
Member

Do you mean get rid of the macros, or get rid of the shell script?

@nox
Copy link
Contributor Author

nox commented Oct 11, 2019

My experience updating spidermonkey with the checked in wrapper files is that the compiler complains when there are invalid wrappers, so there is not actually a risk of drift.

But those wrappers aren't actually idiomatic, so if we actually wanted to have bindings that don't lead to a miserable experience, we would still need to write code by hand, at which point why do this weird grep/sed stuff?

I guess a good compromise would be to retcon the .in files as if they had always been written by hand, delete the shell script, and add future wrappers by hand in there?

@nox
Copy link
Contributor Author

nox commented Feb 11, 2020

Doing the smup, I realise that all these wrappers are hindering me in CodegenRust.py, they are not helping.

@nox
Copy link
Contributor Author

nox commented Feb 11, 2020

Like, wrappers for JS_DefineProperty, on one side we need to wrap RawHandleObject as HandleObject, and on the other we need to unwrap SafeJSContext as *mut JSContext, I do not like this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants