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

Search for .mem in same directory as Node script #4936

Closed
wants to merge 1 commit into from

Conversation

RReverser
Copy link
Collaborator

This seems to be a common source of issues when trying to run optimized code with separate .js.mem from Node.js, while not being in the same working directory (in particular, got hit by this nyself while implementing rust-lang/rust#39490).

It was possible to workaround this and set search path using Module.memoryInitializerPrefixURL since 7e9e24, but I think it makes sense to have a nice default for 90% of cases too, which would be to search for .js.mem file in the same directory as .js itself.

This seems to be a common source of issues when trying to run optimized code with separate .js.mem from Node.js, while not being in the same working directory.

It was possible to workaround this and set search path using memoryInitializerPrefixURL since 7e9e24, but I think it makes sense to have a nice default for 90% of cases too, which would be to search for .js.mem file in the same directory as .js itself when running using Node.js.
@RReverser
Copy link
Collaborator Author

@kripken Looks like there's no active development on master at the moment? Should I target some other branch in this PR?

@CryZe
Copy link
Contributor

CryZe commented Feb 12, 2017

Yeah incoming as far as I know

@juj
Copy link
Collaborator

juj commented Feb 13, 2017

This looks good to me. Pull requests should go against incoming branch, so let me close this one. master branch is where merges of stable releases ultimately land. Could you add a comment though to explain that? I believe other JS shells will need to visit this as well, since they'll probably have the same issue.

@juj juj closed this Feb 13, 2017
@RReverser
Copy link
Collaborator Author

@juj As a suggestion, would it make sense to change Github's default branch (the one at which it will open repository view by default) to incoming to make this more easily visible?

@juj
Copy link
Collaborator

juj commented Feb 13, 2017

The GitHub default branch is the one which people will clone by default, which we want to point to the stable version of Emscripten, which is the master branch. Unfortunately GitHub does not support this use case of separating the default PR target branch from the default git clone branch.

@RReverser
Copy link
Collaborator Author

@juj I thought it rather makes sense to clone by default incoming as well to make contributions easier? And emsdk can specify exact branch anyway.

@RReverser
Copy link
Collaborator Author

RReverser commented Feb 13, 2017

@juj

I believe other JS shells will need to visit this as well, since they'll probably have the same issue.

Looks like other shells got this right with their read function, I've added test to test_other.py now and going to submit another PR.

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.

3 participants