Revert "Build v8 with i18n support"

[rogchap]

Reverts #136

[codecov]

Codecov Report

Merging #138 (2c59f2b) into master (a8894db) will increase coverage by 1.30%.
The diff coverage is n/a.

❗ Current head 2c59f2b differs from pull request most recent head d32e671. Consider uploading reports for the commit d32e671 to get more accurate results

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   95.59%   96.90%   +1.30%     
==========================================
  Files          16       12       -4     
  Lines         545      452      -93     
==========================================
- Hits          521      438      -83     
+ Misses         15        9       -6     
+ Partials        9        5       -4     

Impacted Files	Coverage Δ
context.go	94.59% <0.00%> (-5.41%)	⬇️
template.go	75.00% <0.00%> (-1.93%)	⬇️
errors.go	100.00% <0.00%> (ø)
object.go	100.00% <0.00%> (ø)
isolate.go	100.00% <0.00%> (ø)
function.go	100.00% <0.00%> (ø)
object_template.go	100.00% <0.00%> (ø)
cpuprofile.go
backports.go
cpuprofilenode.go
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8894db...d32e671. Read the comment docs.

[rogchap]

@dylanahsmith
I tried to create a build without this flag, but the windows build consistently failed.
I'm re-running the build against this PR to confirm that this is the cause.

[goenning]

It seems like some recent windwos builds are passing so maybe this is no the issue? I'm hoping this won't be reverted as I'm interested on using Intl as well!

[dylanahsmith]

It seems like some recent windwos builds are passing so maybe this is no the issue?

I think you are looking at the CI workflow runs, which are still being done against the pre-build windows without Intl support.

---

Since the original build output isn't accessible after some amount of time, I'll copy the command that failed in the build from a more recent attempt (https://github.com/rogchap/v8go/runs/4012312091?check_suite_focus=true)

g++ -MMD -MF obj/third_party/icu/icui18n/tzgnames.o.d -DU_I18N_IMPLEMENTATION -DUSE_AURA=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNO_UNWIND_TABLES -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -D_ATL_NO_OPENGL -D_WINDOWS -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DPSAPI_VERSION=2 -DWIN32 -D_SECURE_ATL -DWINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=NTDDI_WIN10_VB -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DHAVE_DLOPEN=0 -DUCONFIG_ONLY_HTML_CONVERSION=1 -DUCONFIG_USE_WINDOWS_LCID_MAPPING_API=0 -DU_CHARSET_IS_UTF8=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_ENABLE_TRACING=1 -DU_ENABLE_RESOURCE_TRACING=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=wchar_t -I../../v8 -Igen -I../../v8/third_party/icu/source/common -I../../v8/third_party/icu/source/i18n -fcf-protection=full -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fno-unwind-tables -fno-asynchronous-unwind-tables -Wa,-mbig-obj -fno-keep-inline-dllexport -m64 -march=x86-64 -msse3 -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -O2 -fno-ident -fdata-sections -ffunction-sections -g0 -municode -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-deprecated-declarations -Wno-comments -Wno-packed-not-aligned -Wno-attributes -Wno-format -Wno-unused-but-set-variable -Wno-stringop-overflow -Wno-missing-field-initializers -Wno-unused-parameter -Wno-invalid-offsetof /wd4005 /wd4068 /wd4267 /utf-8 -std=gnu++14 -fno-exceptions -frtti -Wno-narrowing -Wno-class-memaccess -Wno-subobject-linkage -c ../../v8/third_party/icu/source/i18n/tzgnames.cpp -o obj/third_party/icu/icui18n/tzgnames.o
g++: error: /wd4005: No such file or directory
g++: error: /wd4068: No such file or directory
g++: error: /wd4267: No such file or directory
g++: error: /utf-8: No such file or directory

Looks like the /wd4 prefixed command line arguments correspond to the Microsoft Visual Studio C++ (MSVC) compiler option /wdnnnn

Suppresses the compiler warning that is specified by nnnn.

For example, /wd4326 suppresses compiler warning C4326.
(Source: https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=msvc-160#remarks)

/utf-8 seems to be another MSVC flag (https://docs.microsoft.com/en-us/cpp/build/reference/utf-8-set-source-and-executable-character-sets-to-utf-8?view=msvc-160)

The problem is that we are trying to build using MinGW instead of MSVC. Currently, the windows build relies on V8 patches to support MinGW (as seen in deps/windows_x86_64), but this is brittle because, according to the chromium Windows build instructions

Chromium requires Visual Studio 2017 (>=15.7.2) to build, but VS2019 (>=16.0.0) is preferred.

Unfortunately, it looks like golang doesn't currently support linking MSVC object files (golang/go#20982). If I understand correctly, that would be why MinGW was used with V8 patches to pre-build the static V8 library on Windows. If MSVC were used to build v8, then it seems like it would need to be built as a dynamic library (DLL) that would need to be distributed with the executable that Go builds in order to use v8go on Windows. This is why the PR that introduced the V8 windows patches (#102) says

This PR adds the bits required to provide a prebuilt static V8 library on Windows, offering basically the same level of end user convenience as on Linux/Mac (i.e. not having to deal with a dozen DLLs in a distribution).

[dylanahsmith]

cc @GustavoCaso

Unfortunately, it looks like golang doesn't currently support linking MSVC object files (golang/go#20982).

That issue does mention changes that had been submitted to add support for it (https://go-review.googlesource.com/q/MSVC+toolchain lists the submitted changes). Although, it isn't clear whether there is enough interest in that work actually getting merged.

Chromium requires Visual Studio 2017 (>=15.7.2) to build, but VS2019 (>=16.0.0) is preferred.

Looks like there was older documentation for building V8 that was clearer (https://chromium.googlesource.com/external/github.com/v8/v8.wiki/+/c62669e6c70cc82c55ced64faf44804bd28f33d5/Building-with-Gyp.md#mingw)

Building on MinGW is not officially supported, but it is possible. You even have two options:
Option 1: With Cygwin Installed
...
Option 2: Without Cygwin, just MinGW
...

In fact, there is even some compiler conditional V8 code code for MinGW (e.g. https://chromium.googlesource.com/v8/v8.git/+/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/include/v8config.h#364). However, I didn't find much activity to improve support for MinGW in V8. A search for submitted changes (https://chromium-review.googlesource.com/q/project:v8/v8+mingw) only brought up a single change, but it was recent and got accepted. I'm not sure why the work to patch V8 for the MSYS2 V8 package wasn't submitted upstream to V8, since that would reduce the maintenance burden compared to having to continually rebase these patches to apply them to newer versions of V8.

[dylanahsmith]

I'll merge this revert, since it resolves the inconsistency between the build configuration and what is actually built. That seems like a pre-requisite for making a build for #138 that is consistent with the others on master.