Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(testenv): disable downloads (bitcoind and electrsd) for docs.rs b… #1722

Conversation

riverKanies
Copy link
Contributor

@riverKanies riverKanies commented Nov 15, 2024

…uilds of crate testenv

Description

address this issue #1627
where docs.rs build is failing for bdk_testenv crate

original PR: #1679

Notes to the reviewers

I was not able to reproduce the build issue locally, so this will have to be tested live unless someone else can reproduce the build error https://docs.rs/crate/bdk_testenv/0.10.0/builds/1377651

more details in this thread on discord https://discord.com/channels/753336465005608961/1265333904324427849/1304476756660719668

Bugfixes:

  • I'm linking the issue being fixed by this PR

@ValuedMammal
Copy link
Contributor

Will adding these fix the test error you're seeing?

diff
diff --git a/crates/bitcoind_rpc/Cargo.toml b/crates/bitcoind_rpc/Cargo.toml
index a3426007..6bf55262 100644
--- a/crates/bitcoind_rpc/Cargo.toml
+++ b/crates/bitcoind_rpc/Cargo.toml
@@ -20,9 +20,9 @@ bitcoin = { version = "0.32.0", default-features = false }
 bitcoincore-rpc = { version = "0.19.0" }
 bdk_core = { path = "../core", version = "0.3.0", default-features = false }
 
 [dev-dependencies]
-bdk_testenv = { path = "../testenv", default-features = false }
+bdk_testenv = { path = "../testenv" }
 bdk_chain = { path = "../chain" }
 
 [features]
 default = ["std"]
diff --git a/crates/electrum/Cargo.toml b/crates/electrum/Cargo.toml
index 03c3783b..b34a8d72 100644
--- a/crates/electrum/Cargo.toml
+++ b/crates/electrum/Cargo.toml
@@ -16,9 +16,9 @@ workspace = true
 bdk_core = { path = "../core", version = "0.3.0" }
 electrum-client = { version = "0.21", features = [ "proxy" ], default-features = false }
 
 [dev-dependencies]
-bdk_testenv = { path = "../testenv", default-features = false }
+bdk_testenv = { path = "../testenv" }
 bdk_chain = { path = "../chain" }
 
 [features]
 default = ["use-rustls"]
diff --git a/crates/esplora/Cargo.toml b/crates/esplora/Cargo.toml
index 2f42b4fd..3ebbd59c 100644
--- a/crates/esplora/Cargo.toml
+++ b/crates/esplora/Cargo.toml
@@ -22,9 +22,9 @@ futures = { version = "0.3.26", optional = true }
 miniscript = { version = "12.0.0", optional = true, default-features = false }
 
 [dev-dependencies]
 bdk_chain = { path = "../chain" }
-bdk_testenv = { path = "../testenv", default-features = false }
+bdk_testenv = { path = "../testenv" }
 tokio = { version = "1", features = ["rt", "rt-multi-thread", "macros"] }
 
 [features]
 default = ["std", "async-https", "blocking-https"]
diff --git a/crates/testenv/Cargo.toml b/crates/testenv/Cargo.toml
index 85551673..2b939b59 100644
--- a/crates/testenv/Cargo.toml
+++ b/crates/testenv/Cargo.toml
@@ -16,9 +16,12 @@ readme = "README.md"
 workspace = true
 
 [dependencies]
 bdk_chain = { path = "../chain", version = "0.20.0", default-features = false }
-electrsd = { version = "0.28.0", features = [ "legacy" ] }
+electrsd = { version = "0.28.0", features = [ "legacy" ], default-features = false }
+
+[dev-dependencies]
+bdk_testenv = { path = "." }
 
 [features]
 default = ["std", "download"]
 download = ["electrsd/bitcoind_25_0", "electrsd/esplora_a33e97e1"]

@riverKanies
Copy link
Contributor Author

riverKanies commented Nov 15, 2024

@ValuedMammal that did fix the test! At first I thought this might have unwanted side effects, but after some time I do think this is the solution we want!

P.S. originally I was only able to confirm tests on explora as the tests for other clients were failing for me on master with: Error: JSON-RPC error: transport error: Couldn't connect to host: Resource temporarily unavailable (os error 35)
I found a solution and created issue #1723

all tests are now passing for me locally with your proposed changes VM 🥳

@notmandatory notmandatory added release Release related issue or PR documentation Improvements or additions to documentation labels Nov 18, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 18, 2024
@notmandatory
Copy link
Member

Can you just squash the 2nd commit into the first ? then looks looks good to go. Thanks!

@riverKanies riverKanies force-pushed the fix(testenv)/disable-downloads-testenv-docsrs branch from d4fd746 to fe34cc1 Compare November 18, 2024 15:36
@riverKanies
Copy link
Contributor Author

@notmandatory squashed and rebased to master

@ValuedMammal
Copy link
Contributor

@riverKanies riverKanies force-pushed the fix(testenv)/disable-downloads-testenv-docsrs branch 2 times, most recently from 7b9f349 to f762cad Compare November 18, 2024 18:25
@riverKanies
Copy link
Contributor Author

riverKanies commented Nov 18, 2024

Are you signing git commits also?

https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key

hmm never heard of this. looks like i managed to set it up, now my commits should be tagged as verified 👍

@riverKanies riverKanies force-pushed the fix(testenv)/disable-downloads-testenv-docsrs branch from f762cad to 9b48fd4 Compare November 18, 2024 18:47
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to reproduce the problem locally by running doc.rs locally, but got no success 😢.

It looks pretty straight-forward, and at least running cargo doc locally worked just fine.

crates/testenv/Cargo.toml Show resolved Hide resolved
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9b48fd4 but I guess we won't know for sure until we publish the crate

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9b48fd4

@notmandatory notmandatory merged commit c42b01b into bitcoindevkit:master Nov 21, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation release Release related issue or PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants