Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v8] Add v8 Javascript engine port (#372). #12687

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Conversation

Kwizatz
Copy link
Contributor

@Kwizatz Kwizatz commented Aug 1, 2020

Tested triplets: x64-windows,x64-windows-static,x64-linux

Describe the pull request
This PR adds the v8 Javascript engine port,

The SHARED library version provides cmake configuration files with the following targets and variables:

  • v8::v8
  • v8::v8_libbase
  • v8::_libplatform
  • V8_LIBRARY
  • V8_LIBBASE_LIBRARY
  • V8_LIBPLATFORM_LIBRARY

The STATIC library version provides cmake configuration files with the following target and variable:

  • v8::v8_monolith
  • V8_MONOLITH_LIBRARY

In addition the following variables are set:

  • V8_COMPILE_OPTIONS required definition flags for projects linking against the library
  • V8_INCLUDE_DIR path to the library headers
  • V8_LIBRARIES the collection of libraries/targets required to properly link an external project, this includes icu, zlib, winmm, dbghelp, dl and pthread for a static setup on Windows or Linux accordingly.
  • V8_SNAPSHOT_BLOB location of the snapshot_blob.bin file, required in shared library setups to be copied to the same location as the executable file.

The port also installs the following executables into the tools directory:

  • bytecode_builtins_list_generator
  • gen-regexp-special-case
  • mksnapshot
  • torque

This Port makes no use of depot tools, uses msvc or gcc (no clang) accordingly for Windows or Linux and links against VCPKG provided dependency ports ICU and zlib instead of building a custom version as upstream does.

Example project to build the v8_shell example:

cmake_minimum_required(VERSION 3.0)
project(v8-shell)

find_package(v8 CONFIG REQUIRED)

add_executable(v8-shell shell.cc)
if(V8_MONOLITH_LIBRARY)
set_property(TARGET v8-shell PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$CONFIG:Debug:Debug>")
endif()
target_include_directories(v8-shell PRIVATE ${V8_INCLUDE_DIR})
target_link_libraries(v8-shell ${V8_LIBRARIES})

  • What does your PR fix? Fixes #
    Fixes issue Add Chrome V8 port #372

  • Which triplets are supported/not supported? Have you updated the CI baseline?

x64-windows,x64-windows-static and x64-linux.
It should also support 32 bit versions of those triplets, but this was not tested.
Any other triplet should be considered not supported.

To the best of my knowledge, yes.

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Aug 3, 2020
@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 3, 2020

Is there a way to pick a dependency based on platform at the CONTROL file level?
Seems like x64-linux has a dependency on glib, but x64-windows does not.

@JackBoosY
Copy link
Contributor

JackBoosY commented Aug 4, 2020

@Kwizatz See documentation.
Example: Build-Depends: glib (linux).

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 4, 2020

@JackBoosY Thanks for the heads up.

I am looking at the error on x86_windows now. For some reason the flag /Ob3 on 32 bit cl does not force inlines the way it does for 64 bits, so I am looking for a workaround.

I have no way to do x64_osx builds 😢, so I am going to disable that one on the CONTROL file, however I don't think it would require too many more changes.

@PhoebeHui
Copy link
Contributor

The pbc:x64-osx failures doesn't relate to this changes, it's baseline issue which has been fixed, I have rerun the osx CI testing.

@JackBoosY
Copy link
Contributor

[1/2] cd /Users/vagrant/Data/buildtrees/v8/src/561d05e738-9ecbd0f8c3.clean && /Volumes/data/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake -E env PKG_CONFIG_PATH="/Users/vagrant/Data/installed/x64-osx/debug/lib/pkgconfig" gn gen /Volumes/data/buildtrees/v8/x64-osx-dbg/x64-osx-dbg --args="is_debug=true is_component_build=false target_cpu=\"x64\" is_clang=false  use_custom_libcxx=false v8_enable_verify_heap=false icu_use_data_file=false enable_iterator_debugging=true v8_monolithic=true v8_use_external_startup_data=false" && /Volumes/data/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake -E env PKG_CONFIG_PATH="/Users/vagrant/Data/installed/x64-osx/debug/lib/pkgconfig" ninja -C /Volumes/data/buildtrees/v8/x64-osx-dbg/x64-osx-dbg v8_monolith
FAILED: CMakeFiles/v8 
cd /Users/vagrant/Data/buildtrees/v8/src/561d05e738-9ecbd0f8c3.clean && /Volumes/data/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake -E env PKG_CONFIG_PATH="/Users/vagrant/Data/installed/x64-osx/debug/lib/pkgconfig" gn gen /Volumes/data/buildtrees/v8/x64-osx-dbg/x64-osx-dbg --args="is_debug=true is_component_build=false target_cpu=\"x64\" is_clang=false  use_custom_libcxx=false v8_enable_verify_heap=false icu_use_data_file=false enable_iterator_debugging=true v8_monolithic=true v8_use_external_startup_data=false" && /Volumes/data/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake -E env PKG_CONFIG_PATH="/Users/vagrant/Data/installed/x64-osx/debug/lib/pkgconfig" ninja -C /Volumes/data/buildtrees/v8/x64-osx-dbg/x64-osx-dbg v8_monolith
ERROR at //build/config/mac/mac_sdk.gni:102:19: Script returned non-zero exit code.
_mac_sdk_result = exec_script(script_name, sdk_info_args, "scope")
                  ^----------
Current dir: /Volumes/data/buildtrees/v8/x64-osx-dbg/x64-osx-dbg/
Command: python /Volumes/data/buildtrees/v8/src/561d05e738-9ecbd0f8c3.clean/build/config/mac/sdk_info.py macosx
Returned 1.
stderr:

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance
Traceback (most recent call last):
  File "/Volumes/data/buildtrees/v8/src/561d05e738-9ecbd0f8c3.clean/build/config/mac/sdk_info.py", line 159, in <module>
    FillXcodeVersion(settings, args.developer_dir)
  File "/Volumes/data/buildtrees/v8/src/561d05e738-9ecbd0f8c3.clean/build/config/mac/sdk_info.py", line 66, in FillXcodeVersion
    lines = subprocess.check_output(['xcodebuild', '-version']).splitlines()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 223, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['xcodebuild', '-version']' returned non-zero exit status 1

See //build/toolchain/mac/BUILD.gn:15:1: whence it was imported.
import("//build/config/mac/mac_sdk.gni")
^--------------------------------------
See //BUILD.gn:905:1: which caused the file to be included.
action("postmortem-metadata") {
^------------------------------
ninja: build stopped: subcommand failed.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 7, 2020

Was this check really passing before?

The only change from the last commit that could have affected it was the sysroot flag being set exclusively for Linux.
I'll change it so it is set for OSX too, but like I said, I have no access to a OSX setup to test this on 😢

@JackBoosY
Copy link
Contributor

@vejmartin can you please review this PR?

Thanks.

ports/v8/CMakeLists.txt Outdated Show resolved Hide resolved
ports/v8/icu.gn.in Outdated Show resolved Hide resolved
ports/v8/icu.gn.in Outdated Show resolved Hide resolved
ports/v8/portfile.cmake Outdated Show resolved Hide resolved
ports/v8/portfile.cmake Outdated Show resolved Hide resolved
ports/v8/portfile.cmake Outdated Show resolved Hide resolved
ports/v8/zlib.gn.in Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

The soil regression will be fixed in another PR.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 19, 2020

The soil regression will be fixed in another PR.

Do you mean the changes on pkgconfig fixup and icu?
I am ok with that, I do need the icu pkgconfig files installed for the build to succeed as it is now.
I'll look into the new failures and move this PR to draft for the moment, but overall I think the outlook is good 😁

@Kwizatz Kwizatz marked this pull request as draft August 19, 2020 22:54
@JackBoosY
Copy link
Contributor

@Kwizatz I mean #13000

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 21, 2020

There seems to be something wrong with the release version of the icu libraries, the build for x64-windows-dbg completes properly, but the one for x64-windows-rel fails due to undefined external symbols in icuuc.lib.
x64-linux completes without error, and everything I tried on the V8 side failed to fix the Windows release version.

My guess is that for some reason some symbols are not being exported on icu release build, or something is slightly different between icuucd.lib and icuuc.lib somehow, we need to have what the d version has in the non d version.

It was working before, so maybe a recent change in icu introduced the problem. I have not tried to go back to a previous icu port.

@JackBoosY
Copy link
Contributor

@Kwizatz I think so.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 24, 2020

I think I found the problem with ICU release, trying to solve it.

@JackBoosY
Copy link
Contributor

If you need my help, please ping me.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 25, 2020

Thanks @JackBoosY, I think I got it, it seems that the portfile is adding the optimization option /Oi to generate intrinsics on release mode, the autoconf script then looks for the wcscpy function by linking a program that calls wcscpy(), no arguments, wcscpy is one of the intrinsics, cl.exe doesn't like the call to the intrinsic without arguments and errors out.

This error is taken by autoconf to mean the function is not found, and somehow does a workaround that causes ICU to build, but not have the proper signatures for the functions being used by v8.

Since I like the intrinsics optimization, I added a patch and a call to autoconf to not run the check if both CXX and CC are cl.exe.

Edit: Hmm no, it seems that this problem is there and may be part of the problem, but I am still seeing missing symbol errors.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 25, 2020

Hi, @JackBoosY,
The problem is not in ICU, v8 adds C:/Program Files (x86)/Windows Kits/10/lib/10.0.19041.0/um/x64 as a /LIBPATH before the vcpkg library path, and that path happens to have ICU libraries matching the release name, which is why it is not finding the symbols.

I'll try to find a workaround.

Edit: There is the wcscpy issue which I don't know in what level is causing the CI tests to fail.

@JackBoosY
Copy link
Contributor

JackBoosY commented Aug 26, 2020

@Kwizatz I'll look into it tomorrow, sorry.

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 26, 2020

Hi, @JackBoosY no worries, there is a pkgconfig wrapper, I just added an argument to it so it outputs full paths to the libraries rather than just the name.

I am having another issue though, this one is not as troublesome, but I need some clarification.

ICU static libraries have a "s" prefix by default, for example sicuuc.lib, but the portfile renames them to remove the "s",
pkgconfig outputs the "s" names and this causes an error.

So, is there a reason this was done this way?

Edit: nevermind I found out why.

@Kwizatz Kwizatz marked this pull request as ready for review August 27, 2020 04:05
Tested triplets: x64-windows,x64-windows-static,x64-linux
@JackBoosY
Copy link
Contributor

Adding v8 to vcpkg is a big step for vcpkg's progress. This means that vcpkg can accommodate more complex ports.
Thanks for this PR.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 28, 2020
@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 28, 2020

Adding v8 to vcpkg is a big step for vcpkg's progress. This means that vcpkg can accommodate more complex ports.
Thanks for this PR.

You're welcome 😁,
@JackBoosY, @PhoebeHui, @vejmartin Thank you for your help and support.

@BillyONeal BillyONeal merged commit 5cb765a into microsoft:master Aug 28, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

@Kwizatz
Copy link
Contributor Author

Kwizatz commented Aug 28, 2020

Thanks for your contribution!

While its been a nightmare to get it right, its been a pleasure working with you all. 😁...
Now I have to do the 8.4 upgrade 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants