Replace therubyracer with mini_racer #1961

dsander · 2017-04-09T13:25:48Z

Initially this PR was created to improve compatibility on ARM(64) devices. Sadly this did not work out, the newer libv8 version still has issues compiling on arm devices.

For all other users the change is a improvement, mini_racer is a lot faster because the libv8 version is newer.

It also fixes a massive memory leak reported in #2512.

#1959

Might help with #1919

cantino · 2017-04-10T03:13:28Z

I haven't been following this closely. Would you mind summarizing the main motivation(s) for moving away from therubyracer?

0xdevalias · 2017-04-10T09:30:10Z

From my perspective, my biggest motivation is the alpine issue with therubyracer currently. mini_racer has a smaller bind footprint to libv8, so it can be updated to modern versions more easily. Which may come with performance/etc benefits (i'm not too well versed on this area)

I was thinking if it wasn't too hard to support both, maybe there would be some benefit to that. Originally I was thinking execjs might serve as a higher level wrapper to support that, but as @dsander pointed out in #1959 , due to the functions this wouldn't be possible.

dsander · 2017-04-10T14:44:47Z

It will definitely be faster rubyracers libv8 version is pretty old (3.16 vs 5.3). I am also hoping for better arm support due to the newer libv8 (#1586).

0xdevalias · 2017-04-10T20:25:54Z

How hard would it be to write our own little JS lib helper abstraction? That would check which lib was installed (or use an env var) and then expose the appropriate functions that we use.

dsander · 2017-04-10T20:40:19Z

@alias1 Not very I suppose, what would be the benefit of that?

0xdevalias · 2017-04-10T23:36:08Z

I guess I always think in terms of flexibility/options, rather than rigid decisions. So it would be providing the option of using either therubyracer or mini_racer (and perhaps setting a framework for supporting other options that may be available in that sense). It was the original reason I was thinking execjs would make a good choice.

I'm not really strongly set on it though, if you prefer to go the convention over configuration, 'best choice' approach.

dsander · 2017-04-11T19:43:13Z

Should have checked that earlier, mini_racer requires GCC 4.8 which is not available on debian wheezy. If we want it we need to either drop support for debian 7, wait until the LTS support ends in may 2018 or indeed support both mini_racer and therubyracer. Yay stability 😞

I think we should only pursue this further if it at least solves the ARM 64biy or alpine issues (ideally both).

0xdevalias · 2017-04-11T21:24:31Z

While I haven't directly tested it, i'm at least 90% sure this will work for the alpine issue. From my reading, mini_racer runs well on it. I'm unsure about the ARM issue.

My thought process at the moment would be to create a small wrapper class that holds the appropriate context, and exposes eval and function type methods, as well as a general Exception wrapper, and way of determining the proper array/object type for the underlying library.

If we designed the API properly, it could almost be a source compatible drop-in, at least for our use case.

cantino · 2017-04-12T08:33:33Z

Ah, got it. As long as mini_racer works for the JS Agent and on our supported platforms, it seems fine to me. It sucks that the_ruby_racer isn't being well supported.

dsander · 2017-04-13T19:49:20Z

My hesitation is not due to the small wrapper it would require, but more about the needed gcc version detection (I can't imagine parsing the output of gcc -v is enough to work on every platform). Having two different javascript runtimes on different OS will probably also not reduce the required support 😄 .
For me it comes down to the gain for our users (smaller docker image/supporting ARM 64bit) vs the development/maintenance and support burden for us.

0xdevalias · 2017-04-13T22:16:58Z

@cantino There is some work on upgrading therubyracer to v4.5, but from the little look i've had it seems like it may still be slow going.

@dsander Haha, yeah, that's fair.. Just increases the surface area for potential issues I suppose :p

If we want it we need to either drop support for debian 7, wait until the LTS support ends in may 2018 or indeed support both mini_racer and therubyracer.

Do we know much around usage/etc. I guess we wouldn't have stats at all.. (on a sidenote, I wonder if there is some benefit to having a 'send basic stats' option on install.. mostly gathering OS/ruby vers/etc to help when deciding around platforms to continue supporting)

Would dropping support for debian 7 be a major hit to users? Could we add both runtimes as an interim 'fallback' measure until LTS ends in 2018, then move it entirely to mini_racer then? My gut feel is that supporting 2 JS runtimes (with one designated as the 'officially supported', and the other deprecated/only supported for legacy systems) wouldn't be a terrible overhead on the maintenance team, and would probably be less impactful than dropping support for an entire OS on users. Thoughts?

cantino · 2017-04-15T09:17:00Z

I do think having stats would be nice. I wouldn't want to just add Google Analytics, though, since people like their privacy. Not sure what the best way to add stats in a privacy-respecting way would be.

dsander · 2017-04-15T09:18:47Z

Having usage statistics would be great, but I don't know how many users would agree to send them.

Unsure about dropping Debian 7 support, as a user I would be very annoyed if a software dropped support for a OS that is still in LTS.

dsander · 2017-04-27T19:21:41Z

Scaleway release 64bit ARM servers today and I tested the installation of mini_racer. Sadly it did not went to well:

root@scw-874759:/# lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.2 LTS
Release:	16.04
Codename:	xenial
root@scw-874759:/# uname -ra
Linux scw-874759 4.9.23-std-1 #1 SMP Mon Apr 24 13:18:14 UTC 2017 aarch64 aarch64 aarch64 GNU/Linux

Gem::Ext::BuildError: ERROR: Failed to build gem native extension.

    current directory: /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8
/usr/local/bin/ruby -r ./siteconf20170427-16707-1sv7jvu.rb extconf.rb
creating Makefile
Error: Command '/usr/bin/python v8/build/linux/sysroot_scripts/install-sysroot.py --running-as-hook' returned non-zero exit status 1 in /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor
Running: gclient root
Running: gclient config --spec 'solutions = [
  {
    "managed": False,
    "name": "v8",
    "url": "https://chromium.googlesource.com/v8/v8.git",
    "custom_deps": {},
    "deps_file": "DEPS",
    "safesync_url": "",
  },
]
'
Running: gclient sync --with_branch_heads
Traceback (most recent call last):
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 353, in <module>
    sys.exit(main())
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 348, in main
    return run(options, spec, root)
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 342, in run
    return checkout.init()
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 142, in init
    self.run_gclient(*sync_cmd)
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 76, in run_gclient
    return self.run(cmd_prefix + cmd, **kwargs)
  File "/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/vendor/depot_tools/fetch.py", line 66, in run
    return subprocess.check_output(cmd, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 574, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '('gclient', 'sync', '--with_branch_heads')' returned non-zero exit status 2
/home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8/builder.rb:109:in `block in setup_build_deps!': unable to fetch v8 source (RuntimeError)
	from /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8/builder.rb:107:in `chdir'
	from /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8/builder.rb:107:in `setup_build_deps!'
	from /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8/builder.rb:63:in `build_libv8!'
	from /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5/ext/libv8/location.rb:24:in `install!'
	from extconf.rb:7:in `<main>'

extconf failed, exit code 1

Gem files will remain installed in /home/huginn/huginn/vendor/bundle/ruby/2.3.0/gems/libv8-5.3.332.38.5 for inspection.
Results logged to /home/huginn/huginn/vendor/bundle/ruby/2.3.0/extensions/aarch64-linux/2.3.0-static/libv8-5.3.332.38.5/gem_make.out

An error occurred while installing libv8 (5.3.332.38.5), and Bundler cannot continue.
Make sure that `gem install libv8 -v '5.3.332.38.5'` succeeds before bundling.

0xdevalias · 2017-04-28T06:29:37Z

Hrmm, that sucks :(

dsander · 2017-07-20T12:13:57Z

The latest version of mini_racer works on arm64 now. I will do some more tests but supporting Huginn on Raspberries (and other arm(64) boards/servers) would be really nice.

knu · 2017-07-20T12:54:13Z

JFYI, Ubuntu 12.04 LTS finally reached its EOL last April. Yay!

0xdevalias · 2017-07-20T20:37:59Z

Switching to mini_racer would probably be the unblock I need to get those alpine docker containers finished off too.

danihodovic · 2018-10-20T15:43:38Z

What is the status of this PR? It would be great to get a more modern version of JS into Huginn.

dsander · 2018-10-22T07:54:27Z

We should be able to merge this soon. LTS support for debian 7 ended in may, hopefully everybody upgrades their servers by now 😄

danihodovic · 2018-10-22T16:18:12Z

We should be able to merge this soon. LTS support for debian 7 ended in may, hopefully everybody upgrades their servers by now smile

Is there any concrete timeline?

dsander · 2018-10-23T08:02:36Z

Is there any concrete timeline?

Not really, but any testing of this branch or the huginnbuilder docker images is very welcome.

danihodovic · 2018-10-23T08:23:33Z

Not really, but any testing of this branch or the huginnbuilder docker images is very welcome.

I'll run the huginnbuilder branch :)

danihodovic · 2018-10-28T17:08:51Z

I've been running huginnbuilder/huginn:pr-1961 for a few days using my personal Huginn and it seems to work fine. The only issue for me is that the latest version of libv8 is 6.7 which supports older Ecmascript and not ES6, but I guess there is not much we can do there.

dsander · 2018-10-29T08:32:36Z

We also need to drop support for Ruby 2.2 on top of Debian 7 because it's not supported by the latest mini_racer version.

huginn#1959 Might help with huginn#1919

dsander · 2019-04-19T14:52:55Z

Updated the PR description, I think we should merge this soon (Debian 7 and Ruby 2.2 hit their EOL a long time ago).

knu · 2019-04-20T00:44:19Z

Gemfile.lock

@@ -374,7 +374,7 @@ GEM
      actionmailer (>= 3.2)
      letter_opener (~> 1.0)
      railties (>= 3.2)
-    libv8 (3.16.14.19)


Are we finally dumping this ancient version? So great! 😂

Looks like we could even go better than that.. mini_racer supports 7.3.x as of this PR

Introduce compatibility with V8 7.3 rubyjs/mini_racer#138

And this looks like they do (or soon will) support 7.4.x

Compatibility changes for V8 7.4 support rubyjs/mini_racer#142

Though this lib is still 7.3.x

https://github.com/cowboyd/libv8/releases

With the latest v8 version currently being 7.7.x

https://github.com/v8/v8/releases

dsander closed this Apr 9, 2017

dsander reopened this Apr 9, 2017

dsander force-pushed the mini_racer branch from e394f48 to 8bc8a4b Compare April 9, 2017 13:43

0xdevalias mentioned this pull request Apr 10, 2017

Replace therubyracer with execjs (if possible) #1959

Closed

dsander force-pushed the mini_racer branch from 8bc8a4b to b1c4e5a Compare May 21, 2017 08:25

sfischer13 mentioned this pull request Jan 20, 2018

issue with installing on RPi3 #2199

Open

dsander force-pushed the mini_racer branch from b1c4e5a to d49f939 Compare January 24, 2018 10:13

sfischer13 mentioned this pull request May 31, 2018

Installation problem on RPi 3B+ #2299

Open

LexiconCode mentioned this pull request Sep 20, 2018

Can't install Huginn on ARM device - libv8 tries to build with x64 arch #1586

Open

dsander force-pushed the mini_racer branch from d49f939 to 7dae4b5 Compare October 23, 2018 07:53

dsander added 2 commits April 16, 2019 20:05

Replace therubyracer with mini_racer

287e1f1

huginn#1959 Might help with huginn#1919

Upgrade mini_racer to the latest version

925e130

dsander force-pushed the mini_racer branch from 7dae4b5 to 925e130 Compare April 17, 2019 00:50

dsander mentioned this pull request Apr 17, 2019

Memory leak with single process #2512

Closed

dsander added 2 commits April 18, 2019 16:59

Rescue MiniRacer::Error instead of special subclasses

cdc9438

Drop support for Debian 7

4a505a8

dsander requested a review from knu April 19, 2019 14:53

knu approved these changes Apr 20, 2019

View reviewed changes

knu merged commit 351470a into huginn:master Apr 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace therubyracer with mini_racer #1961

Replace therubyracer with mini_racer #1961

dsander commented Apr 9, 2017 •

edited

cantino commented Apr 10, 2017

0xdevalias commented Apr 10, 2017

dsander commented Apr 10, 2017

0xdevalias commented Apr 10, 2017 via email •

edited

dsander commented Apr 10, 2017

0xdevalias commented Apr 10, 2017

dsander commented Apr 11, 2017

0xdevalias commented Apr 11, 2017

cantino commented Apr 12, 2017

dsander commented Apr 13, 2017

0xdevalias commented Apr 13, 2017

cantino commented Apr 15, 2017

dsander commented Apr 15, 2017

dsander commented Apr 27, 2017

0xdevalias commented Apr 28, 2017

dsander commented Jul 20, 2017 •

edited

knu commented Jul 20, 2017

0xdevalias commented Jul 20, 2017

danihodovic commented Oct 20, 2018 •

edited

dsander commented Oct 22, 2018

danihodovic commented Oct 22, 2018

dsander commented Oct 23, 2018

danihodovic commented Oct 23, 2018

danihodovic commented Oct 28, 2018

dsander commented Oct 29, 2018

dsander commented Apr 19, 2019

knu Apr 20, 2019

0xdevalias Jun 16, 2019 •

edited

Replace therubyracer with mini_racer #1961

Replace therubyracer with mini_racer #1961

Conversation

dsander commented Apr 9, 2017 • edited

cantino commented Apr 10, 2017

0xdevalias commented Apr 10, 2017

dsander commented Apr 10, 2017

0xdevalias commented Apr 10, 2017 via email • edited

dsander commented Apr 10, 2017

0xdevalias commented Apr 10, 2017

dsander commented Apr 11, 2017

0xdevalias commented Apr 11, 2017

cantino commented Apr 12, 2017

dsander commented Apr 13, 2017

0xdevalias commented Apr 13, 2017

cantino commented Apr 15, 2017

dsander commented Apr 15, 2017

dsander commented Apr 27, 2017

0xdevalias commented Apr 28, 2017

dsander commented Jul 20, 2017 • edited

knu commented Jul 20, 2017

0xdevalias commented Jul 20, 2017

danihodovic commented Oct 20, 2018 • edited

dsander commented Oct 22, 2018

danihodovic commented Oct 22, 2018

dsander commented Oct 23, 2018

danihodovic commented Oct 23, 2018

danihodovic commented Oct 28, 2018

dsander commented Oct 29, 2018

dsander commented Apr 19, 2019

knu Apr 20, 2019

Choose a reason for hiding this comment

0xdevalias Jun 16, 2019 • edited

Choose a reason for hiding this comment

dsander commented Apr 9, 2017 •

edited

0xdevalias commented Apr 10, 2017 via email •

edited

dsander commented Jul 20, 2017 •

edited

danihodovic commented Oct 20, 2018 •

edited

0xdevalias Jun 16, 2019 •

edited