search

Blockstream / electrs

An efficient re-implementation of Electrum Server in Rust
MIT License
301 stars 125 forks source link

test: enable sandbox build with --no-default-features #73

Closed delta1 closed 6 months ago

delta1 commented 6 months ago

I'm making a nixpkgs derivation for this repo, which is built in a sandbox with no network access.

This PR allows tests to be run with --no-default-features to disable the binary downloads in the bitcoind, elementsd, and electrumd dev dependencies. Default features will still download them.

RCasatta commented 6 months ago

utACK 3720b7ddf2ca6f522e2421c653815ebdb981983d

shesek commented 6 months ago

This PR allows tests to be run with --no-default-features to disable the binary downloads

Run which tests though? This seems to disable the tests entirely, not just the binary downloads

If the goal is to skip tests entirely, couldn't the nixpkg process skip cargo test (and not install the dev deps)?

Out of curiosity, would it be possible to get nix to supply the binaries in the expected location, so that the build stage skips downloading them but the tests would still work?

delta1 commented 6 months ago

Run which tests though?

it only runs these 

test rest::tests::test_parse_query_param ... ok
test rest::tests::test_parse_value_param ... ok

couldn't the nixpkg process skip cargo test

yes i'll just do that.

RCasatta commented 5 months ago

Out of curiosity, would it be possible to get nix to supply the binaries in the expected location, so that the build stage skips downloading them but the tests would still work?

For this purpose bitcoind crate allows to provide an env var BITCOIND_TARBALL_FILE with the expected tar.gz and then unpacks that instead of trying to download and also verify hashes

RCasatta commented 5 months ago

couldn't the nixpkg process skip cargo test

tests can be skipped but dev-dependencies are in the Cargo.lock on which tools like crane are based on, thus failing the build

It may be the case to re-consider this

shesek commented 5 months ago

Is there no way to tell crane to ignore the dev deps? Or perhaps strip them from Cargo.lock in a pre build hook?

Alternatively, maybe add an env var build option for {bitcoin,elements,electrum}d that skips downloads?

It may be the case to re-consider this

It's possible, but adding a new feature for this feels kinda hacky >.<

RCasatta commented 5 months ago

Or perhaps strip them from Cargo.lock in a pre build hook?

this seems more hacky than adding a feature for this?

Alternatively, maybe add an env var build option for {bitcoin,elements,electrum}d that skips downloads?

the option to skip downloads is not specifying the feature to download?

shesek commented 5 months ago

this seems more hacky than adding a feature for this?

Yes, stripping it like that is admittedly more hacky >.<

But this solution is at least isolated in the context where its needed and doesn't effect other upstream projects.

the option to skip downloads is not specifying the feature to download?

I'm a bit hesitant about adding a feature for this because then we have to make sure that every script or CI configuration with --no-default-features remembers to re-enable it, as well as mention this in the documentation so users know to do it. It also requires the #[cfg(feature)] gating (plus the conditional allow(dead_code) to silence warnings) added to every test.

And then, if you have another project that depends on electrs that also needs to be built in nix, it will have to forward the right features down to electrs. Right now there are no other default features so its just a simple no-default-features, but if there were it would need explicitly re-enable them and make sure to keep the list updated. (And the same goes for projects that depend on that project...)

OTOH, if this used a build time environment variable instead, it could be passed directly from the nix building environment down to the crates that need it ({bitcoin,elements,electrum}d), without having every crate in the tree before them forward it down.

RCasatta commented 5 months ago

I am not convinced because it can cause other issues but ok. Is something like this what you have in mind? https://github.com/rust-bitcoin/bitcoind/pull/154#pullrequestreview-1973687880

shesek commented 5 months ago

Is something like this what you have in mind? https://github.com/rust-bitcoin/bitcoind/pull/154#pullrequestreview-1973687880

Yes exactly :pray:

I can do the same for electrumd

can cause other issues but ok

You mean that the env var approach can cause other issues?

RCasatta commented 5 months ago

You mean that the env var approach can cause other issues?

Yes, like having a different version of the daemon in the PATH in comparison to the specified feature

I can do the same for electrumd

great!

shesek commented 5 months ago

Yes, like having a different version of the daemon in the PATH in comparison to the specified feature

Well you can still use TARBALL_FILE if you intend to use the crate and want to provide the files yourself (& have them hash-verified to be correct and matching the specified version), but SKIP_DOWNLOAD seems useful for cases where you don't intend to use the crate at all?

Also, IIUC mismatching the feature version and actual version was already possible via BITCOIND_EXE, right?

I can do the same for electrumd

great!

Added to electrumd in https://github.com/shesek/electrumd/pull/1

A similar change is also needed in elementsd.

And we'll also need to update electrs to use the latest {bitcoin,elements,electrum}d releases.

RCasatta commented 5 months ago

I already did elementsd

shesek commented 5 months ago

I already did elementsd

Cool! I opened https://github.com/Blockstream/electrs/pull/79 to update the dependencies here.