Need help to unbreak canary builds #69

targos · 2018-07-02T19:40:29Z

The canary branch is currently broken because 714c686#diff-91081f7e2c43274462308861f4191e3b is no longer valid after recent changes to test/cctest/node_test_fixture.h on master.
Would someone be able to help me unbreak it?

The text was updated successfully, but these errors were encountered:

addaleax · 2018-07-02T19:46:21Z

Is d6f7a32 the conflicting change? I think it might be enough for now to remove the first part of the diff linked above in test/cctest/node_test_fixture.h?

targos · 2018-07-02T19:51:09Z

Is d6f7a32 the conflicting change?

Yes.

I think it might be enough for now to remove the first part of the diff linked above in test/cctest/node_test_fixture.h?

You mean to not add the two lines in SetUp? I tried to do that and the test crashes:

[==========] Running 74 tests from 9 test cases.
[----------] Global test environment set-up.
[----------] 14 tests from AliasBufferTest
[ RUN      ] AliasBufferTest.Uint8Array
Segmentation fault (core dumped)
Makefile:238: recipe for target 'cctest' failed
make: *** [cctest] Error 139

addaleax · 2018-07-02T20:01:16Z

Ok, I’ll look into it … should be doable 🤞

(Edit: I need a debug build :/ This is going to take a while, maybe rather tomorrow or so …)

danbev · 2018-07-03T03:45:26Z

Sorry about this! @addaleax Let me know if there is anything I can help out with. Thanks

This commit adds an InitializeV8Platform function which calls v8_platform's Initialize to create the NodePlatform and also set the structs members. When running cctests this functions was not being called (it is called from the Start function but that function is not called by the test fixture. The motivation for adding this is that I'm guessing that embedders would might need the ability to do the same thing. Refs: nodejs#69

danbev · 2018-07-03T07:59:33Z

@addaleax I took a look and have a suggestion linked above. Feel free to ignore you have already started looking into this. I did not mean to step on your toes here, just felt bad bad that you are having to spend time fixing issues that I caused.

addaleax · 2018-07-10T23:25:48Z

@danbev I’ll think about it in more detail (because we don’t seem to have a good story for this part of the API atm), but your fix definitely seems okay :)

addaleax · 2018-07-13T14:37:46Z

@targos Can you confirm that the linked patch works, at least as a temporary workaround?

This commit adds an InitializeV8Platform function which calls v8_platform's Initialize to create the NodePlatform and also set the structs members. When running cctests this functions was not being called (it is called from the Start function but that function is not called by the test fixture. The motivation for adding this is that I'm guessing that embedders would might need the ability to do the same thing. Refs: nodejs/node-v8#69

targos · 2018-07-13T15:17:17Z

@addaleax, @danbev. It does work, thanks!

I pushed the commit to canary: c538b12 and a subsequent one to fix a test (unrelated to this issue): 0f69779

targos · 2018-07-13T15:22:52Z

Now, lkgr has advanced and I'm experiencing a new issue!

I pushed the current state to canary-base: https://github.com/nodejs/node/commits/canary-base.

Something related to ICU changed upstream, probably related to this commit: v8/v8@3ba5445#diff-4eaebbcd1dc0da11e3a3ce488290bfc6

Here is one of the errors I get:

In file included from ../deps/v8/src/interface-descriptors.h:12,
                 from ../deps/v8/src/callable.h:9,
                 from ../deps/v8/src/code-factory.h:10,
                 from ../deps/v8/src/compiler/code-assembler.h:16,
                 from ../deps/v8/src/code-stub-assembler.h:11,
                 from ../deps/v8/src/builtins/builtins-arguments-gen.h:8,
                 from ../deps/v8/src/builtins/builtins-arguments-gen.cc:5:
../deps/v8/src/isolate.h:36:10: fatal error: unicode/uversion.h: No such file or directory
 #include "unicode/uversion.h"  // Define U_ICU_NAMESPACE.
          ^~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [deps/v8/gypfiles/v8_initializers.target.mk:167: /home/mzasso/git/nodejs/canary/out/Release/obj.target/v8_initializers/deps/v8/src/builtins/builtins-arguments-gen.o] Error 1

addaleax · 2018-07-13T15:24:25Z

/cc @srl295 ?

srl295 · 2018-07-13T16:12:30Z

@addaleax i'm concerned about the unconditional inclusion of the //third_party/icu deps folder. v8_enable_i18n_support is turned on but Node is changing which ICU is built. Seems like a v8 bug.

targos · 2018-07-13T16:36:44Z

I think I understand what the problem is. After that commit, when v8_enable_i18n_support is turned on, more targets need to include ICU, similar to how it's done here for the main target:
https://github.com/nodejs/node/blob/45732c7c19672869a7e517e252c83b371a863bee/deps/v8/gypfiles/v8.gyp#L1820-L1824

srl295 · 2018-07-13T16:42:42Z

That makes sense. The v8 change is to move more stuff to C++. more stuff off without ICU..

…

On Fri, Jul 13, 2018 at 9:36 AM Michaël Zasso ***@***.***> wrote: I think I understand what the problem is. After that commit, when v8_enable_i18n_support is turned on, more targets need to include ICU, similar to how it's done here for the main target: https://github.com/nodejs/node/blob/45732c7c19672869a7e517e252c83b371a863bee/deps/v8/gypfiles/v8.gyp#L1820-L1824 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0Mszoz5oFGKyLik66DzxSNudp9FuHAks5uGMyggaJpZM4U_1ns> .

targos · 2018-07-13T16:53:14Z

/cc @nodejs/gyp

Tentative fix: targos/node@adf23f2

Now I see this error:

  g++ '-DV8_GYP_BUILD' '-DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=0' '-DV8_TARGET_ARCH_X64' '-DV8_EMBEDDER_STRING="-node.1"' '-DENABLE_DISASSEMBLER' '-DV8_PROMISE_INTERNAL_FIELD_COUNT' '-Dv8_promise_internal_field_count' '-DV8_INTL_SUPPORT' '-DV8_CONCURRENT_MARKING' '-DDISABLE_UNTRUSTED_CODE_MITIGATIONS' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' -I../deps/v8 -I../. -I/home/mzasso/git/nodejs/canary/out/Release/obj/gen -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++1y -MMD -MF /home/mzasso/git/nodejs/canary/out/Release/.deps//home/mzasso/git/nodejs/canary/out/Release/obj.target/v8_base/deps/v8/src/objects/intl-objects.o.d.raw   -c -o /home/mzasso/git/nodejs/canary/out/Release/obj.target/v8_base/deps/v8/src/objects/intl-objects.o ../deps/v8/src/objects/intl-objects.cc
In file included from ../deps/v8/src/debug/debug.h:19,
                 from ../deps/v8/src/wasm/wasm-objects.h:9,
                 from ../deps/v8/src/wasm/wasm-objects-inl.h:8,
                 from ../deps/v8/src/objects/shared-function-info-inl.h:13,
                 from ../deps/v8/src/objects/templates-inl.h:11,
                 from ../deps/v8/src/objects/map-inl.h:16,
                 from ../deps/v8/src/contexts-inl.h:12,
                 from ../deps/v8/src/objects-inl.h:21,
                 from ../deps/v8/src/objects/intl-objects.cc:19:
../deps/v8/src/frames.h: In member function ‘void v8::internal::InnerPointerToCodeCache::Flush()’:
../deps/v8/src/frames.h:46:41: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct v8::internal::InnerPointerToCodeCache::InnerPointerToCodeCacheEntry’; use assignment or value-initialization instead [-Wclass-memaccess]
     memset(&cache_[0], 0, sizeof(cache_));
                                         ^
../deps/v8/src/frames.h:35:10: note: ‘struct v8::internal::InnerPointerToCodeCache::InnerPointerToCodeCacheEntry’ declared here
   struct InnerPointerToCodeCacheEntry {
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../deps/v8/src/objects/intl-objects.cc: In function ‘void v8::internal::BuildLanguageTagRegexps(v8::internal::Isolate*)’:
../deps/v8/src/objects/intl-objects.cc:1350:73: error: invalid use of incomplete type ‘class icu_62::RegexMatcher’
       icu::UnicodeString::fromUTF8(language_singleton_regexp), 0, status);
                                                                         ^
In file included from ../deps/v8/src/api.h:13,
                 from ../deps/v8/src/objects/intl-objects.cc:14:
../deps/v8/src/isolate.h:40:7: note: forward declaration of ‘class icu_62::RegexMatcher’
 class RegexMatcher;
       ^~~~~~~~~~~~
../deps/v8/src/objects/intl-objects.cc:1352:67: error: invalid use of incomplete type ‘class icu_62::RegexMatcher’
       icu::UnicodeString::fromUTF8(language_tag_regexp), 0, status);
                                                                   ^
In file included from ../deps/v8/src/api.h:13,
                 from ../deps/v8/src/objects/intl-objects.cc:14:
../deps/v8/src/isolate.h:40:7: note: forward declaration of ‘class icu_62::RegexMatcher’
 class RegexMatcher;
       ^~~~~~~~~~~~
../deps/v8/src/objects/intl-objects.cc:1354:71: error: invalid use of incomplete type ‘class icu_62::RegexMatcher’
       icu::UnicodeString::fromUTF8(language_variant_regexp), 0, status);
                                                                       ^
In file included from ../deps/v8/src/api.h:13,
                 from ../deps/v8/src/objects/intl-objects.cc:14:
../deps/v8/src/isolate.h:40:7: note: forward declaration of ‘class icu_62::RegexMatcher’
 class RegexMatcher;
       ^~~~~~~~~~~~
make[1]: *** [deps/v8/gypfiles/v8_base.target.mk:633: /home/mzasso/git/nodejs/canary/out/Release/obj.target/v8_base/deps/v8/src/objects/intl-objects.o] Error 1
rm fc4b5a95735baed148f443409c7e3e4931bc56c2.intermediate d3bc80f7856fcfa3d3d7905265a41fb78f6975d6.intermediate 6c8bbb6e9dc9d22d3b7b17a57b2f4f14600c74e4.intermediate
make: *** [Makefile:87: node] Error 2

srl295 · 2018-07-13T17:14:10Z

for reference, v8's bump to 62.1 https://chromium-review.googlesource.com/c/v8/v8/+/1111857#message-4f92f63a195703cf7cc65aea74efa2f9b8163f2d

srl295 · 2018-07-13T22:55:50Z

@targos i finally realized there's a test-v8-intl …

This commit adds an InitializeV8Platform function which calls v8_platform's Initialize to create the NodePlatform and also set the structs members. When running cctests this functions was not being called (it is called from the Start function but that function is not called by the test fixture. The motivation for adding this is that I'm guessing that embedders would might need the ability to do the same thing. Refs: nodejs/node-v8#69

targos · 2018-07-21T11:02:06Z

Ping @nodejs/v8-update. Would someone be able to help with this forward declaration issue?

addaleax · 2018-07-21T12:31:53Z

@targos I couldn’t reproduce that failure when building the current canary … sounds like adding #include "unicode/regex.h" in the failing file might be enough though?

targos · 2018-07-21T12:41:43Z

@addaleax you can try with canary-base or my branch that has one additional commit.

addaleax · 2018-07-21T14:22:11Z

diff --git a/src/heap_utils.cc b/src/heap_utils.cc
index 2d339c580fa0..ead3716daf44 100644
--- a/src/heap_utils.cc
+++ b/src/heap_utils.cc
@@ -76,7 +76,7 @@ class JSGraph : public EmbedderGraph {
     return n;
   }
 
-  void AddEdge(Node* from, Node* to) override {
+  void AddEdge(Node* from, Node* to, const char* name) override {
     edges_[from].insert(to);
   }
 
diff --git a/tools/icu/icu-generic.gyp b/tools/icu/icu-generic.gyp
index a50f12d7940b..2acc95b409b8 100644
--- a/tools/icu/icu-generic.gyp
+++ b/tools/icu/icu-generic.gyp
@@ -32,7 +32,6 @@
       'direct_dependent_settings': {
         'defines': [
           'UCONFIG_NO_SERVICE=1',
-          'UCONFIG_NO_REGULAR_EXPRESSIONS=1',
           'U_ENABLE_DYLOAD=0',
           'U_STATIC_IMPLEMENTATION=1',
           'U_HAVE_STD_STRING=1',

seems to do the trick for me on your branch 🎉

targos · 2018-07-21T14:57:02Z

Awesome, thank you! I had to fix a few other things but finally got it to compile :)

Update build: https://ci.nodejs.org/view/MyJobs/job/node-update-v8-canary/531/console

This commit adds an InitializeV8Platform function which calls v8_platform's Initialize to create the NodePlatform and also set the structs members. When running cctests this functions was not being called (it is called from the Start function but that function is not called by the test fixture. The motivation for adding this is that I'm guessing that embedders would might need the ability to do the same thing. Refs: #69

targos · 2018-07-21T16:24:51Z

The canary is back!