Opened 5 years ago

Closed 2 years ago

#1197 closed change (rejected)

change local copy of v8 (to 4.3.15) to work with Visual Studio 2013

Reported by: eric@… Assignee: sergz
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd Blocked By: #1280, #1546, #1547, #1548, #1549, #1550, #1551, #1552, #3321, #4053, #4062
Blocking: #1087 Platform: Unknown
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

The current local copy of v8 we maintain in our own repository does not compile with Visual Studio 2013. The root of the issue is that VS 2012 and earlier did not define C99 math functions, so v8 provided replacements. VS 2013 does define them, leading to a number of compiler errors. This was a known issue, apparently fixed in the following patch set: https://codereview.chromium.org/23449035/

This deficit is blocking #1087, an upgrade to VS 2013 for some useful C++11 features.

Also relevant here is that the current version of v8 does not work with libadblockplus out of the box. Evidently internal structures of v8 have changed and our interface code needs modification.

What to change

There are a number of things that might be done. The following lists some of the alternatives:

  1. Simply apply the patch (or its moral equivalent) to our current copy. That is, make just enough modifications to get v8 to compile under VS 2013. The patch mostly changes conditional compilation with Visual Studio, so this change shouldn't affect Android at all.
  1. Upgrade to a published version of v8, one after the above patch landed and yet before its interface changed. Such a release may or may not exist.
  1. Upgrade to the current version of v8 and rework libadblockplus to work with it.

The tradeoffs here are all about the kind of work required. Option 1 requires working on the v8 code directly. Option 2 is largely research. Option 3 means working on our libadblockplus code.

Option 3 keeps our version of v8 up to date, which has other benefits, such as keeping abreast of v8 defect fixes (should they affect us). Even if, however, it's desirable to choose option 3, it has a relatively long lead time before completion, and it would be useful to also pick option 1 or 2 in addition to allow IE to upgrade to VS 2013 in the interim.

Change History (23)

comment:1 Changed 5 years ago by eric@…

There's a mitigation tactic we can use here. It doesn't actually address the issue, so at best it's a delaying tactic, though it could still be valuable. The trick is to declare the build tools for v8 to be "v110", which is for the compiler shipped with VS 2012, and to use "v120", the toolset version shipped with VS 2013, for everything else.

This is only temporary because it requires a second, ever-more-out-of-date toolset to be installed. It works fine for the moment because all the current developers already have VS 2012 installed, and they can just keep it installed until this issue is finally resolved.

comment:2 Changed 5 years ago by eric@…

So, the trick of compiling with different tool sets does not actually work. The trouble is "error LNK2038: mismatch detected for '_MSC_VER'". This is a hard block against using this technique. The standard library required between MSC version is ABI incompatible, and the linker can't link to multiple versions as would be appropriate.

This would likely work if libadblockplus were linked as a DLL rather than a static library, but that's not how the present build system works.

comment:3 Changed 5 years ago by fhd

  • Blocked By 1280 added

This sounds to me like upgrading to v8 (#1197) should do the trick.

Version 0, edited 5 years ago by fhd (next)

comment:4 Changed 5 years ago by fhd

  • Priority changed from Unknown to P4

comment:5 Changed 5 years ago by fhd

  • Priority changed from P4 to P3

Setting to P3 since #1087 depends on it. The version we update to in #1280 is not recent enough.

Last edited 5 years ago by fhd (previous) (diff)

comment:6 Changed 5 years ago by sergz

  • Blocked By 1546 added

comment:7 Changed 5 years ago by sergz

  • Blocked By 1547 added

comment:8 Changed 5 years ago by sergz

  • Blocked By 1548 added

comment:9 Changed 5 years ago by sergz

  • Blocked By 1549 added

comment:10 Changed 5 years ago by sergz

  • Blocked By 1551 added

comment:11 Changed 5 years ago by sergz

  • Blocked By 1552 added

comment:12 Changed 5 years ago by oleksandr

  • Blocking 2599 added

comment:13 Changed 4 years ago by oleksandr

  • Blocking 2940 added

comment:14 Changed 4 years ago by sergz

  • Blocked By 3321 added

comment:15 Changed 4 years ago by sergz

  • Blocked By 1550 added

comment:16 Changed 4 years ago by sergz

  • Blocking 2940 removed

comment:17 Changed 4 years ago by sergz

  • Blocking 2599 removed

comment:18 Changed 4 years ago by sergz

  • Summary changed from change local copy of v8 to work with Visual Studio 2013 to change local copy of v8 (to 4.3.15) to work with Visual Studio 2013
  • Tester set to Unknown

comment:19 Changed 4 years ago by sergz

  • Owner set to sergz

comment:20 Changed 4 years ago by eric@…

Frankly, I had intended this ticket to encompass a number of possible approaches, only one of which was to update v8. I had expected, if updating was the preferred approach, that there would be a new ticket to perform that update. This ticket would then be marked as depending upon that new one.

Since you want to reuse this ticket as the primary ticket to update, I'd recommend the ticket title be "Update local copy of v8 to version 4.3.15". The original motivation in VS version need not appear in the title.

comment:21 Changed 4 years ago by sergz

  • Blocked By 4053 added

comment:22 Changed 4 years ago by sergz

  • Blocked By 4062 added

comment:23 Changed 2 years ago by sergz

  • Resolution set to rejected
  • Status changed from new to closed

So, as a result of #4907 we use VS 2015 and V8 is updated, this issue can be closed. BTW one can now work on #1087.

Note: See TracTickets for help on using tickets.