Patchwork D7952: rust: add a README

login
register
mail settings
Submitter phabricator
Date Jan. 20, 2020, 11:36 p.m.
Message ID <differential-rev-PHID-DREV-b3wwvnln7ncqyngzdv2n-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44531/
State Superseded
Headers show

Comments

phabricator - Jan. 20, 2020, 11:36 p.m.
valentin.gatienbaron created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  In particular to explain how to build any of the rust. It's neither
  obvious, nor easy to find out, nor easy to determine if you did it
  right without some documentation.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

AFFECTED FILES
  rust/README.rst
  setup.py

CHANGE DETAILS




To: valentin.gatienbaron, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 9:32 a.m.
Alphare added inline comments.
Alphare accepted this revision.

INLINE COMMENTS

> README.rst:27
> +  $ HGWITHRUSTEXT=cpython make tests # to run all tests
> +  $ (cd tests; HGWITHRUSTEXT=cpython ./run-tests.py) # only the .t
> +  $ ./hg debuginstall | grep rust # to validate rust is in use

I think you're missing the `-l` option for `run-tests.py`

> README.rst:34
>  
> -If you prefer a non-debug / release configuration::
> +Developing hg-core
> +==================

I would add a mention of using `cargo c(heck)` for faster iteration and using `cargo test --all` to run the Rust tests, but this patch is already good enough on its own.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 12:14 p.m.
pulkit added inline comments.
pulkit added subscribers: gracinet, pulkit.

INLINE COMMENTS

> README.rst:25
>  
> -To build the Rust components::
> +  $ HGWITHRUSTEXT=cpython make local # to use ./hg
> +  $ HGWITHRUSTEXT=cpython make tests # to run all tests

IIRC, it's now possible to use `HGMODULEPOLICY=rust`. @gracinet that's possible now right?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 12:28 p.m.
Alphare added inline comments.

INLINE COMMENTS

> pulkit wrote in README.rst:25
> IIRC, it's now possible to use `HGMODULEPOLICY=rust`. @gracinet that's possible now right?

`HGMODULEPOLICY=rust+c` to select it at runtime yes. But for not for `make local` AFAIK.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 12:36 p.m.
gracinet added a comment.


  My general feeling is that I'd like to deprecate `HGWITHRUSTEXT`.
  
  We now have `python setup.py --rust [build|install|etc]` and the coincidental `make local PURE=--rust` (works because `$PURE`, while meant to be for `--pure` is inserted blindly at the right place in the `setup.py` command.
  
  Maybe this is a good time to decide of a better make variable ? EXTENSION_TYPE ? COMPILE_OPTIONS ? BUILD_OPTIONS ?
  
  It's obviously a good thing to have official instructions, thank you Valentin for raising the subject.

INLINE COMMENTS

> pulkit wrote in README.rst:25
> IIRC, it's now possible to use `HGMODULEPOLICY=rust`. @gracinet that's possible now right?

`HGMODULEPOLICY` is for runtime, it can be `rust+c` or `rust+c-allow`. 
In the first form, not having the Rust extension is a failure, in the second, there's a fallback.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 12:44 p.m.
gracinet added a comment.


  "deprecate" was a bit too strong. Actually I'd like to downgrade `HGWITHRUSTEXT` to be something that'll be useful to play with alternative ways of building the Rust extension(s). Example: if someone wants to experiment with PyO3.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 2:35 p.m.
valentin.gatienbaron added a comment.
valentin.gatienbaron marked an inline comment as done.


  I didn't  mention `setup.py --rust` because the toplevel readme doesn't mention setup.py, so I don't really know if that's really how mercurial is intended to be built or an implementation detail.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 2:38 p.m.
valentin.gatienbaron added inline comments.

INLINE COMMENTS

> gracinet wrote in README.rst:25
> `HGMODULEPOLICY` is for runtime, it can be `rust+c` or `rust+c-allow`. 
> In the first form, not having the Rust extension is a failure, in the second, there's a fallback.

When I tested, setting HGMODULEPOLICY didn't work, perhaps because of what Raphael said.

> Alphare wrote in README.rst:27
> I think you're missing the `-l` option for `run-tests.py`

Well, one can use `-l` to run rust tests, but it's not mandatory?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel
phabricator - Jan. 21, 2020, 3:10 p.m.
gracinet added inline comments.

INLINE COMMENTS

> valentin.gatienbaron wrote in README.rst:25
> When I tested, setting HGMODULEPOLICY didn't work, perhaps because of what Raphael said.

Both Raphaƫl and I were saying the same thin, perhaps a bit partially.

Here's the whole picture:

- options to `make` or `setup.py` govern what is built (`HGWITHRUSTEXT` also).
- among the built extensions, what is actually used at runtime depends on the module policy, which is read from `mercurial/__modulepolicy__`.py and can be overridden by HGMODULEPOLICY ,
- at build time, a default policy value is set in `mercurial/__modulepolicy__.py`. For the record, this is `rust+c-allow` for local builds with Rust and `rust+c` for other Rust builds.

This policy system is actuallly not specific to Rust, it's also in use  with the C extensions, the major difference being that C extensions are built by default.

For example, if one does not build the C extensions and use `HGMODULEPOLICY=c`, that's a hard error.  With `HGMODULEPOLICY=allow`, one gets a fallback to the pure implementation.

Same with Rust: `rust+c` gives a hard error is Rust isn't built, while `rust+c-allow` fallbacks to C extensions only

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7952/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7952

To: valentin.gatienbaron, #hg-reviewers, Alphare
Cc: pulkit, gracinet, Alphare, mjpieters, mercurial-devel

Patch

diff --git a/setup.py b/setup.py
--- a/setup.py
+++ b/setup.py
@@ -1382,9 +1382,9 @@ 
 class RustEnhancedExtension(RustExtension):
     """A C Extension, conditionally enhanced with Rust code.
 
-    If the HGRUSTEXT environment variable is set to something else
-    than 'cpython', the Rust sources get compiled and linked within the
-    C target shared library object.
+    If the HGWITHRUSTEXT environment variable is set to something else
+    than 'cpython', the Rust sources get compiled and linked within
+    the C target shared library object.
     """
 
     def __init__(self, mpath, sources, rustlibname, subcrate, **kw):
diff --git a/rust/README.rst b/rust/README.rst
--- a/rust/README.rst
+++ b/rust/README.rst
@@ -3,17 +3,41 @@ 
 ===================
 
 This directory contains various Rust code for the Mercurial project.
+Rust is not required to use (or build) Mercurial, but using it
+improves performance in some areas.
 
-The top-level ``Cargo.toml`` file defines a workspace containing
-all primary Mercurial crates.
+There are currently three independent rust projects:
+- chg. An implementation of chg, in rust instead of C.
+- hgcli. A experiment for starting hg in rust rather than in python,
+  by linking with the python runtime. Probably meant to be replaced by
+  PyOxidizer at some point.
+- hg-core (and hg-cpython/hg-directffi): implementation of some
+  functionality of mercurial in rust, e.g. ancestry computations in
+  revision graphs or pull discovery. The top-level ``Cargo.toml`` file
+  defines a workspace containing these crates.
+
+Using hg-core
+=============
 
-Building
-========
+Local use (you need to clean previous build artifacts if you have
+built without rust previously)::
 
-To build the Rust components::
+  $ HGWITHRUSTEXT=cpython make local # to use ./hg
+  $ HGWITHRUSTEXT=cpython make tests # to run all tests
+  $ (cd tests; HGWITHRUSTEXT=cpython ./run-tests.py) # only the .t
+  $ ./hg debuginstall | grep rust # to validate rust is in use
+  checking module policy (rust+c-allow)
 
-   $ cargo build
+Setting ``HGWITHRUSTEXT`` to other values like ``true`` is deprecated
+and enables only a fraction of the rust code.
 
-If you prefer a non-debug / release configuration::
+Developing hg-core
+==================
+
+Simply run::
 
    $ cargo build --release
+
+It is possible to build without ``--release``, but it is not
+recommended if performance is of any interest: there can be an order
+of magnitude of degradation when removing ``--release``.