Page MenuHomePhabricator

rust-2018: switch hg-core and hg-cpython to rust 2018 edition
ClosedPublic

Authored by Alphare on Jul 2 2019, 12:31 PM.

Details

Summary

Many interesting changes have happened in Rust since the Oxidation Plan was
introduced, like the 2018 edition and procedural macros:

  • Opting in to the 2018 edition is a clear benefit in terms of future proofing, new (nice to have) syntactical sugar notwithstanding. It also has a new non-lexical, non-AST based borrow checker that has fewer bugs(!) and allows us to write correct code that in some cases would have been rejected by the old one.
  • Procedural macros allow us to use the PyO3 crate which maintainers have expressed the clear goal of compiling on stable, which would help in code maintainability compared to rust-cpython.

In this patch are the following changes:

  • Removing most extern crate uses
  • Updating use clauses (crate keyword, nested use)
  • Removing mod.rs in favor of an aptly named module file

Like discussed in the mailing list (
https://www.mercurial-scm.org/pipermail/mercurial-devel/2019-July/132316.html
), until Rust integration in Mercurial is considered to be out of the
experimental phase, the maximum version of Rust allowed is whatever the latest
version Debian packages.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jul 2 2019, 12:31 PM
pulkit added a subscriber: pulkit.Jul 2 2019, 5:56 PM

Can you add link of mailing list thread in your commit message? Link from markmail or https://www.mercurial-scm.org/pipermail/mercurial-devel/.

kevincox accepted this revision.Jul 10 2019, 10:52 AM

Is the SliceExt change related to the 2018 change? If not could you split the two?

rust/hg-core/src/utils.rs
4

This could really use a doc comment.

8

from.len() != to.len() sounds like a bug and I would probably use an assert!(). Unless this is expected to be common and "okay".

13

This allows overlapping replacements. I don't know if this is intended.

39

Why not define trim as self.trim_start().trim_end()?

Is the SliceExt change related to the 2018 change? If not could you split the two?

It is indeed related to 2018: mod.rs files are not needed anymore and this makes it both simpler in terms of file structure, but also for development, as having 5 mod.rs files open is not fun.

In D6597#96317, @pulkit wrote:

Can you add link of mailing list thread in your commit message? Link from markmail or https://www.mercurial-scm.org/pipermail/mercurial-devel/.

Sure, will amend.

Alphare edited the summary of this revision. (Show Details)Jul 10 2019, 11:05 AM
Alphare updated this revision to Diff 15853.
Alphare added inline comments.Jul 10 2019, 11:06 AM
rust/hg-core/src/utils.rs
4

I'll address those points in a separate (parent) changeset.

Alphare updated this revision to Diff 15896.Jul 12 2019, 5:21 AM

LGTM, but needs rebased (sorry for slow response)

LGTM, but needs rebased (sorry for slow response)

@durin42 I've rebased it on hg-committed, but nothing's changed. Am I missing something?

LGTM, but needs rebased (sorry for slow response)

@durin42 I've rebased it on hg-committed, but nothing's changed. Am I missing something?

I'm getting a failure in dirs_multiset.rs. If it helps:

SHA1(rust/hg-core/src/dirstate/dirs_multiset.rs)= 13be7cbb7e133a90ae68ce838125bc43f452dec8

so maybe yours is different?

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jul 17 2019, 7:32 PM

diff --git a/rust/hg-core/src/utils/mod.rs b/rust/hg-core/src/utils/mod.rs
deleted file mode 100644

Can you rename these instead of adding new files? Last time I fixed that
manually, and I don't wanna do that again.

In D6597#97348, @yuja wrote:

diff --git a/rust/hg-core/src/utils/mod.rs b/rust/hg-core/src/utils/mod.rs
deleted file mode 100644

Can you rename these instead of adding new files? Last time I fixed that
manually, and I don't wanna do that again.

I fixed the renames. The amended changeset is available at, bitbucket.org/octobus/mercurial-devel, rev d4bbf54624e14ac2913d452a8a7cb32823227c16. If you want me to send you the patch some other way, please tell me.

yuja added a comment.Jul 18 2019, 8:51 AM
I fixed the renames. The amended changeset is available at, bitbucket.org/octobus/mercurial-devel, rev d4bbf54624e14ac2913d452a8a7cb32823227c16. If you want me to send you the patch some other way, please tell me.

Pushed as 326fdce22fb2, thanks.