Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move rust-mozjs's build script to mozjs-sys #409

Merged
merged 4 commits into from
Dec 29, 2023
Merged

Move rust-mozjs's build script to mozjs-sys #409

merged 4 commits into from
Dec 29, 2023

Conversation

wusyong
Copy link
Contributor

@wusyong wusyong commented Sep 18, 2023

Move all build scripts into mozjs-sys crate which is more conventional when binding FFI.
This can also make possible future changes like linking binary build instead of building everything from source easier.

I added another commit to rename original mozjs-sys's jsglue.cpp to jsapi.cpp, and kept rust-mozjs's jsglue.
It's just clearer IMHO. Let me know if there are better names.

@wusyong wusyong changed the title Move rust-mozjs's build scirpt to mozjs-sys Move rust-mozjs's build scirpt to mozjs-sys Sep 18, 2023
@sagudev
Copy link
Member

sagudev commented Sep 18, 2023

On one hand jsglue requires headers from mozjs_sys so it might make sense move it there, but on the other hand jsglue provide high level wrappers, that are tightly coupled with stuff in rust-mozjs so it also makes sense to put it there. Moving also violates idea of raw bindings in sys crate ...

I am pretty diverged about this, but I am slightly more in favor to keep things as they are.

So I would love to hear what others think about this and if they are any other reasons for/against this.

@wusyong
Copy link
Contributor Author

wusyong commented Sep 18, 2023

If it's higher level wrappers, it should be implemented by Rust using sys crate directly.
Having another build script to compile and bindgen C++ code makes the structure a bit more complicated.
I'm okay if this module should stay in rust-mozjs, but I think it's better to rewrite in Rust.

This also move libjsapi.a to the same place as jsapi.rs in $OUT_DIR/build
@wusyong
Copy link
Contributor Author

wusyong commented Dec 13, 2023

@sagudev Is it possible to push this PR again? I've rebased the PR to latest main branch.
It will still be the same files but they will be inside the same OUT_DIR at least.
This should make #439 easier.

@mrobinson mrobinson changed the title Move rust-mozjs's build scirpt to mozjs-sys Move rust-mozjs's build script to mozjs-sys Dec 13, 2023
@sagudev
Copy link
Member

sagudev commented Dec 27, 2023

The more I think about it the more I like it.

To sum up here is how new structure looks like (currently jsapi contains some higher stuff too):

  • jsapi.cpp is low level (includes headers for bindings, contains replacement types).
  • jsglue.cpp is place for higher level glue (mozilla::maybe wrappers, all other stuff that is not usable via bindgen directly due to cxx).

@sagudev
Copy link
Member

sagudev commented Dec 27, 2023

@jdm what do you think about this?

@Redfire75369
Copy link
Contributor

Redfire75369 commented Dec 28, 2023

I'm not arguing for either option, but I will note that the current split between mozjs-sys's jsglue.cpp and mozjs's jsglue.cpp has no consistent rhyme or reason, some cleanup would be required there regardless of the outcome.

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.

This seems like a reasonable choice!

@sagudev sagudev mentioned this pull request Dec 29, 2023
3 tasks
@sagudev sagudev added this pull request to the merge queue Dec 29, 2023
Merged via the queue into servo:main with commit 7184f65 Dec 29, 2023
13 checks passed
@wusyong
Copy link
Contributor Author

wusyong commented Jan 1, 2024

Thanks for merging!

@delan delan mentioned this pull request Jan 18, 2024
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants