Skip to content

/ rust-mozjs

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

Introduce feature to only allow initialization once. #447

Merged
merged 1 commit into from Nov 14, 2018
Merged

Introduce feature to only allow initialization once. #447

merged 1 commit into from Nov 14, 2018
+11 −4

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Nov 14, 2018 •
edited by larsbergstrom

See #22039

This change is Reviewable
@paulrouget
Introduce feature to only allow initialisation once.
Loading status checks…
2ccdca9
@jdm
Copy link
Member

jdm commented Nov 14, 2018

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 14, 2018

📌 Commit 2ccdca9 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Nov 14, 2018

⌛ Testing commit 2ccdca9 with merge 5baf536...
bors-servo added a commit that referenced this pull request Nov 14, 2018
@bors-servo
Auto merge of #447 - paulrouget:initonce, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5baf536 
Introduce feature to only allow initialization once.

See #22039

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/447)
<!-- Reviewable:end -->
@CYBAI
Copy link
Contributor

CYBAI commented Nov 14, 2018

servo/servo#22039

(Just help to reference the issue correctly)
@CYBAI
CYBAI reviewed Nov 14, 2018
View changes
Cargo.toml
@@ -2,7 +2,7 @@
name = "mozjs"
description = "Rust bindings to the Mozilla SpiderMonkey JavaScript engine."
repository = "https://github.com/servo/rust-mozjs"
version = "0.9.3"
version = "0.9.4"

This comment has been minimized.

Sign in to view
@CYBAI

CYBAI Nov 14, 2018

Contributor

One thing I'm confused is that I've bumped the version to 0.9.4 at c16e7e2 but GitHub didn't show it 🤔

rust-mozjs/Cargo.toml

Line 5 in c16e7e2

version = "0.9.4"

@bors-servo
Copy link
Contributor

bors-servo commented Nov 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 5baf536 to master...
@bors-servo bors-servo merged commit 2ccdca9 into servo:master Nov 14, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@paulrouget
Copy link
Contributor Author

paulrouget commented Nov 14, 2018

@CYBAI I can't see the commits from PR #445
@CYBAI
Copy link
Contributor

CYBAI commented Nov 14, 2018

Hmm, @paulrouget can you see 9d5d8a9 in the PR?

Btw, mozjs on crates.io is already on 0.9.4 (https://crates.io/crates/mozjs). Not sure why GitHub didn't show the commit 🤔
@paulrouget
Copy link
Contributor Author

paulrouget commented Nov 14, 2018

Can't see the commit here: https://github.com/servo/rust-mozjs/commits/master/Cargo.toml but I see it here: https://github.com/servo/rust-mozjs/commits/master

Anyway. Gonna bump to 0.9.5 and publish.
@nox
Copy link
Member

nox commented Dec 8, 2018

Why not use std::sync::Once, and why is this feature only needed on Android in Servo? AFAIK SM should never be initialised multiple times, on all platforms, so I don't understand why this behaviour is enabled by a feature.
@nox
Copy link
Member

nox commented Dec 8, 2018

Cc @asajeffrey, who reverted most of 066b4ab when he updated to SM 60 in #430.
@nox
Copy link
Member

nox commented Dec 8, 2018

From SpiderMonkey's own documentation:

It is currently not possible to initialize SpiderMonkey multiple times (that is, calling JS_Init, JSAPI methods, then JS_ShutDown in that order, then doing so again). This restriction may eventually be lifted.

JS_Init should never be called multiple times, so this shouldn't be a feature at all.
@jdm
Copy link
Member

jdm commented Dec 8, 2018

It is specific to the way that we're using Servo inside Firefox Reality. The feature simply prevents us from calling shut down; it doesn't actually change how many times we initialize the library.
@asajeffrey
Copy link
Member

asajeffrey commented Dec 10, 2018

Hmm, might be worth finding another name for the feature? It seems to be more about shutdown than init, perhaps never_shutdown would be better?
@jdm
Copy link
Member

jdm commented Dec 10, 2018

I have an open pull request that removes it entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@paulrouget @jdm @bors-servo @CYBAI @nox @asajeffrey
You can’t perform that action at this time.