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

tools: check-imports using utf-8 #30220

Closed
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 on Python 3 after the fix in #30218 has been applied.

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 the tools Issues and PRs related to the tools directory. label Nov 2, 2019
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Nov 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thanks!

(And makes me wonder whether it is useful to add an dockerfile to BUILDING.md since it's pretty easy to understand what's necessary to have a working dev environment from a glance)

@nodejs-github-bot
Copy link
Collaborator

cclauss added a commit that referenced this pull request Nov 5, 2019
PR-URL: #30220
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@cclauss
Copy link
Contributor Author

cclauss commented Nov 5, 2019

Landed in aa4e54e

@targos is this in time for inclusion in #30262 ?

@cclauss cclauss closed this Nov 5, 2019
@cclauss cclauss deleted the fix-tools-check-imports.py branch November 5, 2019 10:30
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #30220
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
targos pushed a commit that referenced this pull request Dec 1, 2019
PR-URL: #30220
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
PR-URL: #30220
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants