Opened on 03/21/2017 at 09:20:06 AM

Closed on 03/27/2017 at 08:04:43 AM

#5020 closed change (fixed)

[emscripten] Improve compile script

Reported by: trev Assignee: trev
Priority: P4 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: #4122 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29390650/
https://codereview.adblockplus.org/29390657/
https://codereview.adblockplus.org/29390663/
https://codereview.adblockplus.org/29392768/

Description (last modified by trev)

Background

The script currently used to compile Emscripten code was meant as a temporary solution and has a number of shortcomings that are easy to fix.

What to change

  • Make the script PEP-8 compliant.
  • Use os.path.join() consistently.
  • Don't use current directory, resolve paths relative to script directory instead.
  • Add command line option to set Emscripten path and only use ../emscripten as fallback.
  • Add --debug command line flag to disable code minification.
  • Add --tracing command line flag to enable memory tracing.
  • Implement a Windows-compatible solution to retrieve Emscripten environment.

Attachments (0)

Change History (6)

comment:1 Changed on 03/21/2017 at 10:33:55 AM by trev

  • Description modified (diff)

comment:2 Changed on 03/21/2017 at 10:45:47 AM by trev

  • Description modified (diff)

comment:3 Changed on 03/21/2017 at 11:24:22 AM by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed on 03/23/2017 at 02:30:22 PM by trev

  • Review URL(s) modified (diff)

Added last part, the one making the script compatible with Windows. It makes the process of retrieving Emscripten configuration somewhat more robust by reading .emscripten configuration file directly instead of relying on stable emsdk output. In the process I found out that Emscripten on Windows isn't exactly a well-supported solution:

  • Node.JS path will be misconfigured. The issue has been fixed months ago but didn't make it into the download package yet. So you will have to edit emsdk_manifest.json of your installation manually.
  • In theory, both Visual Studio 2013 and Visual Studio 2015 are supported (Express Edition and probably the full one as well). However, emsdk will default to the former and you need to use the undocumented --vs2015 command line flag if you want to use Visual Studio 2015. Build directories are different which is why you have to add this flag to any emsdk command - not merely install but activate as well. This is even true when activating tools that weren't compiled at all, simply because it won't recognize compiled tools as active and remove them from configuration.
  • Even with the provisions above I couldn't make Emscripten activate correctly - the path to the native optimizer was always wrong, pointing to the missing Visual Studio 2013 version. I had to edit the configuration file manually to fix that.
  • #pragma once support requires help from the file system, so you shouldn't compile on a VirtualBox shared folder. If you do clang will get confused because it treats compiled\filter\..\String.h and compiled\String.h as different files.

comment:5 Changed on 03/27/2017 at 08:03:56 AM by abpbot

Some commits referencing this issue have landed:

comment:6 Changed on 03/27/2017 at 08:04:43 AM by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from trev.
 
Note: See TracTickets for help on using tickets.