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

Convert to Rust 2018 edition #2084

Merged
merged 6 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@hashmap
Copy link
Member

hashmap commented Dec 5, 2018

  • Remove extern crate, leave it just for macros import. It could be done later, but instead of 2 line we need to add use crate:macro in each file which uses the macro
  • Proper absolute path support
  • Get rid of mod.rs (have some issue with couple tests)

^ https://rust-lang-nursery.github.io/edition-guide/rust-2018/module-system/path-clarity.html

hashmap added some commits Dec 4, 2018

@hashmap hashmap changed the title [WIP] Convert to Rust 2018 edition Convert to Rust 2018 edition Dec 7, 2018

@hashmap

This comment has been minimized.

Copy link
Member

hashmap commented Dec 7, 2018

Ah, can't keep with updates:) will merge one more time

@ignopeverell

This comment has been minimized.

Copy link
Member

ignopeverell commented Dec 7, 2018

Should we merge this soon then (once conflicts are fixed)? I've just done a full binary release (including crates.io) so perhaps we can tell people to use that and not build by default. And then developers can move to the beta channel until release.

I just don't want this to drag on too close to Jan.

@hashmap

This comment has been minimized.

Copy link
Member

hashmap commented Dec 7, 2018

Yeah, this pr oxidizes very quickly:). If no objection I will merge it after resolving conflicts.

No problems with stable rust, it was released yesterday as part of 1.31, it’s why I removed WIP and sacrified a good chunk of the night to fix conflicts. It’s green on Travis, I assume we use the latest stable Rust

@sesam

This comment has been minimized.

Copy link
Contributor

sesam commented Dec 7, 2018

Always be upgrading :). I guess cargo fix can help with part of this upgrade. Is there any pre-2018 version or utility that helps with discovering code deprecations?

@hashmap

This comment has been minimized.

Copy link
Member

hashmap commented Dec 7, 2018

@sesam 80% of this pr is done by cargo fix (twice per crate, plain and idioms). It fails on tests crates though, so there is some hand crafted code.
I find clippy useful and cargo fix may be used with any rust code (without —edition) if I’m not mistaken

@hashmap hashmap merged commit aedac48 into mimblewimble:master Dec 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hashmap hashmap deleted the cyclefortytwo:edition-2018 branch Dec 7, 2018

@clefru

This comment has been minimized.

Copy link

clefru commented Dec 8, 2018

@ignopeverell @hashmap

With all due respect, is this a crypto-currency project or a Rust language demo? I have to ask again to STOP requiring bleeding edge Rust. Rust 1.31.0 was released 2 days ago. I have to drop my general politeness on GitHub issues and say: This is an insane requirement.

No, I am not going to use "rustup". It's a waste of time for everyone to not just keep up to date with Grin, but also update with Rust. That's what we have distros for. Also let me remind you that rustup recommends you to do: "curl https://sh.rustup.rs -sSf | sh". I am surprised that from a security point of view that's still a thing in 2018.

Please revert.

@sesam

This comment has been minimized.

Copy link
Contributor

sesam commented Dec 9, 2018

@clefru Thank you for your concern :). Generally I think you're both right and wrong; older versions have been more tried, but newer versions might contain fixes for yet-to-be-publicly-announced critical bugs. So I think we're stuck with Always be upgrading. Maybe it would suit you to use grin via Docker? That way you can be more certain that grin is built with the same libraries that others use, and with minimal risk of conflicting or tainting your primary OS. Come see us in https://gitter.im/grin_community/dev or https://gitter.im/grin_community/Lobby and we can discuss this more in depth :).

@clefru

This comment has been minimized.

Copy link

clefru commented Dec 9, 2018

@sesam Thanks for your nice words. I have filed a separate issue here: #2103 as it was requested on gitter.

@hashmap

This comment has been minimized.

Copy link
Member

hashmap commented Dec 10, 2018

@clefru thanks, I totally understand your point. Our goal in this case is not to blindly update Rust to the latest, but increase dev productivity and make Grin more inclusive for people without Rust background. That's the reason why we need Rust 2018 [1]:

The theme of Rust 2018 is productivity. Rust 2018 improves upon Rust 2015 through new features, simpler syntax in some cases, a smarter borrow-checker, and a host of other things. These are all in service of the productivity goal. Rust 2015 was a foundation; Rust 2018 smooths off rough edges, makes writing code simpler and easier, and removes some inconsistencies.

Non-lexical lifetimes [2] is a game changer for Rust. I'd argue it would sufficiently lower the barrier for new contributors.

For sure after the mainnet release we will be much more conservative, so it was our the last chance to switch to Rust 2018. It's why we switched to rust 1.31 as fast as possible - to have more time to test it. We may be insane but it was a pragmatic decision in this particular case, discussed in September.

[1] https://rust-lang-nursery.github.io/edition-guide/rust-2018/index.html
[2] https://rust-lang-nursery.github.io/edition-guide/rust-2018/ownership-and-lifetimes/non-lexical-lifetimes.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment