-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
fs: fix cpSync crash on utf characters #54653
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54653 +/- ##
==========================================
+ Coverage 88.03% 88.04% +0.01%
==========================================
Files 652 652
Lines 183763 183779 +16
Branches 35860 35865 +5
==========================================
+ Hits 161774 161817 +43
+ Misses 15237 15228 -9
+ Partials 6752 6734 -18
|
1ba6b97
to
d78ea43
Compare
It looks like it's consistently failing to build on our macOS GHA CI |
Possibly related to this
which indicates no space left on the device, I assume that is why it fails |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8362525
to
52d0c3a
Compare
fe5f947
to
a09e59f
Compare
6b3781d
to
65ed3b1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
65ed3b1
to
f65abcb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
1669d2a
to
7cd8eb1
Compare
7cd8eb1
to
f2f749f
Compare
f2f749f
to
32faca8
Compare
With the requested CI env, I can finally reproduce the segfaults. The original reported issue Simply constructing from |
This is because the error throwing functions convert the messages into JS strings - technically, it is expecting Latin1 Line 121 in eab97b5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I opened a separate PR to fix the decoding in error throwing utils #55024
PR-URL: #54653 Fixes: #54476 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 77ca5ca |
PR-URL: #54653 Fixes: #54476 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#54653 Fixes: nodejs#54476 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: #54476
Refs: #54653 (comment)