Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(86)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by Wladimir Palant
Modified:
2 years, 7 months ago
Reviewers:
Vasily Kuznetsov
CC:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

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

Messages

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 ...
2 years, 7 months ago (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 ...
2 years, 7 months ago (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 ...
2 years, 7 months ago (2017-03-24 06:20:41 UTC) #3
Vasily Kuznetsov
2 years, 7 months ago (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
../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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5