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: fix call of its own deprecated API #25148

Closed

Conversation

sam-github
Copy link
Contributor

v8.h contains calls to the deprecated IsCompressible() API causing a
deprecated warning for every include of v8.h. The warning is disabled
for clang, but GNU compiler chains. Developer builds and ci build logs
are filled with pages and pages of warnings at the moment.

@targos @ofrobots @nodejs/v8 can any of you upstream this? The procedure to do so is complex for me, technically and legally. I can do it, but if its trivial for you it would be great if this could be fixed.

Btw, I don't have clang, but I suspect that it may support GCC pragmas, in which case the easier fix would to be to just change #pragma clang to #pragma GCC instead of a new if block for GCC.

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-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Dec 20, 2018
@sam-github sam-github force-pushed the fix-v8-internal-warnings branch from 85a170e to 7bfcc1d Compare December 20, 2018 16:27
@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2018

Shouldn't we be bumping the V8 node patch level since we're changing things in V8 code?

@BridgeAR
Copy link
Member

@sam-github this is already fixed in newer V8 versions.

@sam-github
Copy link
Contributor Author

@BridgeAR Great, do you know where to find the commit? We should be able to cherry-pick it back to master, right?

Original commit message:

    [api] Deprecate ExternalStringResourceBase::IsCompressible

    R=yangguo@chromium.org

    Bug: v8:8238
    Change-Id: Ia59aefc54c2e9f4fa3348c42fb45e7fadab8ee76
    Reviewed-on: https://chromium-review.googlesource.com/c/1349231
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57788}

Refs: v8/v8@26b145a
@sam-github sam-github force-pushed the fix-v8-internal-warnings branch from 7bfcc1d to 97671c3 Compare December 21, 2018 23:50
@sam-github
Copy link
Contributor Author

Found it. Thanks for git node v8 backport @joyeecheung

PTAL @ofrobots @hashseed @targos

I don't know if we care about the difference between deprecated, and deprecated soon, but we could rewrite that small amount of the change if we do.

@sam-github
Copy link
Contributor Author

If you wonder why it matters... It took me more than a minute just to scroll down in Chrome to the end of this travis build log because its so huge with v8 compiler warnings:

https://travis-ci.com/nodejs/node/jobs/166808240

@addaleax
Copy link
Member

If addons use this – which I would not expect – this is a breaking change, because it makes every external string appear “cacheable”, right? I’m not sure what exactly V8 does based on this flag, though.

Btw, I don't have clang, but I suspect that it may support GCC pragmas, in which case the easier fix would to be to just change #pragma clang to #pragma GCC instead of a new if block for GCC.

That might indeed be easier…

@targos
Copy link
Member

targos commented Dec 22, 2018

@addaleax this commit is only relevant for master so it's fine.

@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

Landed in 59fa7f1

@sam-github sam-github closed this Dec 27, 2018
@sam-github sam-github deleted the fix-v8-internal-warnings branch December 27, 2018 22:36
sam-github added a commit that referenced this pull request Dec 27, 2018
Original commit message:

    [api] Deprecate ExternalStringResourceBase::IsCompressible

    R=yangguo@chromium.org

    Bug: v8:8238
    Change-Id: Ia59aefc54c2e9f4fa3348c42fb45e7fadab8ee76
    Reviewed-on: https://chromium-review.googlesource.com/c/1349231
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57788}

Refs:

v8/v8@26b145a

PR-URL: #25148
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Original commit message:

    [api] Deprecate ExternalStringResourceBase::IsCompressible

    R=yangguo@chromium.org

    Bug: v8:8238
    Change-Id: Ia59aefc54c2e9f4fa3348c42fb45e7fadab8ee76
    Reviewed-on: https://chromium-review.googlesource.com/c/1349231
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57788}

Refs:

v8/v8@26b145a

PR-URL: nodejs#25148
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants