fix_relative_file_loads_node_js #5484

juj · 2017-08-16T15:34:09Z

Add Module['scriptDirectory'] to fix loading files that should be relative to main .js file on Node.js.

Intended to replace #5368 to fix #4542.

kripken · 2017-08-16T22:36:30Z

emcc.py

@@ -2410,6 +2410,7 @@ def generate_html(target, options, js_target, target_basename,
            } else if (Module['memoryInitializerPrefixURL']) {
              memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
            }
+            if (Module['scriptDirectory'] && !/^(?:[a-z]+:)?\/\//i.test(memoryInitializer)) memoryInitializer = Module['scriptDirectory'] + memoryInitializer;


a comment explaining this would be good i think

kripken · 2017-08-16T22:37:43Z

src/preamble.js

+}
+
+function joinUrl(a, b) {
+  return a && !isAbsoluteUrl(b) ? a + b : b;


this seems to assume that a ends with /, perhaps in ASSERTIONS we should check that?

Good point, added.

kripken · 2017-08-16T22:38:45Z

src/preamble.js

@@ -2114,6 +2121,9 @@ function integrateWasmJS(Module) {
    wasmBinaryFile = Module['locateFile'](wasmBinaryFile);
    asmjsCodeFile = Module['locateFile'](asmjsCodeFile);
  }


so the goal is to support both locateFile and scriptDirectory at the same time (otherwise an else here could make sense)?

Yes, that's the idea. The existing ways to form a URL to load will all be treated with respect to scriptDirectory if the specified URLs are relative.

nazar-pc · 2017-08-17T11:36:20Z

This PR only fixes loading files in Node.js, but not in browser (browser suffers from the same issue as Node.js)
This PR introduces 5th prefix option on top of 4 prefix options already present, while practically speaking just 1 should be enough

juj · 2017-08-27T12:21:17Z

This PR only fixes loading files in Node.js, but not in browser (browser suffers from the same issue as Node.js)

What is the problem that exists in browsers? I don't think that has been mentioned anywhere before? Is there an STR or a test case to illustrate this problem?

This PR introduces 5th prefix option on top of 4 prefix options already present, while practically speaking just 1 should be enough

Agree that in an ideal world, we would probably have only one option, but we cannot just tear down the existing methods, since that will break anyone who is using them, and those people will then in turn complain. Once we get better mechanism for preprocessing (PR #5494), we can add optional mechanisms to disable the old ones, and eventually remove the old ones, if the added size consideration is an issue.

nazar-pc · 2017-08-27T12:29:30Z

What is the problem that exists in browsers?

Exactly the same problem as in Node. When you do XHR request, relative path will depend on the environment as I've described in my take on this issue: #5368 (comment).

Once we get better mechanism for preprocessing (PR #5494), we can add optional mechanisms to disable the old ones, and eventually remove the old ones, if the added size consideration is an issue.

Works for me, thanks!

juj · 2017-08-27T13:27:17Z

Exactly the same problem as in Node. When you do XHR request, relative path will depend on the environment as I've described in my take on this issue: #5368 (comment).

Thanks, I see. In that scenario the download will be done relative to the .html file, or, <base> if specified. I don't think there's big reason to change that semantic, since there are already ways to configure it (5 ways by your count :). The big distinction on the web is that it's the site deployer who defines the directory structure, and everyone who visits the site will open via the same directory structure that the web site author defined, whereas on command line, one can invoke a script from any arbitrary CWD.

If we do choose to explicitly change the semantic of loading with respect to html, then we will need to account for the case of having loaded the .js file via a blob URL, in which case document.currentScript.src at least will not work there.

nazar-pc · 2017-08-27T13:37:28Z

I don't think there's big reason to change that semantic, since there are already ways to configure it (5 ways by your count :).

The same exact ways can be used for Node.js too :)

The point I've pursued in my PR was to make it just work out of the box for most people, regardless of the environment, regardless of the JavaScript file location. Unless you move some of generated files to CDN (but not all of them) or do something exotic like that, in which case there are necessary options available already.

kripken · 2017-09-25T20:22:26Z

src/preamble.js

+  return /^(?:[a-z]+:)?\/\//i.test(url);
+}
+
+function joinUrl(a, b) {


please add a comment that this both joins as the name suggests, but also allows a to be undefined, in which case it is ignored.

kripken · 2017-09-25T20:25:02Z

src/preamble.js

+
+function joinUrl(a, b) {
+#if ASSERTIONS
+  if (a && !a.endsWith('/')) throw 'First parameter to joinUrl() should end in /'!;


the string is not terminated at the end of the line

juj · 2017-10-12T20:33:20Z

Ok, here is what we are going to do:

I have added a new test to this PR to verify the command line node.js from relative directory case. Rebased the PR to latest and ran through default, other and browser suites.
This PR fixes the issue for the command line case, without changing the existing semantics, to supersede PR Fix for loading wasm files under Node.js and in browser when files we… #5368.
@nazar-pc : if you would like to contribute to fixing a problem with the browser case, please let's continue on that separately after this pull request, and by providing a test case in the browser test suite that illustrates the problem and verifies the fix.
If we want to start removing the number of different file locator mechanism that we have, we will do so by starting to #ifdef out the existing ones, in separate pull requests, following the style of Introduce SUPPORT_SHELL, SUPPORT_NODEJS, SUPPORT_SPIDERMONKEY, SUPPORT_IE11 etc. -s settings #5554, adding conditional options to opt out from unwanted features, before removing them. This is best done after more flexible preprocessing support (preprocess_pthread_main #5494) lands.

Does that sound agreeable to all? @nazar-pc, @kripken?

nazar-pc · 2017-10-12T21:05:11Z

src/preamble.js

@@ -932,6 +932,18 @@ function stackTrace() {
 }
 {{{ maybeExport('stackTrace') }}}

+function isAbsoluteUrl(url) {
+  return /^(?:[a-z]+:)?\/\//i.test(url);


Absolute paths in Node.js don't start with //, so this will always return false:

>> /^(?:[a-z]+:)?\/\//i.test('/absolute/path/on/server.js') false

Good point. Added a test for this scenario as well and fixed the regex.

nazar-pc · 2017-10-13T01:21:13Z

With all respect I still strongly believe that #5368 is a superior alternative.
Just cherry-picked test case from this PR into it as is, added test case for browser environment that verifies the fix and rebased against incoming.
AFAIK #5368 doesn't actually break something used in real world (no existing tests were modified in the process, default, browser and other tests were re-executed after rebase and are fine).

curiousdannii · 2017-10-13T01:33:41Z

This supersedes #4942 too, correct? Just checking, because I couldn't see that explicitly mentioned here.

nazar-pc · 2017-10-13T01:36:42Z

@curiousdannii, yes

juj · 2017-10-13T18:05:58Z

With all respect I still strongly believe that #5368 is a superior alternative.

Technically it has the potential of being simpler, but sometimes the constraints are non-technical, we do not want to do semantics changing modifications on this.

AFAIK #5368 doesn't actually break something used in real world

Unity content for example uses the prefix paths, and emunittest overrides those with Module.locateFile. This is something we are strict about not changing.

Let's follow up on the remaining limitations one issue at a time, with tests as proofs of problems, and fixes associated with them.

nazar-pc · 2017-10-13T18:36:30Z

Unity content for example uses the prefix paths, and emunittest overrides those with Module.locateFile. This is something we are strict about not changing.

Do you have a link to this?

UPD: I think there is yet another approach to this that will both keep existing semantics and will achieve the same goal as #5368, will start working on it right now

juj · 2017-10-13T20:46:37Z

Do you have a link to this?

Sorry, nothing else than downloading Unity and checking their build output.

nazar-pc · 2017-10-14T00:02:29Z

I think we can both agree on this: https://github.com/kripken/emscripten/pull/5368/files?diff=split&w=1

kripken · 2017-10-16T20:48:05Z

Unity content for example uses the prefix paths, and emunittest overrides those with Module.locateFile. This is something we are strict about not changing.

This is what made me change my mind - I didn't realize people do use these things in combination. And there are probably more people we don't know about.

I agree a nicer overall solution is possible, but I agree with @juj that we should not change the behavior here.

I also agree with @nazar-pc on the link from the last comment - adding scriptDirectory that way seems safe, and a good solution, and maybe a little simpler than this PR. But maybe I'm missing something that this PR does that that doesn't?

nazar-pc · 2017-10-30T22:42:52Z

@juj, did we come to an agreement here?

juj · 2018-01-11T21:50:00Z

@juj, did we come to an agreement here?

Sorry, I still think we should not change the way the existing variables work, and not hoist the old style *prefixURL variables to required-to-always-exist status in Module (even if preamble/shell populates them for you), because that will add to code size, and the desire (#5484 (comment)) was to work towards deprecating them in the future. (by #ifdeffing out).

…ative to main .js file on Node.js.

curiousdannii · 2018-09-17T11:23:30Z

#5368 was landed, so this should be closed as it's no longer mergeable. Doesn't mean the current implementation is perfect though!

kripken · 2018-09-17T19:53:58Z

Agree, thanks @curiousdannii

juj mentioned this pull request Aug 16, 2017

Fix for loading wasm files under Node.js and in browser when files we… #5368

Merged

kripken reviewed Aug 16, 2017

View reviewed changes

curiousdannii mentioned this pull request Aug 27, 2017

Multithreading 1/N: JS scripts via Blob URLs #5526

Merged

juj force-pushed the fix_relative_file_loads_node_js branch from 815d995 to fd5a831 Compare August 27, 2017 12:15

juj force-pushed the fix_relative_file_loads_node_js branch from fd5a831 to 3c88610 Compare September 8, 2017 08:54

kripken reviewed Sep 25, 2017

View reviewed changes

juj force-pushed the fix_relative_file_loads_node_js branch 2 times, most recently from 4965b25 to 6afe133 Compare October 12, 2017 17:57

nazar-pc reviewed Oct 12, 2017

View reviewed changes

curiousdannii mentioned this pull request Oct 14, 2017

Automatically try to load the Emterpreter data file #5665

Closed

juj force-pushed the fix_relative_file_loads_node_js branch from da6cf19 to a2a8329 Compare January 11, 2018 21:08

juj force-pushed the fix_relative_file_loads_node_js branch 3 times, most recently from febc020 to 8a48d0a Compare January 19, 2018 14:40

juj mentioned this pull request Jan 31, 2018

How can I load compiled files from a subfolder? #6164

Closed

curiousdannii mentioned this pull request Feb 1, 2018

Emit WebAssembly by default instead of asm.js #6168

Closed

juj force-pushed the fix_relative_file_loads_node_js branch 2 times, most recently from fd5a137 to a3c1d77 Compare February 2, 2018 00:49

juj force-pushed the incoming branch from e1622af to d54eb4e Compare May 21, 2018 13:46

Add Module['scriptDirectory'] to fix loading files that should be rel…

9f3441b

…ative to main .js file on Node.js.

juj force-pushed the fix_relative_file_loads_node_js branch from a3c1d77 to 9f3441b Compare July 21, 2018 20:37

kripken closed this Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix_relative_file_loads_node_js #5484

fix_relative_file_loads_node_js #5484

juj commented Aug 16, 2017

kripken Aug 16, 2017

juj Aug 27, 2017

kripken Aug 16, 2017

juj Aug 27, 2017

kripken Aug 16, 2017

juj Aug 27, 2017

nazar-pc commented Aug 17, 2017

juj commented Aug 27, 2017

nazar-pc commented Aug 27, 2017

juj commented Aug 27, 2017

nazar-pc commented Aug 27, 2017

kripken Sep 25, 2017

kripken Sep 25, 2017

juj Oct 12, 2017

juj commented Oct 12, 2017

nazar-pc Oct 12, 2017

juj Oct 13, 2017

nazar-pc commented Oct 13, 2017

curiousdannii commented Oct 13, 2017

nazar-pc commented Oct 13, 2017

juj commented Oct 13, 2017

nazar-pc commented Oct 13, 2017 •

edited

Loading

juj commented Oct 13, 2017

nazar-pc commented Oct 14, 2017

kripken commented Oct 16, 2017 •

edited

Loading

nazar-pc commented Oct 30, 2017

juj commented Jan 11, 2018

curiousdannii commented Sep 17, 2018

kripken commented Sep 17, 2018

fix_relative_file_loads_node_js #5484

fix_relative_file_loads_node_js #5484

Conversation

juj commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Aug 17, 2017

juj commented Aug 27, 2017

nazar-pc commented Aug 27, 2017

juj commented Aug 27, 2017

nazar-pc commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juj commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Oct 13, 2017

curiousdannii commented Oct 13, 2017

nazar-pc commented Oct 13, 2017

juj commented Oct 13, 2017

nazar-pc commented Oct 13, 2017 • edited Loading

juj commented Oct 13, 2017

nazar-pc commented Oct 14, 2017

kripken commented Oct 16, 2017 • edited Loading

nazar-pc commented Oct 30, 2017

juj commented Jan 11, 2018

curiousdannii commented Sep 17, 2018

kripken commented Sep 17, 2018

nazar-pc commented Oct 13, 2017 •

edited

Loading

kripken commented Oct 16, 2017 •

edited

Loading