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: fix build at non-English windows #30492

Closed
wants to merge 1 commit into from
Closed

tools: fix build at non-English windows #30492

wants to merge 1 commit into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented Nov 15, 2019

Fixes: #25885

There are some non-ASCII characters in v8's code, for example:

// This is fine.™

This causes build failure at non-English windows. This PR adds /utf-8 compiler option to fix it.

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 tools Issues and PRs related to the tools directory. label Nov 15, 2019
@codecov-io
Copy link

Codecov Report

Merging #30492 into master will decrease coverage by 2.8%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #30492      +/-   ##
==========================================
- Coverage   96.06%   93.26%   -2.81%     
==========================================
  Files         185      404     +219     
  Lines       61301    98276   +36975     
==========================================
+ Hits        58889    91656   +32767     
- Misses       2412     6620    +4208
Impacted Files Coverage Δ
lib/internal/net.js 76.71% <0%> (-23.29%) ⬇️
lib/internal/inspector_async_hook.js 86.11% <0%> (-13.89%) ⬇️
lib/internal/v8_prof_processor.js 92.5% <0%> (-5%) ⬇️
lib/internal/fs/utils.js 94.93% <0%> (-1.64%) ⬇️
lib/internal/url.js 96.21% <0%> (-1.27%) ⬇️
lib/fs.js 94.11% <0%> (-0.77%) ⬇️
lib/crypto.js 96.8% <0%> (-0.71%) ⬇️
lib/internal/child_process.js 94.79% <0%> (-0.48%) ⬇️
lib/assert.js 97.98% <0%> (-0.45%) ⬇️
lib/domain.js 97.97% <0%> (-0.37%) ⬇️
... and 251 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4a089a...6aae756. Read the comment docs.

@pd4d10
Copy link
Contributor Author

pd4d10 commented Nov 17, 2019

Found another issue probably related: #30461

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 18, 2019
@nodejs-github-bot
Copy link
Collaborator

danbev pushed a commit that referenced this pull request Nov 19, 2019
PR-URL: #30492
Fixes: #25885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@danbev
Copy link
Contributor

danbev commented Nov 19, 2019

Landed in 8fbbab8.

@danbev danbev closed this Nov 19, 2019
BridgeAR pushed a commit that referenced this pull request Nov 19, 2019
PR-URL: #30492
Fixes: #25885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
@pd4d10 pd4d10 deleted the patch-2 branch December 1, 2019 03:53
targos pushed a commit that referenced this pull request Dec 1, 2019
PR-URL: #30492
Fixes: #25885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[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: #30492
Fixes: #25885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure on Windows caused by UTF-8 characters
5 participants