Need help to unbreak canary builds

[targos]

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?

[addaleax]

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]

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]

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]

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

[danbev]

@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]

@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]

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

[targos]

@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]

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]

/cc @srl295 ?

[srl295]

@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]

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