Issue 29392768: Issue 5020 - [emscripten] Part 4: Make compile script Windows-compatible (Closed)
|
Created:
March 23, 2017, 2 p.m. by Wladimir Palant Modified:
March 27, 2017, 8 a.m. Reviewers:
Vasily Kuznetsov CC:
hub Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionThis no longer calls emsdk in order to get Emscripten environment - this doesn't produce any useful results on Windows. Instead, we go for another hack and read .emscripten configuration file ourselves. This also has the advantage that we now can use Emscripten's configured Node.js rather than a random one installed on the system.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed line length #
Total comments: 8
MessagesTotal messages: 4
https://codereview.adblockplus.org/29392768/diff/29392769/compile File compile (right): https://codereview.adblockplus.org/29392768/diff/29392769/compile#newcode44 compile:44: 'EM_CONFIG': emscripten_config, It seems that Emscripten itself only needs this environment variable, everything else will be read from the config file. https://codereview.adblockplus.org/29392768/diff/29392769/compile#newcode47 compile:47: 'NODE_JS': scope.get('NODE_JS', 'node'), This will typically be misconfigured on Windows, see https://github.com/juj/emsdk/commit/87cb951ec7b9b3388c16c22aa259807a1c901c7a which still didn't make it into any emsdk releases.
LGTM for how all the changes are implemented. I'm wondering if requiring emscripten config in home directory could be a non-obvious change. I'm not quite aware of the workflow where this script will be used so I assume you know what you're doing. The comments in the code are just for me to keep track of what's going on. I thought I'd leave them just in case someone like me without a real clue will be looking at this review. https://codereview.adblockplus.org/29392768/diff/29393562/compile File compile (right): https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode39 compile:39: def getenv(emscripten_config): Instead of running `emsdk_env.sh` located at `emscripten_path` we now use load emscripten config (which is in python) and read configuration variables from there. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode54 compile:54: env['PYTHON'], os.path.join(env['EMSCRIPTEN'], 'emcc'), BINDINGS_FILE, We use python interpreter configured in emscripten config (or default to our python) to run `emcc` because on windows we can't just run since shebang line doesn't work. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode61 compile:61: subprocess.check_call([env['NODE_JS'], BINDINGS_GENERATOR], We use the path to node from emscripten config (defaulting to just calling "node" and hoping that it's in the path) instead of previous unix-specific way of determining the path to node. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode67 compile:67: env['PYTHON'], os.path.join(env['EMSCRIPTEN'], 'emcc'), Again invoke python script with a python interpreter since it won't work without. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode88 compile:88: '--emscripten-config', We switch to using emscripten config instead of using the path to emscripten installation as before. This is not completely obvious to me since we didn't need the config before and now we need it. As I understood, if we run `emcc` at least once, the config will be generated, so the only situation where the change would cause issues is if we have never un emscripten.
On 2017/03/23 19:26:54, Vasily Kuznetsov wrote: > I'm wondering if requiring emscripten config in home directory could be a > non-obvious change. Actually, that should work out of the box on almost every environment - much unlike the previous default expecting that emsdk is installed in ../emscripten. The only exception would be where people explicitly override the Emscripten config location, but then they probably know what to do. https://codereview.adblockplus.org/29392768/diff/29393562/compile File compile (right): https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode54 compile:54: env['PYTHON'], os.path.join(env['EMSCRIPTEN'], 'emcc'), BINDINGS_FILE, On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > We use python interpreter configured in emscripten config (or default to our > python) to run `emcc` because on windows we can't just run since shebang line > doesn't work. There is emcc.bat on Windows but it relies on Python being in PATH. It's better to explicitly run the Python version that Emscripten came with - and that approach doesn't require platform-specific code. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode61 compile:61: subprocess.check_call([env['NODE_JS'], BINDINGS_GENERATOR], On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > We use the path to node from emscripten config (defaulting to just calling > "node" and hoping that it's in the path) instead of previous unix-specific way > of determining the path to node. The drawback here is that Emscripten comes with a pretty old Node version. But then again - that's the Node version they tested with, so there shouldn't be any issues. https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode88 compile:88: '--emscripten-config', On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > We switch to using emscripten config instead of using the path to emscripten > installation as before. This is not completely obvious to me since we didn't > need the config before and now we need it. emsdk construct_env (which is what we have been calling essentially) doesn't produce any ouput on Windows - it cannot set environment variables for the current shell, so instead it manipulates the environment settings in the registry. Either way, parsing that output was quite a hack. I would have preferred loading emsdk as a module but it doesn't have anything resembling a stable API. So loading the config file seems to be the least hacky way to do it (yes, it is a Python file - "import os" at the top but then only defining constants). Emscripten defaults to ~/.emscripten for it on all platforms but allows overriding via EM_CONFIG environment variable. So we also allow overriding the config path and will forward it via environment. > As I understood, if we run `emcc` at > least once, the config will be generated, so the only situation where the change > would cause issues is if we have never un emscripten. Actually, you need emsdk in order to *install* Emscripten (emsdk can manage multiple Emscripten versions). The path to emcc is written into the config file then. So if you installed Emscripten you should always have that config file.
On 2017/03/24 06:20:41, Wladimir Palant wrote: > On 2017/03/23 19:26:54, Vasily Kuznetsov wrote: > > I'm wondering if requiring emscripten config in home directory could be a > > non-obvious change. > > Actually, that should work out of the box on almost every environment - much > unlike the previous default expecting that emsdk is installed in ../emscripten. > The only exception would be where people explicitly override the Emscripten > config location, but then they probably know what to do. > > https://codereview.adblockplus.org/29392768/diff/29393562/compile > File compile (right): > > https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode54 > compile:54: env['PYTHON'], os.path.join(env['EMSCRIPTEN'], 'emcc'), > BINDINGS_FILE, > On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > > We use python interpreter configured in emscripten config (or default to our > > python) to run `emcc` because on windows we can't just run since shebang line > > doesn't work. > > There is emcc.bat on Windows but it relies on Python being in PATH. It's better > to explicitly run the Python version that Emscripten came with - and that > approach doesn't require platform-specific code. > > https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode61 > compile:61: subprocess.check_call([env['NODE_JS'], BINDINGS_GENERATOR], > On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > > We use the path to node from emscripten config (defaulting to just calling > > "node" and hoping that it's in the path) instead of previous unix-specific way > > of determining the path to node. > > The drawback here is that Emscripten comes with a pretty old Node version. But > then again - that's the Node version they tested with, so there shouldn't be any > issues. > > https://codereview.adblockplus.org/29392768/diff/29393562/compile#newcode88 > compile:88: '--emscripten-config', > On 2017/03/23 19:26:53, Vasily Kuznetsov wrote: > > We switch to using emscripten config instead of using the path to emscripten > > installation as before. This is not completely obvious to me since we didn't > > need the config before and now we need it. > > emsdk construct_env (which is what we have been calling essentially) doesn't > produce any ouput on Windows - it cannot set environment variables for the > current shell, so instead it manipulates the environment settings in the > registry. Either way, parsing that output was quite a hack. > > I would have preferred loading emsdk as a module but it doesn't have anything > resembling a stable API. So loading the config file seems to be the least hacky > way to do it (yes, it is a Python file - "import os" at the top but then only > defining constants). Emscripten defaults to ~/.emscripten for it on all > platforms but allows overriding via EM_CONFIG environment variable. So we also > allow overriding the config path and will forward it via environment. > > > As I understood, if we run `emcc` at > > least once, the config will be generated, so the only situation where the > change > > would cause issues is if we have never un emscripten. > > Actually, you need emsdk in order to *install* Emscripten (emsdk can manage > multiple Emscripten versions). The path to emcc is written into the config file > then. So if you installed Emscripten you should always have that config file. Thanks for the explanations, still LGTM :) 
|
