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.

Sign up for GitHub

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

Jump to bottom

v8: load_objects_from_file() using utf-8 #30218

Closed
wants to merge 1 commit into from
Closed

v8: load_objects_from_file() using utf-8 #30218

wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 2, 2019

This PR uses io.open(encoding="utf-8") to resolve a UnicodeDecodeError reported at nodejs/code-and-learn#99 (comment)

This can be replicated by running docker build . using the following:

Dockerfile 
FROM ubuntu:bionic

# Install tools
RUN apt-get update
RUN apt-get install -y build-essential python3 python3-pip ninja-build make g++ vim git sudo

# Expose ports used by net tests
ENV NODE_COMMON_PORT=12346
EXPOSE 12346
# debug port used by v8 debugger
EXPOSE 5858
# debug port used by v8 insepctor
EXPOSE 9229

# Set up user node-dev: the tests fail when run by the root
RUN adduser --disabled-password --gecos '' node-dev
RUN adduser node-dev sudo
RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
USER node-dev
WORKDIR /home/node-dev
ENV HOME /home/node-dev

# Clone the code and build
RUN git clone --depth=1 https://github.com/nodejs/node.git
WORKDIR /home/node-dev/node
RUN python3 -m pip install setuptools
RUN python3 ./configure --ninja
ENV BUILD_WITH ninja
RUN make -j2 test
Checklist

@nodejs-github-bot nodejs-github-bot added post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Nov 2, 2019
@cclauss cclauss mentioned this pull request Nov 2, 2019
Closed
3 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26290/

@targos
Copy link
Member

targos commented Nov 2, 2019

do you want me to upstream the change?

@cclauss
Copy link
Contributor Author

cclauss commented Nov 2, 2019 •
edited
Loading

Yes, please. Should we wait for our Jenkins tests to pass first? Your call.

It works on the Dockerfile when I do a COPY gen-postmortem-metadata.py ./deps/v8/tools/ with these mods.

@cclauss cclauss mentioned this pull request Nov 2, 2019
Closed
1 task
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Nov 2, 2019
This was referenced Nov 2, 2019
Merged
Closed
@targos
Copy link
Member

targos commented Nov 2, 2019

https://chromium-review.googlesource.com/c/v8/v8/+/1895560

@cclauss
Copy link
Contributor Author

cclauss commented Nov 3, 2019

Nice! Any hints on how to get that arm-fanned to pass?

@targos
Copy link
Member

targos commented Nov 3, 2019

CL landed. I'll update this to be a cherry pick

legendecas
legendecas approved these changes Nov 3, 2019
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM

deps/v8/tools/gen-postmortem-metadata.py Outdated Show resolved Hide resolved
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
c21d28e 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd
@targos targos force-pushed the fix-gen-postmortem-metadata.py branch from f4122ca to c21d28e Compare November 3, 2019 16:11
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26301/

@cclauss
Copy link
Contributor Author

cclauss commented Nov 4, 2019

Landed in c80771d

@cclauss cclauss closed this Nov 4, 2019
@cclauss cclauss deleted the fix-gen-postmortem-metadata.py branch November 4, 2019 10:39
cclauss added a commit that referenced this pull request Nov 4, 2019
@cclauss
deps: V8: cherry-pick a7dffcd767be
c80771d 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 5, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
46c9194 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Nov 5, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
f88fdd0 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 8, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
10af006 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: nodejs#30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
6df1e25 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
ad4e217 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
b1529c6 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 17, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
493311b 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: nodejs#30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
@cclauss @MylesBorins
deps: V8: cherry-pick a7dffcd767be
5a1ad57 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

Backport-PR-URL: #30513
PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 21, 2019
Merged
targos pushed a commit to targos/node that referenced this pull request Dec 5, 2019
@cclauss @targos
deps: V8: cherry-pick a7dffcd767be
5726545 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: nodejs#30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit to targos/node that referenced this pull request Jan 7, 2020
@cclauss @MylesBorins
deps: V8: cherry-pick a7dffcd767be
15794c9 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

PR-URL: nodejs#30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
@cclauss @MylesBorins
deps: V8: cherry-pick a7dffcd767be
6e3828f 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

Backport-PR-URL: #30109
PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@cclauss @BethGriggs
deps: V8: cherry-pick a7dffcd767be
593d989 
Original commit message:

    [postmortem] Load files using utf-8 to support Python 3

    Change-Id: I174d38cc33210c07d1a7596627e1b2d21bb06313
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1895560
    Reviewed-by: Mathias Bynens <[email protected]>
    Commit-Queue: Michaël Zasso <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64717}

Refs: v8/v8@a7dffcd

Backport-PR-URL: #30109
PR-URL: #30218
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@sam-github sam-github Awaiting requested review from sam-github

Assignees
No one assigned
Labels
post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. python PRs and issues that require attention from people who are familiar with Python. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cclauss @nodejs-github-bot @targos @cjihrig @joyeecheung @devnexen @richardlau @legendecas