Chore: remove tempdir dependency by jaoleal · Pull Request #153 · sedited/rust-bitcoinkernel

jaoleal · 2026-04-07T17:20:01Z

This PR removes the tempdir crate that is deprecated.

The removal reflected some changes on tests as expected.

The created path was also exported for those tests but they weren't used and I took the chance to remove those vars.

alexanderwiederin · 2026-04-08T08:13:23Z

~~I think this should build on #150 - just be aware that we may need to rebase.~~

To replicate the behaviour of tempdir we also need to clean up the directories after the tests have run. To make it reusable, I would recommend adding a TempDir struct similar to:

pub struct TempDir {
    data_dir: std::path::PathBuf,
    blocks_dir: std::path::PathBuf,
}

impl TempDir {
    pub fn new(name: &str) -> Self {
        let data_dir = std::env::temp_dir().join(name);
        std::fs::create_dir_all(&data_dir).expect("failed to create temp dir");
        let blocks_dir = data_dir.join("blocks");
        Self { data_dir, blocks_dir }
    }

    pub fn data_dir(&self) -> &str {
        self.data_dir.to_str().expect("temp dir path is not valid UTF-8")
    }

    pub fn blocks_dir(&self) -> &str {
        self.blocks_dir.to_str().expect("temp dir path is not valid UTF-8")
    }
}

impl Drop for TempDir {
    fn drop(&mut self) {
        let _ = std::fs::remove_dir_all(&self.data_dir);
    }
}

To avoid exposing this in the public API, I think this struct is simple enough to be duplicated in src/test_utils.rs and test/common.rs files.

What do you think?

jaoleal · 2026-04-08T19:31:40Z

What do you think?

Cant we reach the same behavior with a versioning or numbering of the directory ?

alexanderwiederin · 2026-04-09T12:16:49Z

Cant we reach the same behavior with a versioning or numbering of the directory ?

I don't think so. Numbering/versioning would help with uniqueness, but that is a separate concern from cleanup.

jaoleal · 2026-04-09T21:35:11Z

done, @alexanderwiederin!

alexanderwiederin

Cool!

alexanderwiederin · 2026-04-10T08:55:22Z

Also don't forget to remove tempdir from the Cargo.toml :)

Without commited lock files, CI dependency resolution can pull in transitive dependencies that require a newer compiler than the project's MSRV. The triggering issue was that bindgen v0.72 depends on rustc-hash v2.x, and the recent release of rustc-hash v2.1.2 introduced a rustc 1.77.0 requirement, breaking the MSRV build against rustc 1.71.0. Cargo-minimal.lock pins dependencies to their oldest MSRV-compatible versions. Cargo-recent.lock pins to the latest patch versions compatible with the MSRV compiler. The CI msrv-check job now runs twice as a matrix, once with each lock file, similar to the approach used by rust-bitcoin. Both lockfiles pin rustc-hash below 2.1.2 to avoid the incompatible release, to 2.1.0 in the minimal lockfile and 2.1.1 in the recent lockfile. tempdir is bumped to >=0.3.6 in dev-dependencies to prevent it from resolving to an older version that depends on an incompatible rand release under minimal-versions. See also: sedited#147

jaoleal · 2026-04-15T17:23:06Z

Sorry, fighting the CI.

Okay, addressed suggestions on 30cb2ad , to avoid that duplication and repetition which could lead to code drift i preferred another approach similar to whats done on bip39 rust crate

alexanderwiederin · 2026-04-15T18:17:05Z

Okay, addressed suggestions on 30cb2ad , to avoid that duplication and repetition which could lead to code drift i preferred another approach similar to whats done on bip39 rust crate

Appreciate you looking at rust-bip39.

From what I understand rand is an optional dependency in Cargo.toml which is re-exported behind #[cfg(feature = "rand")] so the benches get access to a compatible version.

I don't think that is quite what we need. Do you agree?

jaoleal · 2026-04-15T18:37:20Z

I don't think that is quite what we need. Do you agree?

I thought that importing itself as a dev dependency is weird too, agree with that... But that addresses a drift problem if this new struct thats being introduced were evolved or refined, avoiding a possible maintenance burn, it also sounds solid if new test components are needed. For me that was enough to suggest it here but i think is as valid as your suggestion, do you want for me to stick with it ?

alexanderwiederin

I appreciate the maintenance argument, but I think we should be conservative about what we expose as a feature. TempDir is test infrastructure and I'd rather not have it appear in the public API even behind a feature gate. Given how stable the struct is likely to be, I think the duplication cost is acceptable. I'd go with duplication.

This commit removes tempdir as a development dependency, it was used to create test directories that were removed on drop. In its place is made a TempDir that mimics such behavior but provide specifics for our test cases, the new structure is present both in src/test_utils.rs and tests/common.rs

jaoleal · 2026-04-16T17:45:52Z

I'd rather not have it appear in the public API even behind a feature gate

Yes, thats a considerable flaw.

Applied suggestions on 3c65b25

Plase take a look

alexanderwiederin

Thanks for addressing that!

I think the number of characters per line in the body of the commit message is too long. Please follow https://cbea.ms/git-commit/#wrap-72, which suggest a maximum character length of 72.

We should also add to the commit message why TempDir is duplicated. Something like: "TempDir is duplicated in src/test_utils.rs and tests/common/mod.rs to avoid exposing it in the public API."

alexanderwiederin · 2026-04-17T11:04:02Z

@@ -0,0 +1,47 @@
+use std::sync::atomic::{AtomicU64, Ordering};


I found this log running the tests:

test result: ok. 233 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s Running tests/common.rs (target/debug/deps/common-9c3bf7c061820654)

Which means that tests/common.rs is being compiled as its own test binary. From what I understand the conventional fix is to move the file to tests/common/mod.rs, which cargo won't compile as a standalone binary.

Can we try that?

alexanderwiederin · 2026-04-17T11:06:19Z

+
+static COUNTER: AtomicU64 = AtomicU64::new(0);
+
+/// Utility to create temporary directories that are cleaned on drop. Exact same struct as found in `src/test_utils.rs`.


I think this should be a normal comment:

// Utility to create temporary directories that are cleaned on drop. Duplicated in `src/test_utils.rs` and `tests/common/mod.rs`.

It's describing an implementation details for maintainers (the duplication), not documenting a public API.

Same applies to src/test_utils.rs.

alexanderwiederin · 2026-04-17T11:07:14Z

+}
+
+impl TempDir {
+    /// Creates a new `TempDir` from a given name concatenating it with an unique id.


This can be removed along with line 28 and 35. It's self-explanatory. The less content the files have the smaller the risk of drift.

Same applies to src/test_utils.rs.

jaoleal force-pushed the remove_tempdir branch 2 times, most recently from 68420bd to 0b7bbdd Compare April 9, 2026 21:34

alexanderwiederin suggested changes Apr 10, 2026

View reviewed changes

Comment thread src/ffi/test_utils.rs Outdated

Comment thread src/ffi/test_utils.rs Outdated

Comment thread tests/test.rs Outdated

Comment thread src/ffi/test_utils.rs Outdated

Comment thread tests/test.rs Outdated

jaoleal force-pushed the remove_tempdir branch 4 times, most recently from 71207d6 to 30cb2ad Compare April 15, 2026 17:16

alexanderwiederin reviewed Apr 15, 2026

View reviewed changes

Comment thread src/test_utils.rs Outdated

Comment thread src/test_utils.rs Outdated

jaoleal force-pushed the remove_tempdir branch from 30cb2ad to 57aa4be Compare April 16, 2026 16:18

jaoleal force-pushed the remove_tempdir branch from 57aa4be to 3c65b25 Compare April 16, 2026 17:36

jaoleal requested a review from alexanderwiederin April 16, 2026 17:46

alexanderwiederin reviewed Apr 17, 2026

View reviewed changes

		@@ -0,0 +1,47 @@
		use std::sync::atomic::{AtomicU64, Ordering};


		static COUNTER: AtomicU64 = AtomicU64::new(0);

		/// Utility to create temporary directories that are cleaned on drop. Exact same struct as found in `src/test_utils.rs`.

Conversation

jaoleal commented Apr 7, 2026

Uh oh!

alexanderwiederin commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaoleal commented Apr 8, 2026

Uh oh!

alexanderwiederin commented Apr 9, 2026

Uh oh!

jaoleal commented Apr 9, 2026

Uh oh!

alexanderwiederin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexanderwiederin commented Apr 10, 2026

Uh oh!

jaoleal commented Apr 15, 2026

Uh oh!

alexanderwiederin commented Apr 15, 2026

Uh oh!

jaoleal commented Apr 15, 2026

Uh oh!

alexanderwiederin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexanderwiederin left a comment

Choose a reason for hiding this comment

Uh oh!

alexanderwiederin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

alexanderwiederin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

alexanderwiederin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexanderwiederin commented Apr 8, 2026 •

edited

Loading

jaoleal commented Apr 16, 2026 •

edited

Loading