Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29392768: Issue 5020 - [emscripten] Part 4: Make compile script Windows-compatible (Closed)

March 23, 2017, 2 p.m. by Wladimir Palant
March 27, 2017, 8 a.m.
Vasily Kuznetsov
Base URL:


This 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -22 lines) Patch
M compile View 1 3 chunks +18 lines, -22 lines 8 comments Download


Total messages: 4
Wladimir Palant
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 ...
March 23, 2017, 2:10 p.m. (2017-03-23 14:10:58 UTC) #1
Vasily Kuznetsov
LGTM for how all the changes are implemented. I'm wondering if requiring emscripten config in ...
March 23, 2017, 7:26 p.m. (2017-03-23 19:26:54 UTC) #2
Wladimir Palant
On 2017/03/23 19:26:54, Vasily Kuznetsov wrote: > I'm wondering if requiring emscripten config in home ...
March 24, 2017, 6:20 a.m. (2017-03-24 06:20:41 UTC) #3
Vasily Kuznetsov
March 24, 2017, 10:04 a.m. (2017-03-24 10:04:55 UTC) #4
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
> 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'),
> 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
> > doesn't work.
> There is emcc.bat on Windows but it relies on Python being in PATH. It's
> 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
> > 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
> 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
> 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
> then. So if you installed Emscripten you should always have that config file.

Thanks for the explanations, still LGTM :)

Powered by Google App Engine
This is Rietveld