-
Notifications
You must be signed in to change notification settings - Fork 290
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
fix for issue#3130 - Compile example contracts failed on Windows #3135
Conversation
Thanks for your contribution. |
Codecov Report
@@ Coverage Diff @@
## master #3135 +/- ##
==========================================
+ Coverage 32.15% 32.23% +0.09%
==========================================
Files 503 503
Lines 46298 46311 +13
Branches 21235 21245 +10
==========================================
+ Hits 14882 14926 +44
- Misses 17397 17453 +56
+ Partials 14019 13932 -87
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
please |
different topic - the unittest coverage seems pretty low, any existing ticket to bump it up a bit? I can probably help too @jolestar |
@lerencao Move compiler has a new version and is this fix still needed? |
CRLF or LF can be solved by set your editor to save newline as window's CRLF or unix's LF. |
If you intend to make move support windows' CRLF, you can fire the PR into diem repo to make move parser support CRLF parsing |
Thanks for the comments, @lerencao. I agree a better fix could be done on Diem side. Before that happens though, it does impact Windows user experience, especially for developers new to Starcoin. I can close this PR if you guys prefer not to merge it. |
The preferred way is:
|
vm/compiler/src/lib.rs
Outdated
/// and perform the conversion to Unix stype (LF) if yes | ||
fn windows_line_ending_to_unix(text: &str) -> String { | ||
let mut converted = String::new(); | ||
for c in text.chars() { |
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.
you can just replace "\r\n" with "\n", '\r' alone is still allowed I think.
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.
@zhutoulala Can you help to resolve this and rebase this pr?
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.
Hello. \r
is usually used with \n
in most cases. It alone is still allowed, it means to return to the beginning of the current line though, so I doubt if anyone wants to do that in move. I can update this PR with a quick .replace("\r\n", "\n")
but it could be a bit slower. Let me know if that's what you want.
Unfortunately editor doesn't get a chance to play here, all move files under vm/stdlib/modules and examples would have the Windows line ending once git clone on Windows using default settings |
@lerencao I try on windows and ensure this. |
add check and conversion from Windows style line ending (CRLF) to Unix stype (LF) for Move contracts