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

[delta1]

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]

utACK 3720b7d

[shesek]

Seems like this slipped in by mistake?

[shesek]

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]

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]

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]

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]

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]

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]

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]

I am not convinced because it can cause other issues but ok.
Is something like this what you have in mind? rust-bitcoin/bitcoind#154 (review)

[shesek]

Is something like this what you have in mind? rust-bitcoin/bitcoind#154 (review)

Yes exactly 🙏

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]

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]

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 shesek/electrumd#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]

I already did elementsd

[shesek]

I already did elementsd

Cool! I opened #79 to update the dependencies here.