Issue 4907 - Update v8 to 5.7.278 in libadblockplus

[sergei, 2017-05-30 15:21:32]

https://codereview.adblockplus.org/29451722/diff/29451723/Makefile
File Makefile (right):

https://codereview.adblockplus.org/29451722/diff/29451723/Makefile#newcode83
Makefile:83: -C ../../build \
these "cd third_party/v8 && ..." are required by gyp, the latter is very fragile
and in one or another place paths become wrong.

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
File android-v8-options.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
android-v8-options.gypi:5: 'clang_dir': '/usr/',
"/usr/" is a new magic string here, not sure whether it should be already
configurable though. If so, what about making it as a part of a new issue?

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies#newcode3
dependencies:3: buildtools = buildtools hg:
git:https://github.com/xxxz/buildtools.git@8c6d37906df5761d71a86d680293f601e4e8dee3
This line will be changed as soon as decision about the fix in buildtools is
made. The current line won't be pushed.

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi
File libadblockplus.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi...
libadblockplus.gypi:5: or OS=="netbsd" or OS=="android"', {
I used the old list of OSes (see also common.gypi) but do we really need them?
We don't say anything about it in readme, our Makefile supports only linux,
android and mac, and I'm pretty sure that whoever is trying to compile it for
something else will face many other issues anyway.
I would leave linux and android here. What do you think?

[hub, 2017-05-30 15:48:04]

Just a question, not preventing from landing this.

LGTM

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi
File common.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi#newcode17
common.gypi:17: 'cflags_cc': [ '-fno-rtti', '-std=c++11', '-fexceptions',
'-fpermissive' ],
we can look at that after, but would it build without -fpermissive?

[Eric, 2017-05-30 17:31:41]

For the record, I've been waiting three years for a v8 upgrade of any kind to
land. I reluctantly favor a bunch of seemingly-unnecessary version dependencies
over waiting further. That said, such dependencies need to be documented
correctly.

https://codereview.adblockplus.org/29451722/diff/29451723/README.md
File README.md (right):

https://codereview.adblockplus.org/29451722/diff/29451723/README.md#newcode23
README.md:23: * Microsoft Visual Studio 2015
We need to say here that VS 2017 will not work. Ordinary people will not assume
a specific version dependency.

https://codereview.adblockplus.org/29451722/diff/29451723/README.md#newcode67
README.md:67: You need Microsoft Visual C++ 2015 (Community Edition is
sufficient)
Again, this should say that the C++ version in VS 2017 will not work.

Is there actually a toolset upgrade required? Yes, and it's hard-coded to an old
version, and that version isn't listed here.

It looks like this should say "You need Microsoft Visual Studio 2015 installed
with the default directories". There's a dependency on a hard coded path name
for "vcvars32.bat".

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
File android-v8-options.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
android-v8-options.gypi:5: 'clang_dir': '/usr/',
On 2017/05/30 15:21:32, sergei wrote:
> "/usr/" is a new magic string here, not sure whether it should be already
> configurable though. If so, what about making it as a part of a new issue?

Since you're already asking for default installation directories for Windows, no
reason not to document that Clang must also be installed in the default
location.

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd
File build-v8.cmd (right):

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd#newcode13
build-v8.cmd:13: @call "c:\Program Files (x86)\Microsoft Visual Studio
14.0\vc\bin\vcvars32.bat"
This is completely ugly. I'm using VS 2017 ordinarily now, as will anybody
setting up a development environment from scratch. I still have VS 2015
installed, so this would run, but that won't be the case for others.

It looks as though the only reason to have this is to ensure "msbuild" is in the
path. Better just to document that requirement and get rid of a hard-coded path
to an yet-another superseded MS software version.

There's already a requirement to have a particular version of python in the
path, so it's not even new to need a special PATH environment in order to make
the build succeed.

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd#newcode21
build-v8.cmd:21: exit /b 0
Why suppress errors?  It seems that if there's an error in the script, you'd
want the build to halt.

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi
File common.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi#newcode16
common.gypi:16: # We might be able to do without after upgrading V8.
We're upgrading v8 now. Is this comment still relevant?

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat
File createsolution.bat (right):

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat#...
createsolution.bat:9: python msvs_gyp_wrapper.py --depth=build\ia32 -f msvs -I
libadblockplus.gypi --generator-output=build\ia32 -G msvs_version=2015
-Dtarget_arch=ia32 -Dhost_arch=ia32 libadblockplus.gyp
Here's another version dependency. Is it actually necessary?

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies#newcode12
dependencies:12: third_party/v8/tools/clang = chromium-src-tools-clang
hg:24cee4531b21 git:286099f3928097ad1602710b550dfc1ff5c22142
There's a dependency on a particular Google version of Clang??? That seems
insane. It's certainly not needed on Windows.

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gyp
File libadblockplus.gyp (right):

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gyp#...
libadblockplus.gyp:25:
'build/<(target_arch)/v8/build/<(CONFIGURATION_NAME)/v8_libplatform.lib',
Is there a reason for the explicit build instructions? Specifying VS project
dependencies explicitly with gyp "dependencies" data worked fine in the past.
This seems like a step backwards.

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi
File libadblockplus.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi...
libadblockplus.gypi:21: 'msbuild_toolset': 'v140_xp',
Why the XP version? Toolset 14.0 already won't work with ABP-IE as currently
configured, since its build is hard-coded to use toolset 12. We've talked about
dropping support for XP for while now. It looks like it's finally time.

And why require an out-dated version at all? It would seem we could at last just
stop specifying the toolset version entirely.

If ABP-IE needs a particular version, it would be better to have its gyp file
specify it. Then this gyp file could use that parameter. And since ABP-IE needs
new gyp files in order to use that version, just omit this entirely for now and
change it if or when ABP-IE ever upgrades.

https://codereview.adblockplus.org/29451722/diff/29451723/msvs_gyp_wrapper.py
File msvs_gyp_wrapper.py (right):

https://codereview.adblockplus.org/29451722/diff/29451723/msvs_gyp_wrapper.py...
msvs_gyp_wrapper.py:15: # js2c
# js2c paths

https://codereview.adblockplus.org/29451722/diff/29451723/msvs_gyp_wrapper.py...
msvs_gyp_wrapper.py:17: # build-v8
# build-v8 paths

The new comments only made sense to me when I was looking at the old code, so
they need some clarification.

https://codereview.adblockplus.org/29451722/diff/29451723/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29451722/diff/29451723/src/Utils.h#newcode35
src/Utils.h:35: void ThrowException(v8::Isolate* isolate, const std::string&
str);
Does this mean to throw a C++ exception or to throw a Javascript exception? The
name should specify.

I'd use "ThrowV8Exception".

[sergei, 2017-05-31 12:47:12]

https://codereview.adblockplus.org/29451722/diff/29451723/README.md
File README.md (right):

https://codereview.adblockplus.org/29451722/diff/29451723/README.md#newcode23
README.md:23: * Microsoft Visual Studio 2015
On 2017/05/30 17:31:40, Eric wrote:
> We need to say here that VS 2017 will not work. Ordinary people will not
assume
> a specific version dependency.

Should we say in addition that VS 2020 and 2030 will not work? BTW, 2017 maybe
does work, at least as editor, I have not tested on it.

I have changed this line to a new one mentioning minimal toolset because other
lines are about compilers, not about IDE.

https://codereview.adblockplus.org/29451722/diff/29451723/README.md#newcode67
README.md:67: You need Microsoft Visual C++ 2015 (Community Edition is
sufficient)
On 2017/05/30 17:31:40, Eric wrote:
> Again, this should say that the C++ version in VS 2017 will not work.
I'm not sure about it, as far as I know it's possible to install v140 toolset in
MSVS 2017, and v141 should be compatible with v140, so maybe it does work there.
> 
> Is there actually a toolset upgrade required? Yes, and it's hard-coded to an
old
> version, and that version isn't listed here.
The toolset is added above where compilers are listed.
> 
> It looks like this should say "You need Microsoft Visual Studio 2015 installed
> with the default directories". There's a dependency on a hard coded path name
> for "vcvars32.bat".
> 
I have added that it's tested with MSVS 2015 CE, it will be updated as soon as
we know whether it works with MSVS 2017 and what actions are required.

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
File android-v8-options.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/android-v8-options....
android-v8-options.gypi:5: 'clang_dir': '/usr/',
On 2017/05/30 17:31:40, Eric wrote:
> On 2017/05/30 15:21:32, sergei wrote:
> > "/usr/" is a new magic string here, not sure whether it should be already
> > configurable though. If so, what about making it as a part of a new issue?
> 
> Since you're already asking for default installation directories for Windows,
no
> reason not to document that Clang must also be installed in the default
> location.

I'm pretty sure that there is no default location for clang, because it highly
depends on a system and the package managing system. I would rather prefer to
have it configurable and required. However I think it should not stop the
current change.

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd
File build-v8.cmd (right):

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd#newcode11
build-v8.cmd:11: @python msvs_gyp_wrapper.py --depth=build\%ARCH%\v8 -f msvs -I
v8.gypi --generator-output=build\%ARCH%\v8 -G msvs_version=2015
-Dtarget_arch=%ARCH% -Dhost_arch=%ARCH% third_party/v8/src/v8.gyp
I would like to keep 2015 here hardcoded for present because v8 gyp files rely
on it internally. Of course it would be better to use the same configuration for
all projects, but even with hardcoded 2015 for v8 the rest should work. We can
always come back to it later if we see that it works without the parameter or
does not work and some actions are required.

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd#newcode13
build-v8.cmd:13: @call "c:\Program Files (x86)\Microsoft Visual Studio
14.0\vc\bin\vcvars32.bat"
On 2017/05/30 17:31:40, Eric wrote:
> This is completely ugly. I'm using VS 2017 ordinarily now, as will anybody
> setting up a development environment from scratch. I still have VS 2015
> installed, so this would run, but that won't be the case for others.
> 
> It looks as though the only reason to have this is to ensure "msbuild" is in
the
> path. Better just to document that requirement and get rid of a hard-coded
path
> to an yet-another superseded MS software version.
> 
> There's already a requirement to have a particular version of python in the
> path, so it's not even new to need a special PATH environment in order to make
> the build succeed.

Done. This script actually is not supposed to be manually called for present, as
it's currently called we can actually pass a path to msbuild binary directory,
what is done in the change.

https://codereview.adblockplus.org/29451722/diff/29451723/build-v8.cmd#newcode21
build-v8.cmd:21: exit /b 0
On 2017/05/30 17:31:40, Eric wrote:
> Why suppress errors?  It seems that if there's an error in the script, you'd
> want the build to halt.

Done.

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi
File common.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi#newcode16
common.gypi:16: # We might be able to do without after upgrading V8.
On 2017/05/30 17:31:40, Eric wrote:
> We're upgrading v8 now. Is this comment still relevant?
As far as I remember '-fpermissive' was not needed in my environment, so I would
propose to keep the comment as the reminder to check it later.

https://codereview.adblockplus.org/29451722/diff/29451723/common.gypi#newcode17
common.gypi:17: 'cflags_cc': [ '-fno-rtti', '-std=c++11', '-fexceptions',
'-fpermissive' ],
On 2017/05/30 15:48:03, hub wrote:
> we can look at that after, but would it build without -fpermissive?

Could you check it after landing of the change?

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat
File createsolution.bat (right):

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat#...
createsolution.bat:9: python msvs_gyp_wrapper.py --depth=build\ia32 -f msvs -I
libadblockplus.gypi --generator-output=build\ia32 -G msvs_version=2015
-Dtarget_arch=ia32 -Dhost_arch=ia32 libadblockplus.gyp
On 2017/05/30 17:31:40, Eric wrote:
> Here's another version dependency. Is it actually necessary?

It was already specified here and changing to a new version looks a smaller
change in comparison with dropping the parameter at all. Anyway, since it seems
working, let's try without it.

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies#newcode12
dependencies:12: third_party/v8/tools/clang = chromium-src-tools-clang
hg:24cee4531b21 git:286099f3928097ad1602710b550dfc1ff5c22142
On 2017/05/30 17:31:40, Eric wrote:
> There's a dependency on a particular Google version of Clang??? That seems
> insane. It's certainly not needed on Windows.

It's not clang, it's some additions and tools and these files are required by
gyp files in order to generate Makefiles for v8 when it's built for android.
Currently there is no way to say that a particular dependency should not be used
on a particular platform or case.
JIC, the size of this directory is few megabytes.
Basically, with the current usage it's needed merely to get a hardcoded in
scripts/update.py (it seems so) version of clang :).

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gyp
File libadblockplus.gyp (right):

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gyp#...
libadblockplus.gyp:25:
'build/<(target_arch)/v8/build/<(CONFIGURATION_NAME)/v8_libplatform.lib',
On 2017/05/30 17:31:40, Eric wrote:
> Is there a reason for the explicit build instructions? Specifying VS project
> dependencies explicitly with gyp "dependencies" data worked fine in the past.

Now it does not work because gyp messes the configuration, so it was required to
exclude v8 gyp-files from our configuration.

> This seems like a step backwards.
It is indeed a step forward because it makes no sense to spend so much time on
building of v8 and it should be used in a compiled state. Having it built
separately here also simplifies the future switching to using of v8 binaries.

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi
File libadblockplus.gypi (right):

https://codereview.adblockplus.org/29451722/diff/29451723/libadblockplus.gypi...
libadblockplus.gypi:21: 'msbuild_toolset': 'v140_xp',
On 2017/05/30 17:31:41, Eric wrote:
> Why the XP version? Toolset 14.0 already won't work with ABP-IE as currently
> configured, since its build is hard-coded to use toolset 12. We've talked
about
> dropping support for XP for while now. It looks like it's finally time.
> 
> And why require an out-dated version at all? It would seem we could at last
just
> stop specifying the toolset version entirely.
> 
> If ABP-IE needs a particular version, it would be better to have its gyp file
> specify it. Then this gyp file could use that parameter. And since ABP-IE
needs
> new gyp files in order to use that version, just omit this entirely for now
and
> change it if or when ABP-IE ever upgrades.
Since there is no decision about dropping of _xp yet, I've added it here to be
sure that we still can compile, link for XP and tests successfully work.
Basically libadblockplus should not be aware about projects where it's used,
though it's unpractical sometimes.
Basically everything does work so far, so I think we may drop it here and make
required changes during updating of libadblockplus in IE.
Removed.

https://codereview.adblockplus.org/29451722/diff/29451723/msvs_gyp_wrapper.py
File msvs_gyp_wrapper.py (right):

https://codereview.adblockplus.org/29451722/diff/29451723/msvs_gyp_wrapper.py...
msvs_gyp_wrapper.py:17: # build-v8
On 2017/05/30 17:31:41, Eric wrote:
> # build-v8 paths
> 
> The new comments only made sense to me when I was looking at the old code, so
> they need some clarification.

they are not paths, they are parameters for js2c and for build-v8 tools what was
in the comment below. It is gyp trying to treat everything as paths and expand
them.
Added a comment before the array, although it's rather unrelated change here.

Interestingly that it does not expand just added $(MSBuildBinPath), maybe I will
look into it later.

https://codereview.adblockplus.org/29451722/diff/29451723/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29451722/diff/29451723/src/Utils.h#newcode35
src/Utils.h:35: void ThrowException(v8::Isolate* isolate, const std::string&
str);
On 2017/05/30 17:31:41, Eric wrote:
> Does this mean to throw a C++ exception or to throw a Javascript exception?
The
> name should specify.
> 
> I'd use "ThrowV8Exception".

It's still unclear, changed to `ThrowExceptionInJS`.

[sergei, 2017-06-02 07:22:18]

rebased and in addition ARCH=ia32 fixed.
Now it can be applied.

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29451722/diff/29451723/dependencies#newcode3
dependencies:3: buildtools = buildtools hg:
git:https://github.com/xxxz/buildtools.git@8c6d37906df5761d71a86d680293f601e4e8dee3
On 2017/05/30 15:21:32, sergei wrote:
> This line will be changed as soon as decision about the fix in buildtools is
> made. The current line won't be pushed.

Done.

[hub, 2017-06-02 13:20:29]

LGTM

https://codereview.adblockplus.org/29451722/diff/29454579/README.md
File README.md (right):

https://codereview.adblockplus.org/29451722/diff/29454579/README.md#newcode29
README.md:29: * clang 3.6 for OS X
Trying to build it on macOS, I got hit by

https://github.com/nodejs/node-gyp/issues/569#issuecomment-255589932

Once solved it was fine.


Maybe worth mentioning you actually need Xcode.

[sergei, 2017-06-02 14:05:30]

https://codereview.adblockplus.org/29451722/diff/29454579/README.md
File README.md (right):

https://codereview.adblockplus.org/29451722/diff/29454579/README.md#newcode29
README.md:29: * clang 3.6 for OS X
On 2017/06/02 13:20:28, hub wrote:
> Trying to build it on macOS, I got hit by
> 
> https://github.com/nodejs/node-gyp/issues/569#issuecomment-255589932
> 
> Once solved it was fine.
> 
> 
> Maybe worth mentioning you actually need Xcode.

Interesting, was clang already installed? BTW what is called xcode_settings in
gyp is actually reflected in Makefiles, quite misleading.

[hub, 2017-06-02 14:14:13]

On 2017/06/02 14:05:30, sergei wrote:

> Interesting, was clang already installed? BTW what is called xcode_settings in
> gyp is actually reflected in Makefiles, quite misleading.

clang wasn't the problem. I had everything installed, just needed to do the
proper "xcode-select", etc. It is all documented in the issue.

[sergei, 2017-06-02 14:26:03]

https://codereview.adblockplus.org/29451722/diff/29454579/README.md
File README.md (right):

https://codereview.adblockplus.org/29451722/diff/29454579/README.md#newcode29
README.md:29: * clang 3.6 for OS X
On 2017/06/02 14:05:30, sergei wrote:
> On 2017/06/02 13:20:28, hub wrote:
> > Trying to build it on macOS, I got hit by
> > 
> > https://github.com/nodejs/node-gyp/issues/569#issuecomment-255589932
> > 
> > Once solved it was fine.
> > 
> > 
> > Maybe worth mentioning you actually need Xcode.
> 
> Interesting, was clang already installed? BTW what is called xcode_settings in
> gyp is actually reflected in Makefiles, quite misleading.

Done.

[hub, 2017-06-02 14:46:42]

LGTM

[Oleksandr, 2017-06-03 00:40:16]

Only a few minor comments, as I am very excited to see this as well!

I think it is important to make sure we support VS 2017 as well, because VS 2015
is now only available by either registering for MSDN subscription or for Dev
Essentials account for free https://www.visualstudio.com/vs/older-downloads/,
which some contributors might not like.

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd
File build-v8.cmd (right):

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode4
build-v8.cmd:4: rem %1 - MSBuildBinPath, e.g. c:\Program Files
(x86)\MSBuild\14.0\Bin
How about making the first parameter optional and using msbuild from PATH if it
isn't provided?

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode13
build-v8.cmd:13: @python msvs_gyp_wrapper.py --depth=build\%ARCH%\v8 -f msvs -I
v8.gypi --generator-output=build\%ARCH%\v8 -G msvs_version=2015
-Dtarget_arch=%ARCH% -Dhost_arch=%ARCH% third_party/v8/src/v8.gyp
I have tried building using VS2017, and it works somewhat fine, once the
projects are converted. However problem is that I had to manually upgrade the
project files produced here, which are overwritten every time this script runs.
Other option is to use something like /p:PlatformToolset=v141
(https://stackoverflow.com/questions/33380128/visual-studio-2015-command-line-...)
in the line below, but that doesn't work for some reason. 

How about making the solution generation optional here? Better yet, why not move
this into createsolutions.bat?

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode15
build-v8.cmd:15: @"%MSBuildBinPath%/msbuild.exe" /m
build/%ARCH%/v8/third_party/v8/src/v8.sln /p:Configuration=%CONFIGURATION%
/target:v8_snapshot,v8_libplatform,v8_libsampler
It's a lot of building. How about adding /maxcpucount:3, for example?
https://msdn.microsoft.com/en-us/library/ms164311.aspx

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp#ne...
src/JsEngine.cpp:216: while (!GetIsolate()->IdleNotification(1000));
Maybe an explanation about the magic number would be good to have? Alternatively
I understand we can just remove the parameter here, because default value for
'hint' is 1000 anyway.

[sergei, 2017-06-14 16:05:03]

So, MSVS 2017 can be used as IDE as well as with v141, however there are options
and I'm not sure about v8 and v141, though it seems working.

I would propose to leave it as is, namely there is a default configuration, we
say about minimal requirements and tested version which seamlessly works in
readme and on practice one can use both MSVS 2017 with v141 and v140 toolset and
MSVS2015. So, I would propose to make any changes regarding the support of
future version of MSVS in following issues.

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat
File createsolution.bat (right):

https://codereview.adblockplus.org/29451722/diff/29451723/createsolution.bat#...
createsolution.bat:9: python msvs_gyp_wrapper.py --depth=build\ia32 -f msvs -I
libadblockplus.gypi --generator-output=build\ia32 -G msvs_version=2015
-Dtarget_arch=ia32 -Dhost_arch=ia32 libadblockplus.gyp
On 2017/05/31 12:47:11, sergei wrote:
> On 2017/05/30 17:31:40, Eric wrote:
> > Here's another version dependency. Is it actually necessary?
> 
> It was already specified here and changing to a new version looks a smaller
> change in comparison with dropping the parameter at all. Anyway, since it
seems
> working, let's try without it.

Without that flag it generates for Visual Studio 2005 and MSVS 2017 requires a
conversion. However with "-G msvs_version=2015" MSVS 2017 can open it and any
conversion is optional, of course if v140 is installed (one can do it via adding
"VC++ 2015.3 v140 toolset" feature). It seems reasonable to have 2015 here
because the generated files are almost seamlessly supported by both 2015 and
2017 and less conversion is required.
So, "-G msvs_version=2015" is restored.

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd
File build-v8.cmd (right):

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode4
build-v8.cmd:4: rem %1 - MSBuildBinPath, e.g. c:\Program Files
(x86)\MSBuild\14.0\Bin
On 2017/06/03 00:40:13, Oleksandr wrote:
> How about making the first parameter optional and using msbuild from PATH if
it
> isn't provided?

Currently it's not needed because that script is not manually called, so I would
like to not complicate it.

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode13
build-v8.cmd:13: @python msvs_gyp_wrapper.py --depth=build\%ARCH%\v8 -f msvs -I
v8.gypi --generator-output=build\%ARCH%\v8 -G msvs_version=2015
-Dtarget_arch=%ARCH% -Dhost_arch=%ARCH% third_party/v8/src/v8.gyp
On 2017/06/03 00:40:13, Oleksandr wrote:
> I have tried building using VS2017, and it works somewhat fine, once the
> projects are converted. However problem is that I had to manually upgrade the
> project files produced here, which are overwritten every time this script
runs.
V8 project files don't require conversion when one uses MSVS2017. It does not
show any dialog or what ever, why did you have to convert them?
> Other option is to use something like /p:PlatformToolset=v141
>
(https://stackoverflow.com/questions/33380128/visual-studio-2015-command-line-...)
> in the line below, but that doesn't work for some reason. 
I'm not sure how V8 is working with v141 because the used version of V8
explicitly supports v140. However it seems it does work, so instead of adding
"/p:PlatformToolset=v141" with a hardcoded value I have add a command line
argument for it, so currently the value is taken from project setting for the
toolset. Namely, if one uses MSVS 2017 and converts libadblockplus project files
then toolset for build-v8 project is v141, otherwise v140 is used.
> 
> How about making the solution generation optional here? Better yet, why not
move
> this into createsolutions.bat?
I agree that generating of v8 project files each time we build libadblockplus
looks bad, fortunately it does not cause a full rebuilding of v8, so it should
not hurt here.
If the solution generation is in createsolutions.bat then it should be done for
all projects using libadblockplus.gyp but not that createsolution.bat, e.g. ABP
for IE. Beside it complicates things, it looks a bit strange that one has to
generate a solution for dependency (v8) of a dependency (libadblockplus). If it
does not hurt, what should not, I would rather keep it as is for present. There
is a plan to stop building of V8 in libadblockplus and use prebuilt binaries, so
I consider this script rather as a temporary solution.

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode15
build-v8.cmd:15: @"%MSBuildBinPath%/msbuild.exe" /m
build/%ARCH%/v8/third_party/v8/src/v8.sln /p:Configuration=%CONFIGURATION%
/target:v8_snapshot,v8_libplatform,v8_libsampler
On 2017/06/03 00:40:13, Oleksandr wrote:
> It's a lot of building. How about adding /maxcpucount:3, for example?
> https://msdn.microsoft.com/en-us/library/ms164311.aspx

What do you mean under a lot of building? Does /m slow down the overall
performance and you want to limit it to use only three CPU? I'm not sure that we
should do it.

https://codereview.adblockplus.org/29451722/diff/29465685/googletest.gyp
File googletest.gyp (left):

https://codereview.adblockplus.org/29451722/diff/29465685/googletest.gyp#oldc...
googletest.gyp:41: 'OutputDirectory': '<(DEPTH)\\build\\$(ConfigurationName)',
I had to move the file and change it a bit because otherwise it was generating
files one folder upper from the root project folder, e.g. instead of
x:\projects\libadblockplus\build\ia32\build\Debug
it was in
x:\projects\build\ia32\build\Debug
(libadblockplus is missing).
Now it stores files as other projects in $(SolutionDir)$(Configuration)\ and
intermediate ones in $(Configuration)\obj\$(ProjectName)\.
For example, x:\projects\libadblockplus\build\ia32\Debug\obj\googletest.

[sergei, 2017-06-14 16:13:11]

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp#ne...
src/JsEngine.cpp:216: while (!GetIsolate()->IdleNotification(1000));
On 2017/06/03 00:40:15, Oleksandr wrote:
> Maybe an explanation about the magic number would be good to have?
Alternatively
> I understand we can just remove the parameter here, because default value for
> 'hint' is 1000 anyway.

One cannot remove 1000 because there is no default value for the parameter now.
Instead I have created another ticket https://issues.adblockplus.org/ticket/5320
for it.

[sergei, 2017-07-03 07:48:00]

Could you please review it?

[Oleksandr, 2017-07-03 21:50:39]

LGTM

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd
File build-v8.cmd (right):

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode13
build-v8.cmd:13: @python msvs_gyp_wrapper.py --depth=build\%ARCH%\v8 -f msvs -I
v8.gypi --generator-output=build\%ARCH%\v8 -G msvs_version=2015
-Dtarget_arch=%ARCH% -Dhost_arch=%ARCH% third_party/v8/src/v8.gyp
On 2017/06/14 16:05:01, sergei wrote:
> On 2017/06/03 00:40:13, Oleksandr wrote:
> > I have tried building using VS2017, and it works somewhat fine, once the
> > projects are converted. However problem is that I had to manually upgrade
the
> > project files produced here, which are overwritten every time this script
> runs.
> V8 project files don't require conversion when one uses MSVS2017. It does not
> show any dialog or what ever, why did you have to convert them?
> > Other option is to use something like /p:PlatformToolset=v141
> >
>
(https://stackoverflow.com/questions/33380128/visual-studio-2015-command-line-...)
> > in the line below, but that doesn't work for some reason. 
> I'm not sure how V8 is working with v141 because the used version of V8
> explicitly supports v140. However it seems it does work, so instead of adding
> "/p:PlatformToolset=v141" with a hardcoded value I have add a command line
> argument for it, so currently the value is taken from project setting for the
> toolset. Namely, if one uses MSVS 2017 and converts libadblockplus project
files
> then toolset for build-v8 project is v141, otherwise v140 is used.
> > 
> > How about making the solution generation optional here? Better yet, why not
> move
> > this into createsolutions.bat?
> I agree that generating of v8 project files each time we build libadblockplus
> looks bad, fortunately it does not cause a full rebuilding of v8, so it should
> not hurt here.
> If the solution generation is in createsolutions.bat then it should be done
for
> all projects using libadblockplus.gyp but not that createsolution.bat, e.g.
ABP
> for IE. Beside it complicates things, it looks a bit strange that one has to
> generate a solution for dependency (v8) of a dependency (libadblockplus). If
it
> does not hurt, what should not, I would rather keep it as is for present.
There
> is a plan to stop building of V8 in libadblockplus and use prebuilt binaries,
so
> I consider this script rather as a temporary solution.
> 

Acknowledged.

https://codereview.adblockplus.org/29451722/diff/29454636/build-v8.cmd#newcode15
build-v8.cmd:15: @"%MSBuildBinPath%/msbuild.exe" /m
build/%ARCH%/v8/third_party/v8/src/v8.sln /p:Configuration=%CONFIGURATION%
/target:v8_snapshot,v8_libplatform,v8_libsampler
On 2017/06/14 16:05:01, sergei wrote:
> On 2017/06/03 00:40:13, Oleksandr wrote:
> > It's a lot of building. How about adding /maxcpucount:3, for example?
> > https://msdn.microsoft.com/en-us/library/ms164311.aspx
> 
> What do you mean under a lot of building? Does /m slow down the overall
> performance and you want to limit it to use only three CPU? I'm not sure that
we
> should do it.

Looks like I have missed the /m switch in this line. Please nevermind this.

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29451722/diff/29454636/src/JsEngine.cpp#ne...
src/JsEngine.cpp:216: while (!GetIsolate()->IdleNotification(1000));
On 2017/06/14 16:13:10, sergei wrote:
> On 2017/06/03 00:40:15, Oleksandr wrote:
> > Maybe an explanation about the magic number would be good to have?
> Alternatively
> > I understand we can just remove the parameter here, because default value
for
> > 'hint' is 1000 anyway.
> 
> One cannot remove 1000 because there is no default value for the parameter
now.
> Instead I have created another ticket
https://issues.adblockplus.org/ticket/5320
> for it.

Acknowledged.

[hub, 2017-07-04 11:54:15]

LGTM