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

Add crate docstring and two code examples #489

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Add crate docstring and two code examples #489

merged 1 commit into from
Feb 24, 2020

Conversation

jhwgh1968
Copy link
Contributor

@jhwgh1968 jhwgh1968 commented Dec 1, 2019

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
Copy link
Contributor

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

@jhwgh1968
Copy link
Contributor Author

jhwgh1968 commented Jan 18, 2020

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 jhwgh1968 changed the title [WIP] Add crate docstring and example Add crate docstring and example Jan 18, 2020
@jhwgh1968
Copy link
Contributor Author

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

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@jhwgh1968
Copy link
Contributor Author

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 jhwgh1968 requested a review from jdm February 14, 2020 00:21
@jhwgh1968
Copy link
Contributor Author

(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.)

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is definitely an improvement over the status quo.

examples/eval.rs Outdated Show resolved Hide resolved
examples/minimal.rs Show resolved Hide resolved
@jhwgh1968
Copy link
Contributor Author

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 jhwgh1968 changed the title Add crate docstring and example Add crate docstring and two code examples Feb 15, 2020
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that intentionally difficult is really a goal; it's a reflection of writing just enough Rust bindings to wrap some of the most basic or tricky parts of the SpiderMonkey API. In any case I don't see how we can make a better example, because we can't do anything with spidermonkey without creating a global object!

examples/eval.rs Outdated Show resolved Hide resolved
examples/minimal.rs Outdated Show resolved Hide resolved
@jhwgh1968
Copy link
Contributor Author

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

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

@jhwgh1968
Copy link
Contributor Author

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
Copy link
Member

jdm commented Feb 24, 2020

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

@jdm
Copy link
Member

jdm commented Feb 24, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 58bc43c has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 58bc43c with merge c1e5ad8...

@bors-servo
Copy link
Contributor

💔 Test failed - checks-travis

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis, status-appveyor
Approved by: jdm
Pushing c1e5ad8 to master...

@bors-servo bors-servo merged commit c1e5ad8 into servo:master Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants