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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b5c1a0
This seems to be a common source of issues when trying to run optimis…
RReverser Feb 11, 2017
e493a8c
Add comment explaining __dirname trick
RReverser Mar 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/postamble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ if (memoryInitializer) {
memoryInitializer = Module['locateFile'](memoryInitializer);
} else if (Module['memoryInitializerPrefixURL']) {
memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
} else if (ENVIRONMENT_IS_NODE) {
// Look for .mem file in the same folder as script by default
memoryInitializer = __dirname + '/' + memoryInitializer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some questions do come to mind:

  • does node.js have a path join function that could be used here? I.e. is it guaranteed that __dirname does not end in a '/' so that this won't do a /path/to/foo//foo.mem? On Windows, should this be a backslash '\' or is node.js satisfied or even expect '/' on Windows as well?
  • are other file loads with node.js affected similarly? e.g. why is .mem file special here for node.js?

Last, I wonder if this would be better to read

if (typeof Module['locateFile'] === 'function') {
  memoryInitializer = Module['locateFile'](memoryInitializer);
} else if (Module['memoryInitializerPrefixURL']) {
  memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
}

+ if (ENVIRONMENT_IS_NODE && isThisARelativeUrl(memoryInitializer)) {
+   // Relative paths should be treated relative to the folder where the .js script resides in
+   memoryInitializer = __dirname + '/' + memoryInitializer;
+ }

since Module['locateFile'] and Module['memoryInitializerPrefixURL'] may also be relative - do we ever want to load files relative to the cwd in node.js?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively to make it more generic, perhaps we could have a Module['documentDirectory'] which would be initialized to __dirname on node.js by default, and the above type of code would always read relative directories with respect to Module['documentDirectory'] if that exists. That way we'd avoid having to put in node.js specifics at each place of use (assuming we will have more of them in the future, e.g. when node.js adds .wasm support?), and if we need a similar thing for SpiderMonkey or other shells, it would be decoupled as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I.e. is it guaranteed that __dirname does not end in a '/' so that this won't do a /path/to/foo//foo.mem?

Yeah, it is.

On Windows, should this be a backslash '' or is node.js satisfied or even expect '/' on Windows as well?

Windows itself doesn't mind forward slashes and accepts either, so I wouldn't bother.

e.g. why is .mem file special here for node.js?

.mem is special only because Emscripten 1) generates it alongside the .js but 2) searches in current working dir and not alongside .js, which causes app to immediately crash if they don't match (e.g. when invoked by Cargo or other builder from top-level working directory). For regular file reads by the app itself, I think it's reasonable to leave behaviour as-is because they're not generated alongside .js in the first place.

Module['memoryInitializerPrefixURL'] may also be relative - do we ever want to load files relative to the cwd in node.js

Yeah, as said above, I think for normal cases we want to load files relative to cwd, so that program would behave in the same way as when compiled natively.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I don't understand the idea for Module['documentDirectory'] - what does "document" mean here?

Perhaps we need something like Module['pwd']?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kripken Overriding current working directory sounds irrelevant to this issue though? We want cwd to be still the same (at least where it exists for real, like in Node.js), it's only .mem that needs to be fixed to be found alongside .js as I explained above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@curiousdannii I already commented about the browser situations above couple of times - TL;DR it's quite different because browser doesn't have a notion of "pwd" unlike all the console shells, and not having a solution for everything is not a reason not to fix at least those we can. Anyway, this is blocked due to the other reasons now.

The idea to add this path name is a good solution. As commented earlier, I think it would be good to separate the location of initialization (platform specific) from the location of use (platform independent), so that we don't need to sprinkle if (ENVIRONMENT_IS_NODE) x = __dirname + x; statements to wherever we need to load items relative to the main .js document in question. I understand there is only one such place where we are fixing this for now, but as we discussed above, there will likely be more of such fixes in the future (.data, .wasm?), so having something like

    // in preamble:
    if (!Module['documentDirectory']) {
        if (ENVIRONMENT_IS_NODE) Module['documentDirectory'] = __dirname + '/';
    }

    // in location(s) of use:
    memoryInitializer = Module['documentDirectory'] + memoryInitializer;

This way when we expand in the future to having multiple locations that need this fix, we can just slap the Module['documentDirectory'] + bit to them without having to worry about node.js specifics in such locations of use. That will keep platform specifics down to that one line at init time. It's a small change to the current proposed form, but I think it would show a good form for future fixes. @kripken, did you have a thought about the name of such a variable?

Copy link
Collaborator Author

@RReverser RReverser Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

so that we don't need to sprinkle if (ENVIRONMENT_IS_NODE) x = __dirname + x; statements to wherever we need to load items relative to the main .js document in question

Oh of course, I was thinking about having a generic function rather than copy-pasting in the worst case.

Anyway, this idea sounds good to me, except the naming which might be confusing, especially on the browser side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about scriptDirectory? It should state pretty clearly that it should be a directory with generated script (and, consequently, other artifacts it depends upon).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Module['scriptDirectory'] sounds good to me. @kripken ?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me too.

}
if (ENVIRONMENT_IS_NODE || ENVIRONMENT_IS_SHELL) {
var data = Module['readBinary'](memoryInitializer);
Expand Down
12 changes: 12 additions & 0 deletions tests/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,18 @@ def _test():
output_filename='a.out.js')
_test()

def test_memory_file_not_in_cwd(self):
# Test that even if JS is executed in different working directory, then it should still find .js.mem file in the same folder as script itself
filename = path_from_root('a.out.js')
Building.emcc(path_from_root('tests', 'hello_world.c'), ['--memory-init-file', '1'], output_filename=filename)
cwd = path_from_root('tests')
print
print 'Filename: %s' % filename
print 'Working dir: %s' % cwd
for engine in JS_ENGINES:
print engine
run_js(filename, cwd=cwd)

def test_ungetc_fscanf(self):
open('main.cpp', 'w').write(r'''
#include <stdio.h>
Expand Down