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

Modernize the codebase #204

Merged
merged 10 commits into from Feb 4, 2020
Merged

Modernize the codebase #204

merged 10 commits into from Feb 4, 2020

Conversation

markbt
Copy link
Collaborator

@markbt markbt commented Jan 28, 2020

Putting this up as an RFC.

This updates to more modern code, including edition 2018. It now requires rustc 1.36 so that we can use things like AssumeUninit (without this, all the std::mem::uninitialized() calls show up as warnings).

Rust 1.36 was released over 6 months ago. However, it looks like Debian stable is pinned at 1.34, so if we want to keep targetting that as a minumum, we won't be able to take all of this. In practice I would expect users of Rust that are keeping their dependent crates up-to-date would also be using rustup to keep the compiler up-to-date, too.

Any comments?

@Alphare
Copy link
Contributor

Alphare commented Jan 29, 2020

Mercurial uses rust-cpython extensively and targets Debian stable's version, I'm afraid this version bump breaks our compatibility guarantees. I would love to target 1.40+ for obvious reasons, but I'm afraid we are going to have to wait until Bullseye is released.

@Alphare
Copy link
Contributor

Alphare commented Jan 29, 2020

Pinging @gracinet in case you have anything to add.

@markbt
Copy link
Collaborator Author

markbt commented Jan 29, 2020

I thought that might be the case. I can drop the std::mem::MaybeUninit parts of the PR, as they are the ones that are not available in 1.34. That should let us still target 1.32+.

This will allow us to use edition 2018.
Since we are targetting 1.32, and std::mem::MaybeUninit wasn't stabilized until
1.36, suppress the deprecation warning.
Use the `paste` crate to generate the module init symbol names (`initfoo` and
`PyInit_foo`), rather than requiring the user to pass them in.  This makes
creating modules less error-prone.
@Alphare
Copy link
Contributor

Alphare commented Jan 29, 2020

@markbt You could play with config attributes to silence warnings depending on the version?
Thanks for being considerate of this matter.

@markbt
Copy link
Collaborator Author

markbt commented Jan 29, 2020

Using cfg to target specific complier versions hasn't been stabilized yet (see rust-lang/rfcs#2523). Blanket #[allow(deprecated)] on the unsafe blocks that contain std::mem::uninitialized seems to work ok. I've left TODOs so we can clean up once bullseye is released.

I'll wait to see what @gracinet says, but hopefully this PR is now suitable for merging.

@gracinet
Copy link
Collaborator

@markbt yes we'll want to be able to build Mercurial Rust extensions on Debian buster, hence 1.34 and maybe even submit that for buster-backports, who knows? I'll take a closer look this evening if I can or maybe tomorrow.

@markbt
Copy link
Collaborator Author

markbt commented Jan 29, 2020

I've switched to 1.32 now (which is the minimum version for edition 2018 and a couple of the associated syntax improvements). That should mean it works on stock Debian buster, although please do check.

I've suppressed the deprecation warnings for now. There's no real rush to replace std::mem::uninitialized() with std::mem::MaybeUninit.

@gracinet
Copy link
Collaborator

I've tested successfully with the Rust toolchain of Debian 10:

~ $ which cargo; which rustc; cargo --version; rustc --version
/usr/bin/cargo
/usr/bin/rustc
cargo 1.34.0
rustc 1.34.2
~ divers/rust-cpython $ git log --oneline -n1
a9e7e49 (HEAD, markbt/modernize) use paste to generate module init symbol names

and unsurprisingly have also been able to build Mercurial's hg-cpython

@gracinet
Copy link
Collaborator

Thanks for this, @markbt, switching to the 2018 edition will be a boon. And this unlocks the procedural macros

@gracinet
Copy link
Collaborator

I've read the whole diff quickly.

  • switch to new use and reference to constructs from std or other crates
  • explicit naming of arguments in traits
  • removal of local_inner_macros, in favor of $crate:: and the associated local reexports of standard macros such as concat! (good riddance)
  • thanks to the paste crate capability to concat identifiers, py_module_initializer! usage is vastly simplified, and the examples now reflect that (can't wait to apply this in the Mercurial code base)

@gracinet
Copy link
Collaborator

I think there's at least one fix in master (PR #206) that we'd maybe like to release first as a 0.4.1, and then this PR could bump the version number to 0.5.0, but maybe I'm overly conservative.

@markbt
Copy link
Collaborator Author

markbt commented Jan 31, 2020

A 0.4.1 for #206 seems reasonable. I can spin that later this week unless you get there first, @gracinet.

@gracinet
Copy link
Collaborator

gracinet commented Feb 3, 2020

A 0.4.1 for #206 seems reasonable. I can spin that later this week unless you get there first, @gracinet.

Didn't realize I had the permissions to do that, but it makes sense. Trying right now, and should merge this PR afterwards.

@gracinet
Copy link
Collaborator

gracinet commented Feb 3, 2020

@markbt no, I can't publish directly on crates.io. Did all the other preparations, including the tag

@markbt markbt mentioned this pull request Feb 4, 2020
@gracinet gracinet merged commit 5b180f6 into dgrunwald:master Feb 4, 2020
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.

None yet

3 participants