servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Add crate docstring and two code examples #489

Closed jhwgh1968 closed 4 years ago

jhwgh1968 commented 4 years ago

In the hopes of embedding SpiderMonkey into a non-Servo Rust project, I spent some time digging into these bindings. Unfortunately, it was very easy to get lost.

This PR is my attempt to make it easier for the next person who attempts my exercise by adding a few sign posts I would have found helpful, and translating one of the basic examples from the SpiderMonkey User Guide on MDN to Rust. Documenting the entire crate (and where it dips directly into mozjs_sys) is a task too big for any one person, I think.

EDIT: removed WIP comment

bors-servo commented 4 years ago

:umbrella: The latest upstream changes (presumably #493) made this pull request unmergeable. Please resolve the merge conflicts.

jhwgh1968 commented 4 years ago

I've fought with this quite a bit, but it's really a struggle. I can't figure out how to make a global object (and a null pointer anywhere results in a crash), and the API is really hard to use. My attempts to figure out how Servo uses this library -- the only working code I know of -- seem futile.

Surely I am looking at this API upside down and this is not as difficult as it seems. Any guidance on offer? What am I missing?

EDIT: I was actually quite close, according to #481. Let me see if I can finish it...

jhwgh1968 commented 4 years ago

It's still a bit kludgy, I think, but should work and demonstrate the basics of the API pretty well. Ready for review.

jhwgh1968 commented 4 years ago

Thanks, @jdm. I didn't know CI didn't run doc tests. Given that, I agree that examples make sense.

I could not figure out how to make the examples get their own doc pages, so did my best to generate links to the source files in the crate root, since I feel those should be mentioned. If you know of a trick or prefer I remove them, let me know.

jhwgh1968 commented 4 years ago

(Note to self: there is a request review button at the bottom, when GitHub doesn't detect that I have fixed all the changes sometimes.)

jhwgh1968 commented 4 years ago

Thanks, @jdm! I have fixed one of the comments, but the other one tells me that I am missing something. I have two questions.

The line you commented on minimal.rs was a bit of a mess. However, that mess was trying to copy from the C++ example, which was specifically showing how to expose a global to SpiderMonkey. I used the rooted! macro combined with JS_NewGlobalObject basically out of trial and error. I could not find anything else that would work in RustDoc.

I presume creating a global like this in Rust is intentionally difficult, because Rust wants in-depth knowledge of lifetimes. Thus, my first question: does this mean that is a bad example for Rust? Or is there a better way to do this I missed?

Regarding eval.rs, I could not remove the unsafe block even after implementing your suggestion. That is because the same JS_NewGlobalObject construction is in there, too, and the global variable would not live long enough otherwise.

Why is it is there? Because I read a "global root object" is used as a compartment -- at least, in the C++ version. In Rust, I only know that if I mess up that object creation, lots of ugly UB happens.

That is my second question: does that need to be in eval.rs at all? A lot of the other code like AutoRequest is dropped in the translation to Rust, so can that go out as well and I just missed it?

Thanks for your patience!

jhwgh1968 commented 4 years ago

Thanks for the pointers and comments, @jdm! All fixed!

I also ran Rustfmt on it to make sure it's nice and clean. :sparkles:

jhwgh1968 commented 4 years ago

Uh... the AppVeyor run seems to have a broken mozjs_sys on win32:

: failed to run custom build command for `mozjs_sys v0.67.1 (https://github.com/servo/mozjs?rev=bc4935b668171863e537d3fb0d911001a6742013#bc4935b6)`
Caused by:
  process didn't exit successfully: `C:\projects\rust-mozjs\target\debug\build\mozjs_sys-0c301899e9ba10f4\build-script-build` (exit code: 101)
--- stdout
cargo:outdir=C:\projects\rust-mozjs\target\i686-pc-windows-msvc\debug\build\mozjs_sys-66162aad09d2f7b9\out\build
[[ 'C:\Users\appveyor\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\bc4935b\mozjs'/js/src/configure -ot 'C:\Users\appveyor\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\bc4935b\mozjs'/js/src/configure.in ]] && touch 'C:\Users\appveyor\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\bc4935b\mozjs'/js/src/configure || true
C:\Users\appveyor\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\bc4935b\makefile.cargo:191: recipe for target 'maybe-configure' failed
--- stderr
      0 [main] us 0 init_cheap: VirtualAlloc pointer is null, Win32 error 487
AllocationBase 0x0, BaseAddress 0x607A0000, RegionSize 0x320000, State 0x10000
C:\mozilla-build\msys\bin\sh.exe: *** Couldn't reserve space for cygwin's heap, Win32 error 0
mozmake: *** [maybe-configure] Error 1
thread 'main' panicked at 'assertion failed: result.success()', C:\Users\appveyor\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\bc4935b\build.rs:167:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

I can't imagine it's related to my simple change...

jdm commented 4 years ago

Yeah, that's a known intermittent error that occurs.

jdm commented 4 years ago

@bors-servo r+

bors-servo commented 4 years ago

:pushpin: Commit 58bc43c has been approved by jdm

bors-servo commented 4 years ago

:hourglass: Testing commit 58bc43cc72961fb5a26c1d3750aee6c50d067ced with merge c1e5ad8de9f4f2c4c09eb033c40ebf1020b107fa...

bors-servo commented 4 years ago

:broken_heart: Test failed - checks-travis

bors-servo commented 4 years ago

:sunny: Test successful - checks-travis, status-appveyor Approved by: jdm Pushing c1e5ad8de9f4f2c4c09eb033c40ebf1020b107fa to master...