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

[v19.x backport] deps: V8: cherry-pick cb30b8e17429 #47307

Conversation

RaisinTen
Copy link
Contributor

Original commit message:

Fix compilation error in platform.h for ASAN
The last two operands of the conditional expression needs to be
of the same type to compile.

Change-Id: Ib6cba4acb1238394910c650c776a7fd1ee93721e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4306802
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#86235}

Refs: v8/v8@cb30b8e
Refs: #43370

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Mar 30, 2023
@RaisinTen RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@targos
Copy link
Member

targos commented Mar 31, 2023

I just landed V8 11.3 on main and it contains this fix. You might want to backport to v19.x-staging instead.

@RaisinTen RaisinTen changed the base branch from main to v19.x-staging April 3, 2023 09:37
@RaisinTen RaisinTen changed the title deps: V8: cherry-pick cb30b8e17429 [v19.x] deps: V8: cherry-pick cb30b8e17429 Apr 3, 2023
Original commit message:

    Fix compilation error in platform.h for ASAN
    The last two operands of the conditional expression needs to be
    of the same type to compile.

    Change-Id: Ib6cba4acb1238394910c650c776a7fd1ee93721e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4306802
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86235}

Refs: v8/v8@cb30b8e
Refs: nodejs#43370
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the fix-compilation-error-in-platform.h-for-ASAN branch from 995715a to 97f9fbc Compare April 3, 2023 09:54
@RaisinTen
Copy link
Contributor Author

@targos done, could you PTAL?

@RaisinTen RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2023
@RaisinTen RaisinTen changed the title [v19.x] deps: V8: cherry-pick cb30b8e17429 [v19.x backport] deps: V8: cherry-pick cb30b8e17429 Apr 3, 2023
@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor Author

@targos it seems like the only remaining CI failure on this PR is coming from https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5269/:

Testing hash seed
export PATH="/usr/bin:/home/iojs/build/workspace/node-test-commit-v8-linux/depot_tools:/home/iojs/nghttp2/src:/home/iojs/wrk:/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" && \
	/usr/bin/python3.9 deps/v8/tools/run-tests.py --gn --arch=ppc64 --progress=dots --timeout=120 intl \
			mjsunit cctest debugger inspector message preparser \
			--json-test-results /home/iojs/build/workspace/node-test-commit-v8-linux/v8-tap.json --slow-tests-cutoff 1000000
Traceback (most recent call last):
  File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/run-tests.py", line 11, in <module>
    from testrunner import standard_runner
  File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/standard_runner.py", line 16, in <module>
    from testrunner.testproc.progress import ResultsTracker, ProgressProc
  File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/testproc/progress.py", line 9, in <module>
    from testrunner.testproc.resultdb import ResultDBIndicator
  File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/tools/testrunner/testproc/resultdb.py", line 8, in <module>
    import requests
ModuleNotFoundError: No module named 'requests'
make: *** [Makefile:713: test-v8] Error 1
Build step 'Execute shell' marked build as failure
Sending e-mails to: michael_dawson@ca.ibm.com vieuxtech@gmail.com info@bnoordhuis.nl
Recording test results
ERROR: StepPublish JUnit test result reportfailed: No test report files were found. Configuration error?
Collecting metadata...
Metadata collection done.
Notifying upstream projects of job completion
Finished: FAILURE

which looks totally unrelated. Is this a known issue on v19.x-staging? Should we land this PR manually irrespective of that failure?

@targos
Copy link
Member

targos commented Apr 4, 2023

Yes, it's a known issue fixed by #47239

@targos targos added the v19.x label Apr 4, 2023
RaisinTen added a commit that referenced this pull request Apr 5, 2023
Original commit message:

    Fix compilation error in platform.h for ASAN
    The last two operands of the conditional expression needs to be
    of the same type to compile.

    Change-Id: Ib6cba4acb1238394910c650c776a7fd1ee93721e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4306802
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86235}

Refs: v8/v8@cb30b8e
Refs: #43370
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47307
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RaisinTen
Copy link
Contributor Author

Landed in 5ddd1c9

@RaisinTen RaisinTen closed this Apr 5, 2023
@RaisinTen RaisinTen deleted the fix-compilation-error-in-platform.h-for-ASAN branch April 5, 2023 12:30
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
Original commit message:

    Fix compilation error in platform.h for ASAN
    The last two operands of the conditional expression needs to be
    of the same type to compile.

    Change-Id: Ib6cba4acb1238394910c650c776a7fd1ee93721e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4306802
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86235}

Refs: v8/v8@cb30b8e
Refs: #43370
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47307
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants