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

test: fix test-zlib-unused-weak on V8 8.2 #152

Closed

Conversation

mmarchini
Copy link

Ref: https://chromium-review.googlesource.com/c/v8/v8/+/1997438
Ref: https://chromium-review.googlesource.com/c/v8/v8/+/2010107
Ref: #144
Signed-off-by: Matheus Marchini mmarchini@netflix.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-ci and others added 30 commits March 13, 2020 05:55
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Richard Lau <riclau@uk.ibm.com>
Co-authored-by: Ujjwal Sharma <ryzokuken@igalia.com>
Original commit message:

    [testrunner] delete ancient junit compatible format support

    Testrunner has ancient support for JUnit compatible XML output.

    This CL removes this old feature.

    R=mstarzinger@chromium.org,jgruber@chromium.org,jkummerow@chromium.org
    CC=​machenbach@chromium.org

    Bug: v8:8728
    Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0
    Reviewed-on: https://chromium-review.googlesource.com/c/1430065
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
    Commit-Queue: Tamer Tas <tmrts@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59045}

Refs: v8/v8@bd019bd
Patch V8 (compiler/js-heap-broker.cc) to remove the use of an optional
property, which is a fairly new C++ feature, since that requires a newer
XCode version than the minimum requirement in BUILDING.md and thus
breaks CI.
Fixes a compilation issue on some platforms
This should be semver-patch since actual invocation is version
conditional.
There is a bug in the most recent version of VS2015 that affects v8.h
and therefore prevents compilation of addons.

Refs: https://stackoverflow.com/q/38378693
Bump minimum version of ICU needed to build node to 65.

Refs: v8/v8@74bf96e
Original commit message:

    Remove unnecessary export, which happens to break MSVC DLL builds.

    Change-Id: I47c9211274cefd26bde6bd93aa7503e022df4357
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2042874
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Commit-Queue: Bill Ticehurst <billti@microsoft.com>
    Cr-Commit-Position: refs/heads/master@{#66179}

Refs: v8/v8@1e36e21
Original commit message:

    [torque] fix build on VS2017

    Node.js build fails on VS2017 without these headers, see the downstream
    issue (nodejs#128).

    Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
    Co-authored-by: gengjiawen <technicalcute@gmail.com>
    Change-Id: I771eab435dce5cf548581f3acd78681180c77692
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2093951
    Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
    Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#66661}

Refs: v8/v8@931bdbd
PR-URL: nodejs/node#26685
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Patch V8 (wasm/wasm-module.cc) to remove const qualifier from type
passed to template call of `OwnedVector::Of`. Xcode 8 can't convert
'OwnedVector<unsigned char>' to 'OwnedVector<const unsigned char>' when
returning from a function (which is likely a bug on Xcode, considering
this worked on the prior version of Xcode as well as newer versions).
This workaround shouldn't affect the application, since the const
qualifier is preserved in the AsmJsOffsetInformation::encoded_offset_.

There's also a V8 test passing a const-qualified type to ::Of, but since
we don't test V8 on Xcode 8, it should be fine to leave it as is.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>
These methods will be removed in V8 8.1, hence we need to stop
overriding them.
This commit replaces Symbol::Name() with
Symbol::Description().

Fixes: nodejs/node#30916
The {SetExpectInlineWasm} method is deprecated and non-functional since
V8 v8.1.
Thus node should stop calling it, so that it can be fully removed in a
future v8 version.
This removes uses of the "IsWebAssemblyCompiledModule" method, which is
deprecated in V8 v8.1 and will be removed in v8.2.
We could replace it by "IsWasmModuleObject", but since it's unused in
node anyway, I just remove the definition.
V8 is removing support for serializing wasm modules via the value
serializer. Once this is complete (https://crrev.com/c/2013110), we can
re-add this test.
GitHub Actions doesn't have enough memory to complete an ASAN+Debug with
V8 8.1. Don't take ASAN into account on CI results until we find a
workaround, or until we upgrade to V8 8.2.
until 72fc962b4db88af63a50e8f0b0a33313eb2e08ff
until fe6bd3019d47c8522e2cad2bb52fd82dce906485
until 4c7c6f732cb607676bec1c9acb570e91ddfc507d
@targos
Copy link
Member

targos commented Mar 14, 2020

Don't hesitate to push fixes to canary base. It's not easy to PR this repo because it's reset once a day by CI.

@targos
Copy link
Member

targos commented Mar 14, 2020

And if we want to open pull requests, maybe we should do that against canary-base in the main repo? that branch is less likely to change every day.

@mmarchini
Copy link
Author

Cherry-picked on canary-base.

And if we want to open pull requests, maybe we should do that against canary-base in the main repo? that branch is less likely to change every day.

Agreed, it's probably better to open PRs there.

Copy link

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could you please just push this as @targos outlined?

@mmarchini
Copy link
Author

Yes, I sent the comment before pushing 😅

Just waiting for make test to run after rebase+cherry pick, so I'll push in a few minutes.

@mmarchini
Copy link
Author

nodejs/node@46b4399

@mmarchini mmarchini closed this Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants