Add eckit crate to workspace by Choochmeque · Pull Request #322 · ecmwf/eckit

Choochmeque · 2026-07-29T18:43:01Z

Description

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-322

codecov-commenter · 2026-07-29T18:54:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.60%. Comparing base (bb29250) to head (150889e).
⚠️ Report is 7 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #322      +/-   ##
===========================================
- Coverage    67.60%   67.60%   -0.01%     
===========================================
  Files         1182     1182              
  Lines        61697    61697              
  Branches      4667     4667              
===========================================
- Hits         41713    41710       -3     
- Misses       19984    19987       +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ata handling

Copilot

Pull request overview

This PR adds a new eckit Rust crate to the rust/ workspace, providing a safe, higher-level wrapper API over the existing eckit-sys FFI bindings (configuration, data handles, message reading, and stream serialization).

Changes:

Introduces the new rust/crates/eckit crate with wrappers for Config, DataHandle, Message/MessageReader, and Stream (TCP + in-memory).
Adds the eckit crate to the Rust workspace members and workspace dependencies.
Adds a small build script to re-export the ECKIT_SYS dependency root for downstream consumers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
rust/crates/eckit/src/lib.rs	New crate root, module wiring, and public re-exports; adds `init()` helper.
rust/crates/eckit/src/error.rs	Defines crate-wide `Result` and re-exports `eckit_sys::Error`.
rust/crates/eckit/src/config.rs	Safe wrapper around `LocalConfiguration` with typed get/set and sub-iteration.
rust/crates/eckit/src/datahandle.rs	Typestate `DataHandle` wrapper with `Read/Seek/Write` integration and factories.
rust/crates/eckit/src/message.rs	GRIB message abstraction and streaming reader with lifetime tying to the source handle.
rust/crates/eckit/src/stream.rs	Stream trait + TCP/memory stream wrappers and primitive (de)serialization traits.
rust/crates/eckit/Cargo.toml	New crate manifest, features mapped to `eckit-sys`, and build dependencies.
rust/crates/eckit/build.rs	Re-exports `ECKIT_SYS` dep root via `bindman-utils`.
rust/Cargo.toml	Adds `crates/eckit` to workspace members and workspace dependencies.

Suppressed comments (2)

rust/crates/eckit/src/datahandle.rs:284

cast_unsigned() is not defined on i64/u64 in this crate, so this line will not compile. Since new is already checked to be non-negative, cast to u64 directly (or use u64::try_from(new)).

                if new < 0 {
                    return Err(std::io::Error::other("seek to negative position"));
                }
                new.cast_unsigned()

rust/crates/eckit/src/stream.rs:207

There’s an unsafe impl Send here without a SAFETY rationale, while other types in this crate (e.g. DataHandle, Config) document the safety argument. Adding a short SAFETY comment makes the unsafe contract clearer for maintainers.

#[allow(clippy::non_send_fields_in_send_ty)]
unsafe impl Send for MemoryStream {}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Choochmeque · 2026-08-05T16:45:24Z

+    /// The C++ side calls back into the Rust reader on each `read()`; no
+    /// intermediate buffer or temp file is staged. Forward-only — the
+    /// resulting handle cannot be seeked or written to.


Fixed in 042f28c.

Choochmeque · 2026-08-05T16:45:28Z

+    pub fn as_sys(&self) -> &eckit_sys::MessageWrapper {
+        // inner is always Some for a valid Message
+        &self.inner
+    }


Fixed in 110e0bd.

Choochmeque · 2026-08-05T16:45:29Z

+#[allow(clippy::non_send_fields_in_send_ty)]
+unsafe impl Send for TcpStream {}


Fixed in 150889e.

caraghbiner

Semantically okay I think, though flush should not be a NOOP.

caraghbiner · 2026-08-05T15:14:43Z

+    /// `eckit::DataHandle::flush()` is virtual but its base implementation
+    /// throws `NotImplemented`, so most handle types do not actually
+    /// support a mid-stream flush. The commit boundary for write-side
+    /// handles is [`DataHandle::close`] — call it explicitly to surface
+    /// any flush/commit errors. This `Write::flush` impl is therefore a
+    /// no-op by design.
+    fn flush(&mut self) -> std::io::Result<()> {


I definitely don't think "flush" should be a hardcoded to be a noop (is this masking all concrete implementations of flush?). That is a decision for the underlying implementation to make: if it wants to raise a NotImplemented error, then this should error.

Please make sure no other functions are masking the implemented behaviour.

Fixed in 5428123.

caraghbiner · 2026-08-05T15:43:33Z

+    }
+
+    /// Access the underlying C++ `DataHandleWrapper` for FFI interop.
+    pub fn inner_mut(&mut self) -> Result<std::pin::Pin<&mut eckit_sys::DataHandleWrapper>> {


Does this need to be pub instead of e.g. pub(crate)?

Fixed in 9be04d6.

caraghbiner · 2026-08-05T15:45:00Z

+impl DataHandle<Closed> {
+    /// Create from a raw eckit-sys wrapper.
+    #[must_use]
+    pub const fn from_raw(inner: eckit_sys::UniquePtr<eckit_sys::DataHandleWrapper>) -> Self {


Constructs a DataHandle<closed> but what if the raw data handle was already opened?

Fixed in 4f0ab97.

caraghbiner · 2026-08-05T15:50:33Z

+    /// Wrap a Rust `std::io::Read` source as a `DataHandle`.
+    ///
+    /// The C++ side calls back into the Rust reader on each `read()`; no
+    /// intermediate buffer or temp file is staged. Forward-only — the
+    /// resulting handle cannot be seeked or written to.
+    pub fn from_reader<R>(reader: R) -> Result<Self>
+    where
+        R: std::io::Read + std::io::Seek + Send + 'static,
+    {
+        let inner = eckit_sys::DataHandleWrapper::from_reader(eckit_sys::make_reader_box(reader))


I am confused about whether this object should be seekable, or not.

Implies we will only ever read:

/// Forward-only — the resulting handle cannot be seeked

Implies we must be able to seek:

R: Read + Seek + Send + 'static

Fixed in 042f28c.

caraghbiner · 2026-08-05T16:09:17Z

Are we always reporting IO errors as io::Error::other?

It would be nice to map the eckit errors to matching std::io error kinds if we can. e.g.

fn to_io_error(error: eckit_sys::Error) -> std::io::Error { use std::io::{Error, ErrorKind}; match error { eckit_sys::Error::NotImplemented(message) => { Error::new(ErrorKind::Unsupported, message) } // ... // etc ... // .... other => Error::other(other.to_string()), } }

Fixed in 042f28c.

…move unused `pin_mut` method.

…tation

Choochmeque and others added 2 commits June 11, 2026 02:21

Add eckit crate to workspace

1e6349e

Merge branch 'develop' into rust-binding-high-level-crate

f0043b3

Choochmeque added 2 commits July 29, 2026 20:56

Refactor eckit_sys usage to use wrapper types for configuration and d…

3f71000

…ata handling

Remove unused from_raw method from Config implementation

14d714b

caraghbiner requested a review from Copilot August 5, 2026 14:13

Copilot started reviewing on behalf of caraghbiner August 5, 2026 14:14 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

caraghbiner requested changes Aug 5, 2026

View reviewed changes

Choochmeque added 7 commits August 5, 2026 18:27

Add flush method to DataHandleWrapper for improved data handling

5428123

Update DataHandle documentation and error handling

042f28c

Add documentation to from_raw method in DataHandle

4f0ab97

Make inner_mut method crate-private for better encapsulation and re…

9be04d6

…move unused `pin_mut` method.

Update documentation examples to use String::new() for default values

719605a

Remove unnecessary comment from as_sys method in Message implemen…

110e0bd

…tation

Add safety comments to TcpStream and MemoryStream implementations

150889e

		#[allow(clippy::non_send_fields_in_send_ty)]
		unsafe impl Send for TcpStream {}

Uh oh!

Conversation

Choochmeque commented Jul 29, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Declaration

Uh oh!

codecov-commenter commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caraghbiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Choochmeque commented Jul 29, 2026 •

edited by github-actions Bot

Loading

codecov-commenter commented Jul 29, 2026 •

edited

Loading