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

fix for issue#3130 - Compile example contracts failed on Windows #3135

Merged
merged 7 commits into from
Feb 10, 2022

Conversation

zhutoulala
Copy link
Contributor

add check and conversion from Windows style line ending (CRLF) to Unix stype (LF) for Move contracts

@jolestar
Copy link
Member

jolestar commented Jan 4, 2022

Thanks for your contribution.

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #3135 (d1e4125) into master (51fc701) will increase coverage by 0.09%.
The diff coverage is 21.43%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 32.23% <21.43%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
vm/compiler/src/lib.rs 39.64% <21.43%> (-1.17%) ⬇️
vm/types/src/language_storage_ext.rs 50.00% <0.00%> (-50.00%) ⬇️
...m/types/src/account_config/events/config_change.rs 66.67% <0.00%> (-33.33%) ⬇️
network/src/worker.rs 30.65% <0.00%> (-19.35%) ⬇️
cmd/miner_client/src/solver.rs 16.67% <0.00%> (-16.66%) ⬇️
vm/types/src/on_chain_config/vm_config.rs 35.00% <0.00%> (-11.66%) ⬇️
...ccount_config/resources/module_upgrade_strategy.rs 34.29% <0.00%> (-8.57%) ⬇️
node/src/network_service_factory.rs 20.84% <0.00%> (-8.33%) ⬇️
abi/decoder/src/lib.rs 1.58% <0.00%> (-7.87%) ⬇️
vm/types/src/transaction_metadata.rs 56.72% <0.00%> (-7.46%) ⬇️
... and 101 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 51fc701...d1e4125. Read the comment docs.

@jolestar
Copy link
Member

jolestar commented Jan 5, 2022

please cargo fmt the code @zhutoulala

@zhutoulala
Copy link
Contributor Author

different topic - the unittest coverage seems pretty low, any existing ticket to bump it up a bit? I can probably help too @jolestar

@jolestar
Copy link
Member

jolestar commented Jan 5, 2022

different topic - the unittest coverage seems pretty low, any existing ticket to bump it up a bit? I can probably help too @jolestar

Welcome, I write a issue for this #3136

@jolestar
Copy link
Member

jolestar commented Jan 7, 2022

@lerencao Move compiler has a new version and is this fix still needed?

@nanne007
Copy link
Member

CRLF or LF can be solved by set your editor to save newline as window's CRLF or unix's LF.
It's not a problem of Move.

@nanne007
Copy link
Member

If you intend to make move support windows' CRLF, you can fire the PR into diem repo to make move parser support CRLF parsing

@zhutoulala
Copy link
Contributor Author

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.

@nanne007
Copy link
Member

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:

CRLF or LF can be solved by set your editor to save newline as window's CRLF or unix's LF.

/// 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() {
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@zhutoulala
Copy link
Contributor Author

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

@jolestar
Copy link
Member

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.

@nanne007 nanne007 merged commit 78046e9 into starcoinorg:master Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants