Patchwork D6232: rust-discovery: cpython bindings for the core logic

login
register
mail settings
Submitter phabricator
Date April 12, 2019, 6:34 p.m.
Message ID <differential-rev-PHID-DREV-xixeewxlqcgdguvrjsjq-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39577/
State Superseded
Headers show

Comments

phabricator - April 12, 2019, 6:34 p.m.
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As previously done with the ancestors submodule, testing for
  the bindings is provided from Python on a trivial case.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  rust/hg-cpython/src/discovery.rs
  rust/hg-cpython/src/lib.rs
  tests/test-rust-discovery.py

CHANGE DETAILS




To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - April 15, 2019, 4:06 p.m.
gracinet added inline comments.

INLINE COMMENTS

> test-rust-discovery.py:39
> +    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00'
> +    )
> +

This is also exactly the same binary data I'm using in `test-rust-ancestors.py`. I'd prefer to mutualize them, but I'm not sure what the preferred pattern would be:

- binary file sitting next to the test module:

  with open(os.path.join(os.path.dirname(__file__), 'index_data.bin', 'rb')) as indexf):
           ...



- helper module next to the test module:

  from .index_data import non_inlined



- or even

  ``` from .index_data import parseindex ```

what would be the more inline with common practices within the Mercurial project ?

REPOSITORY
  rHG Mercurial

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

To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
Yuya Nishihara - April 18, 2019, 11:16 p.m.
> +    def addinfo(&self, sample: PyObject) -> PyResult<PyObject> {
> +        let mut missing: Vec<Revision> = Vec::new();
> +        let mut common: Vec<Revision> = Vec::new();
> +        for info in sample.iter(py)? { // info is a pair (Revision, bool)
> +            let mut revknown = info?.iter(py)?;
> +            let rev: Revision = revknown.next().unwrap()?.extract(py)?;
> +            let known: bool = revknown.next().unwrap()?.extract(py)?;

Just to confirm. Can we be sure that this `unwrap()` never fails? I mean if
we passed in an empty tuple for example, what would happen?
phabricator - April 18, 2019, 11:22 p.m.
yuja added a comment.


  > +    def addinfo(&self, sample: PyObject) -> PyResult<PyObject> {
  >  +        let mut missing: Vec<Revision> = Vec::new();
  >  +        let mut common: Vec<Revision> = Vec::new();
  >  +        for info in sample.iter(py)? { // info is a pair (Revision, bool)
  >  +            let mut revknown = info?.iter(py)?;
  >  +            let rev: Revision = revknown.next().unwrap()?.extract(py)?;
  >  +            let known: bool = revknown.next().unwrap()?.extract(py)?;
  
  Just to confirm. Can we be sure that this `unwrap()` never fails? I mean if
  we passed in an empty tuple for example, what would happen?

REPOSITORY
  rHG Mercurial

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

To: gracinet, #hg-reviewers, kevincox
Cc: yuja, durin42, kevincox, mercurial-devel

Patch

diff --git a/tests/test-rust-discovery.py b/tests/test-rust-discovery.py
new file mode 100644
--- /dev/null
+++ b/tests/test-rust-discovery.py
@@ -0,0 +1,103 @@ 
+from __future__ import absolute_import
+import unittest
+
+try:
+    from mercurial import rustext
+    rustext.__name__  # trigger immediate actual import
+except ImportError:
+    rustext = None
+else:
+    # this would fail already without appropriate ancestor.__package__
+    from mercurial.rustext.discovery import (
+        PartialDiscovery,
+    )
+
+try:
+    from mercurial.cext import parsers as cparsers
+except ImportError:
+    cparsers = None
+
+# picked from test-parse-index2, copied rather than imported
+# so that it stays stable even if test-parse-index2 changes or disappears.
+data_non_inlined = (
+    b'\x00\x00\x00\x01\x00\x00\x00\x00\x00\x01D\x19'
+    b'\x00\x07e\x12\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff'
+    b'\xff\xff\xff\xff\xd1\xf4\xbb\xb0\xbe\xfc\x13\xbd\x8c\xd3\x9d'
+    b'\x0f\xcd\xd9;\x8c\x07\x8cJ/\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+    b'\x00\x00\x00\x00\x00\x00\x01D\x19\x00\x00\x00\x00\x00\xdf\x00'
+    b'\x00\x01q\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x00\xff'
+    b'\xff\xff\xff\xc1\x12\xb9\x04\x96\xa4Z1t\x91\xdfsJ\x90\xf0\x9bh'
+    b'\x07l&\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+    b'\x00\x01D\xf8\x00\x00\x00\x00\x01\x1b\x00\x00\x01\xb8\x00\x00'
+    b'\x00\x01\x00\x00\x00\x02\x00\x00\x00\x01\xff\xff\xff\xff\x02\n'
+    b'\x0e\xc6&\xa1\x92\xae6\x0b\x02i\xfe-\xe5\xbao\x05\xd1\xe7\x00'
+    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01F'
+    b'\x13\x00\x00\x00\x00\x01\xec\x00\x00\x03\x06\x00\x00\x00\x01'
+    b'\x00\x00\x00\x03\x00\x00\x00\x02\xff\xff\xff\xff\x12\xcb\xeby1'
+    b'\xb6\r\x98B\xcb\x07\xbd`\x8f\x92\xd9\xc4\x84\xbdK\x00\x00\x00'
+    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+    )
+
+
+@unittest.skipIf(rustext is None or cparsers is None,
+                 "rustext or the C Extension parsers module "
+                 "discovery relies on is not available")
+class rustdiscoverytest(unittest.TestCase):
+    """Test the correctness of binding to Rust code.
+
+    This test is merely for the binding to Rust itself: extraction of
+    Python variable, giving back the results etc.
+
+    It is not meant to test the algorithmic correctness of the provided
+    methods. Hence the very simple embedded index data is good enough.
+
+    Algorithmic correctness is asserted by the Rust unit tests.
+    """
+
+    def parseindex(self):
+        return cparsers.parse_index2(data_non_inlined, False)[0]
+
+    def testindex(self):
+        idx = self.parseindex()
+        # checking our assumptions about the index binary data:
+        self.assertEqual({i: (r[5], r[6]) for i, r in enumerate(idx)},
+                         {0: (-1, -1),
+                          1: (0, -1),
+                          2: (1, -1),
+                          3: (2, -1)})
+
+    def testaddcommonsmissings(self):
+        idx = self.parseindex()
+        disco = PartialDiscovery(idx, [3])
+        self.assertFalse(disco.hasinfo())
+        self.assertFalse(disco.iscomplete())
+
+        disco.addcommons([1])
+        self.assertTrue(disco.hasinfo())
+        self.assertFalse(disco.iscomplete())
+
+        disco.addmissings([2])
+        self.assertTrue(disco.hasinfo())
+        self.assertTrue(disco.iscomplete())
+
+        self.assertEqual(disco.commonheads(), {1})
+
+    def testaddinfocommonfirst(self):
+        idx = self.parseindex()
+        disco = PartialDiscovery(idx, [3])
+        disco.addinfo([(1, True), (2, False)])
+        self.assertTrue(disco.hasinfo())
+        self.assertTrue(disco.iscomplete())
+        self.assertEqual(disco.commonheads(), {1})
+
+    def testaddinfomissingfirst(self):
+        idx = self.parseindex()
+        disco = PartialDiscovery(idx, [3])
+        disco.addinfo([(2, False), (1, True)])
+        self.assertTrue(disco.hasinfo())
+        self.assertTrue(disco.iscomplete())
+        self.assertEqual(disco.commonheads(), {1})
+
+if __name__ == '__main__':
+    import silenttestrunner
+    silenttestrunner.main(__name__)
diff --git a/rust/hg-cpython/src/lib.rs b/rust/hg-cpython/src/lib.rs
--- a/rust/hg-cpython/src/lib.rs
+++ b/rust/hg-cpython/src/lib.rs
@@ -28,6 +28,7 @@ 
 mod cindex;
 mod conversion;
 pub mod dagops;
+pub mod discovery;
 pub mod exceptions;
 
 py_module_initializer!(rustext, initrustext, PyInit_rustext, |py, m| {
@@ -40,6 +41,7 @@ 
     let dotted_name: String = m.get(py, "__name__")?.extract(py)?;
     m.add(py, "ancestor", ancestors::init_module(py, &dotted_name)?)?;
     m.add(py, "dagop", dagops::init_module(py, &dotted_name)?)?;
+    m.add(py, "discovery", discovery::init_module(py, &dotted_name)?)?;
     m.add(py, "GraphError", py.get_type::<exceptions::GraphError>())?;
     Ok(())
 });
diff --git a/rust/hg-cpython/src/discovery.rs b/rust/hg-cpython/src/discovery.rs
new file mode 100644
--- /dev/null
+++ b/rust/hg-cpython/src/discovery.rs
@@ -0,0 +1,113 @@ 
+// discovery.rs
+//
+// Copyright 2018 Georges Racinet <gracinet@anybox.fr>
+//
+// This software may be used and distributed according to the terms of the
+// GNU General Public License version 2 or any later version.
+
+//! Bindings for the `hg::discovery` module provided by the
+//! `hg-core` crate. From Python, this will be seen as `rustext.discovery`
+//!
+//! # Classes visible from Python:
+//! - [`PartialDiscover`] is the Rust implementation of
+//!   `mercurial.setdiscovery.partialdiscovery`.
+
+use crate::conversion::{py_set, rev_pyiter_collect};
+use cindex::Index;
+use cpython::{ObjectProtocol, PyDict, PyModule, PyObject, PyResult, Python};
+use exceptions::GraphError;
+use hg::discovery::PartialDiscovery as CorePartialDiscovery;
+use hg::Revision;
+
+use std::cell::RefCell;
+
+py_class!(pub class PartialDiscovery |py| {
+    data inner: RefCell<Box<CorePartialDiscovery<Index>>>;
+
+    def __new__(
+        _cls,
+        index: PyObject,
+        targetheads: PyObject
+    ) -> PyResult<PartialDiscovery> {
+        Self::create_instance(
+            py,
+            RefCell::new(Box::new(CorePartialDiscovery::new(
+                Index::new(py, index)?,
+                rev_pyiter_collect(py, &targetheads)?,
+            )))
+        )
+    }
+
+    def addcommons(&self, commons: PyObject) -> PyResult<PyObject> {
+        let mut inner = self.inner(py).borrow_mut();
+        let commons_vec: Vec<Revision> = rev_pyiter_collect(py, &commons)?;
+        inner.add_common_revisions(commons_vec)
+            .map_err(|e| GraphError::pynew(py, e))?;
+        Ok(py.None())
+    }
+
+    def addmissings(&self, missings: PyObject) -> PyResult<PyObject> {
+        let mut inner = self.inner(py).borrow_mut();
+        let missings_vec: Vec<Revision> = rev_pyiter_collect(py, &missings)?;
+        inner.add_missing_revisions(missings_vec)
+            .map_err(|e| GraphError::pynew(py, e))?;
+        Ok(py.None())
+    }
+
+    def addinfo(&self, sample: PyObject) -> PyResult<PyObject> {
+        let mut missing: Vec<Revision> = Vec::new();
+        let mut common: Vec<Revision> = Vec::new();
+        for info in sample.iter(py)? { // info is a pair (Revision, bool)
+            let mut revknown = info?.iter(py)?;
+            let rev: Revision = revknown.next().unwrap()?.extract(py)?;
+            let known: bool = revknown.next().unwrap()?.extract(py)?;
+            if known {
+                common.push(rev);}
+            else {
+                missing.push(rev);}
+        }
+        let mut inner = self.inner(py).borrow_mut();
+        inner.add_common_revisions(common)
+            .map_err(|e| GraphError::pynew(py, e))?;
+        inner.add_missing_revisions(missing)
+            .map_err(|e| GraphError::pynew(py, e))?;
+        Ok(py.None())
+    }
+
+    def hasinfo(&self) -> PyResult<bool> {
+        Ok(self.inner(py).borrow().has_info())
+    }
+
+    def iscomplete(&self) -> PyResult<bool> {
+        Ok(self.inner(py).borrow().is_complete())
+    }
+
+    def commonheads(&self) -> PyResult<PyObject> {
+        py_set(
+            py,
+            &self.inner(py).borrow().common_heads()
+                .map_err(|e| GraphError::pynew(py, e))?
+        )
+    }
+});
+
+/// Create the module, with __package__ given from parent
+pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
+    let dotted_name = &format!("{}.discovery", package);
+    let m = PyModule::new(py, dotted_name)?;
+    m.add(py, "__package__", package)?;
+    m.add(
+        py,
+        "__doc__",
+        "Discovery of common node sets - Rust implementation",
+    )?;
+    m.add_class::<PartialDiscovery>(py)?;
+
+    let sys = PyModule::import(py, "sys")?;
+    let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;
+    sys_modules.set_item(py, dotted_name, &m)?;
+    // Example C code (see pyexpat.c and import.c) will "give away the
+    // reference", but we won't because it will be consumed once the
+    // Rust PyObject is dropped.
+    Ok(m)
+}