Patchwork [1,of,6,V2] setup-rust: remove the legacy 'direct-ffi' variant

login
register
mail settings
Submitter Pierre-Yves David
Date March 8, 2020, 9:06 p.m.
Message ID <ac97907dc3f16b039213.1583701612@nodosa.octobus.net>
Download mbox | patch
Permalink /patch/45620/
State Accepted
Headers show

Comments

Pierre-Yves David - March 8, 2020, 9:06 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1583509786 -3600
#      Fri Mar 06 16:49:46 2020 +0100
# Node ID ac97907dc3f16b039213f54adb37d0a5b04eaf9b
# Parent  c8fd21413458cec4537f3906cdb87f3c9ebe3367
# EXP-Topic rust-test-option
# Available At https://dev.heptapod.net/octobus/mercurial-devel/
#              hg pull https://dev.heptapod.net/octobus/mercurial-devel/ -r ac97907dc3f1
setup-rust: remove the legacy 'direct-ffi' variant

This variant have been abandoned for a while. Keeping it around just get people
confused.
Yuya Nishihara - March 9, 2020, 1:37 p.m.
On Sun, 08 Mar 2020 22:06:52 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1583509786 -3600
> #      Fri Mar 06 16:49:46 2020 +0100
> # Node ID ac97907dc3f16b039213f54adb37d0a5b04eaf9b
> # Parent  c8fd21413458cec4537f3906cdb87f3c9ebe3367
> # EXP-Topic rust-test-option
> # Available At https://dev.heptapod.net/octobus/mercurial-devel/
> #              hg pull https://dev.heptapod.net/octobus/mercurial-devel/ -r ac97907dc3f1
> setup-rust: remove the legacy 'direct-ffi' variant

Queued 1-2, thanks.

> diff --git a/setup.py b/setup.py
> --- a/setup.py
> +++ b/setup.py
> @@ -3,6 +3,7 @@
>  #
>  # 'python setup.py install', or
>  # 'python setup.py --help' for more options
> +from __future__ import print_function

Maybe there's a reason to not do future import in setup.py. We have printf().

> -class RustEnhancedExtension(RustExtension):
> -    """A C Extension, conditionally enhanced with Rust code.
> -
> -    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):
> -        RustExtension.__init__(
> -            self, mpath, sources, rustlibname, subcrate, **kw
> -        )
> -        if hgrustext != 'direct-ffi':
> -            return
> -        self.extra_compile_args.append('-DWITH_RUST')

Nice. We can get rid of WITH_RUST block and its callers at all. Can you
send a follow up patch?

Patch

diff --git a/rust/Cargo.lock b/rust/Cargo.lock
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -217,14 +217,6 @@  dependencies = [
 ]
 
 [[package]]
-name = "hgdirectffi"
-version = "0.1.0"
-dependencies = [
- "hg-core 0.1.0",
- "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)",
-]
-
-[[package]]
 name = "lazy_static"
 version = "1.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -1,3 +1,3 @@ 
 [workspace]
-members = ["hg-core", "hg-direct-ffi", "hg-cpython"]
+members = ["hg-core", "hg-cpython"]
 exclude = ["chg", "hgcli"]
diff --git a/rust/hg-direct-ffi/Cargo.toml b/rust/hg-direct-ffi/Cargo.toml
deleted file mode 100644
--- a/rust/hg-direct-ffi/Cargo.toml
+++ /dev/null
@@ -1,12 +0,0 @@ 
-[package]
-name = "hgdirectffi"
-version = "0.1.0"
-authors = ["Georges Racinet <gracinet@anybox.fr>"]
-description = "Low level Python bindings for hg-core, going through existing C extensions"
-
-[dependencies]
-libc = "*"
-hg-core = { path = "../hg-core" }
-
-[lib]
-crate-type = ["staticlib"]
diff --git a/rust/hg-direct-ffi/rustfmt.toml b/rust/hg-direct-ffi/rustfmt.toml
deleted file mode 100644
--- a/rust/hg-direct-ffi/rustfmt.toml
+++ /dev/null
@@ -1,3 +0,0 @@ 
-max_width = 79
-wrap_comments = true
-error_on_line_overflow = true
diff --git a/rust/hg-direct-ffi/src/ancestors.rs b/rust/hg-direct-ffi/src/ancestors.rs
deleted file mode 100644
--- a/rust/hg-direct-ffi/src/ancestors.rs
+++ /dev/null
@@ -1,282 +0,0 @@ 
-// 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 CPython extension code
-//!
-//! This exposes methods to build and use a `rustlazyancestors` iterator
-//! from C code, using an index and its parents function that are passed
-//! from the caller at instantiation.
-
-use hg::AncestorsIterator;
-use hg::{Graph, GraphError, Revision, NULL_REVISION};
-use libc::{c_int, c_long, c_void, ssize_t};
-use std::ptr::null_mut;
-use std::slice;
-
-type IndexPtr = *mut c_void;
-
-extern "C" {
-    fn HgRevlogIndex_GetParents(
-        op: IndexPtr,
-        rev: c_int,
-        parents: *mut [c_int; 2],
-    ) -> c_int;
-}
-
-/// A Graph backed up by objects and functions from revlog.c
-///
-/// This implementation of the Graph trait, relies on (pointers to)
-/// - the C index object (`index` member)
-/// - the `index_get_parents()` function (`parents` member)
-pub struct Index {
-    index: IndexPtr,
-}
-
-impl Index {
-    pub fn new(index: IndexPtr) -> Self {
-        Index { index: index }
-    }
-}
-
-impl Graph for Index {
-    /// wrap a call to the C extern parents function
-    fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
-        let mut res: [c_int; 2] = [0; 2];
-        let code = unsafe {
-            HgRevlogIndex_GetParents(
-                self.index,
-                rev,
-                &mut res as *mut [c_int; 2],
-            )
-        };
-        match code {
-            0 => Ok(res),
-            _ => Err(GraphError::ParentOutOfRange(rev)),
-        }
-    }
-}
-
-/// Wrapping of AncestorsIterator<Index> constructor, for C callers.
-///
-/// Besides `initrevs`, `stoprev` and `inclusive`, that are converted
-/// we receive the index and the parents function as pointers
-#[no_mangle]
-pub extern "C" fn rustlazyancestors_init(
-    index: IndexPtr,
-    initrevslen: ssize_t,
-    initrevs: *mut c_long,
-    stoprev: c_long,
-    inclusive: c_int,
-) -> *mut AncestorsIterator<Index> {
-    assert!(initrevslen >= 0);
-    unsafe {
-        raw_init(
-            Index::new(index),
-            initrevslen as usize,
-            initrevs,
-            stoprev,
-            inclusive,
-        )
-    }
-}
-
-/// Testable (for any Graph) version of rustlazyancestors_init
-#[inline]
-unsafe fn raw_init<G: Graph>(
-    graph: G,
-    initrevslen: usize,
-    initrevs: *mut c_long,
-    stoprev: c_long,
-    inclusive: c_int,
-) -> *mut AncestorsIterator<G> {
-    let inclb = match inclusive {
-        0 => false,
-        1 => true,
-        _ => {
-            return null_mut();
-        }
-    };
-
-    let slice = slice::from_raw_parts(initrevs, initrevslen);
-
-    Box::into_raw(Box::new(
-        match AncestorsIterator::new(
-            graph,
-            slice.into_iter().map(|&r| r as Revision),
-            stoprev as Revision,
-            inclb,
-        ) {
-            Ok(it) => it,
-            Err(_) => {
-                return null_mut();
-            }
-        },
-    ))
-}
-
-/// Deallocator to be called from C code
-#[no_mangle]
-pub extern "C" fn rustlazyancestors_drop(
-    raw_iter: *mut AncestorsIterator<Index>,
-) {
-    raw_drop(raw_iter);
-}
-
-/// Testable (for any Graph) version of rustlazayancestors_drop
-#[inline]
-fn raw_drop<G: Graph>(raw_iter: *mut AncestorsIterator<G>) {
-    unsafe {
-        Box::from_raw(raw_iter);
-    }
-}
-
-/// Iteration main method to be called from C code
-///
-/// We convert the end of iteration into NULL_REVISION,
-/// it will be up to the C wrapper to convert that back into a Python end of
-/// iteration
-#[no_mangle]
-pub extern "C" fn rustlazyancestors_next(
-    raw: *mut AncestorsIterator<Index>,
-) -> c_long {
-    raw_next(raw)
-}
-
-/// Testable (for any Graph) version of rustlazayancestors_next
-#[inline]
-fn raw_next<G: Graph>(raw: *mut AncestorsIterator<G>) -> c_long {
-    let as_ref = unsafe { &mut *raw };
-    let rev = match as_ref.next() {
-        Some(Ok(rev)) => rev,
-        Some(Err(_)) | None => NULL_REVISION,
-    };
-    rev as c_long
-}
-
-#[no_mangle]
-pub extern "C" fn rustlazyancestors_contains(
-    raw: *mut AncestorsIterator<Index>,
-    target: c_long,
-) -> c_int {
-    raw_contains(raw, target)
-}
-
-/// Testable (for any Graph) version of rustlazayancestors_next
-#[inline]
-fn raw_contains<G: Graph>(
-    raw: *mut AncestorsIterator<G>,
-    target: c_long,
-) -> c_int {
-    let as_ref = unsafe { &mut *raw };
-    match as_ref.contains(target as Revision) {
-        Ok(r) => r as c_int,
-        Err(_) => -1,
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-    use std::thread;
-
-    #[derive(Clone, Debug)]
-    struct Stub;
-
-    impl Graph for Stub {
-        fn parents(&self, r: Revision) -> Result<[Revision; 2], GraphError> {
-            match r {
-                25 => Err(GraphError::ParentOutOfRange(25)),
-                _ => Ok([1, 2]),
-            }
-        }
-    }
-
-    /// Helper for test_init_next()
-    fn stub_raw_init(
-        initrevslen: usize,
-        initrevs: usize,
-        stoprev: c_long,
-        inclusive: c_int,
-    ) -> usize {
-        unsafe {
-            raw_init(
-                Stub,
-                initrevslen,
-                initrevs as *mut c_long,
-                stoprev,
-                inclusive,
-            ) as usize
-        }
-    }
-
-    fn stub_raw_init_from_vec(
-        mut initrevs: Vec<c_long>,
-        stoprev: c_long,
-        inclusive: c_int,
-    ) -> *mut AncestorsIterator<Stub> {
-        unsafe {
-            raw_init(
-                Stub,
-                initrevs.len(),
-                initrevs.as_mut_ptr(),
-                stoprev,
-                inclusive,
-            )
-        }
-    }
-
-    #[test]
-    // Test what happens when we init an Iterator as with the exposed C ABI
-    // and try to use it afterwards
-    // We spawn new threads, in order to make memory consistency harder
-    // but this forces us to convert the pointers into shareable usizes.
-    fn test_init_next() {
-        let mut initrevs: Vec<c_long> = vec![11, 13];
-        let initrevs_len = initrevs.len();
-        let initrevs_ptr = initrevs.as_mut_ptr() as usize;
-        let handler = thread::spawn(move || {
-            stub_raw_init(initrevs_len, initrevs_ptr, 0, 1)
-        });
-        let raw = handler.join().unwrap() as *mut AncestorsIterator<Stub>;
-
-        assert_eq!(raw_next(raw), 13);
-        assert_eq!(raw_next(raw), 11);
-        assert_eq!(raw_next(raw), 2);
-        assert_eq!(raw_next(raw), 1);
-        assert_eq!(raw_next(raw), NULL_REVISION as c_long);
-        raw_drop(raw);
-    }
-
-    #[test]
-    fn test_init_wrong_bool() {
-        assert_eq!(stub_raw_init_from_vec(vec![11, 13], 0, 2), null_mut());
-    }
-
-    #[test]
-    fn test_empty() {
-        let raw = stub_raw_init_from_vec(vec![], 0, 1);
-        assert_eq!(raw_next(raw), NULL_REVISION as c_long);
-        raw_drop(raw);
-    }
-
-    #[test]
-    fn test_init_err_out_of_range() {
-        assert!(stub_raw_init_from_vec(vec![25], 0, 0).is_null());
-    }
-
-    #[test]
-    fn test_contains() {
-        let raw = stub_raw_init_from_vec(vec![5, 6], 0, 1);
-        assert_eq!(raw_contains(raw, 5), 1);
-        assert_eq!(raw_contains(raw, 2), 1);
-    }
-
-    #[test]
-    fn test_contains_exclusive() {
-        let raw = stub_raw_init_from_vec(vec![5, 6], 0, 0);
-        assert_eq!(raw_contains(raw, 5), 0);
-        assert_eq!(raw_contains(raw, 2), 1);
-    }
-}
diff --git a/rust/hg-direct-ffi/src/lib.rs b/rust/hg-direct-ffi/src/lib.rs
deleted file mode 100644
--- a/rust/hg-direct-ffi/src/lib.rs
+++ /dev/null
@@ -1,19 +0,0 @@ 
-// 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 CPython extension code
-//!
-//! This exposes methods to build and use a `rustlazyancestors` iterator
-//! from C code, using an index and its parents function that are passed
-//! from the caller at instantiation.
-
-extern crate hg;
-extern crate libc;
-
-mod ancestors;
-pub use ancestors::{
-    rustlazyancestors_contains, rustlazyancestors_drop,
-    rustlazyancestors_init, rustlazyancestors_next,
-};
diff --git a/setup.py b/setup.py
--- a/setup.py
+++ b/setup.py
@@ -3,6 +3,7 @@ 
 #
 # 'python setup.py install', or
 # 'python setup.py --help' for more options
+from __future__ import print_function
 
 import os
 
@@ -141,7 +142,9 @@  hgrustext = os.environ.get('HGWITHRUSTEX
 # TODO record it for proper rebuild upon changes
 # (see mercurial/__modulepolicy__.py)
 if hgrustext != 'cpython' and hgrustext is not None:
-    hgrustext = 'direct-ffi'
+    if hgrustext:
+        print('unkown HGWITHRUSTEXT value: %s' % hgrustext, file=sys.stderr)
+    hgrustext = None
 
 import ctypes
 import errno
@@ -543,7 +546,7 @@  class hgbuildext(build_ext):
         # Build Rust standalon extensions if it'll be used
         # and its build is not explictely disabled (for external build
         # as Linux distributions would do)
-        if self.distribution.rust and self.rust and hgrustext != 'direct-ffi':
+        if self.distribution.rust and self.rust:
             for rustext in ruststandalones:
                 rustext.build('' if self.inplace else self.build_lib)
 
@@ -1391,29 +1394,6 @@  class RustExtension(Extension):
             )
 
 
-class RustEnhancedExtension(RustExtension):
-    """A C Extension, conditionally enhanced with Rust code.
-
-    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):
-        RustExtension.__init__(
-            self, mpath, sources, rustlibname, subcrate, **kw
-        )
-        if hgrustext != 'direct-ffi':
-            return
-        self.extra_compile_args.append('-DWITH_RUST')
-        self.libraries.append(rustlibname)
-        self.library_dirs.append(self.rusttargetdir)
-
-    def rustbuild(self):
-        if hgrustext == 'direct-ffi':
-            RustExtension.rustbuild(self)
-
-
 class RustStandaloneExtension(RustExtension):
     def __init__(self, pydottedname, rustcrate, dylibname, **kw):
         RustExtension.__init__(
@@ -1453,7 +1433,7 @@  extmodules = [
         include_dirs=common_include_dirs,
         depends=common_depends,
     ),
-    RustEnhancedExtension(
+    Extension(
         'mercurial.cext.parsers',
         [
             'mercurial/cext/charencode.c',
@@ -1463,16 +1443,9 @@  extmodules = [
             'mercurial/cext/pathencode.c',
             'mercurial/cext/revlog.c',
         ],
-        'hgdirectffi',
-        'hg-direct-ffi',
         include_dirs=common_include_dirs,
         depends=common_depends
-        + [
-            'mercurial/cext/charencode.h',
-            'mercurial/cext/revlog.h',
-            'rust/hg-core/src/ancestors.rs',
-            'rust/hg-core/src/lib.rs',
-        ],
+        + ['mercurial/cext/charencode.h', 'mercurial/cext/revlog.h',],
     ),
     Extension(
         'mercurial.cext.osutil',