-
Notifications
You must be signed in to change notification settings - Fork 56
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
installer: Download and install bitcoind from GUI installer #630
installer: Download and install bitcoind from GUI installer #630
Conversation
Please note that all commits shown so far are from #592. |
@darosior For the download, do you have a preferred HTTP client to use? |
I would personally suggest that you follow the example of https://github.com/iced-rs/iced/tree/0.9/examples/download_progress |
0c8e8b7
to
afd10bf
Compare
Closed this accidentally when rebasing on master. Will reopen again when I have some commits 🥲 |
As part of this PR, would it be worth moving all the |
d8e058c
to
fd95805
Compare
This is now working and ready to try. The presentation probably still needs some work and there may be further refactoring needed. Most of the changes are in 3882f8a and 1e3272e. I've added I've kept all changes to dependencies in their own commits for now in case we want to tweak versions or make things OS-dependent. |
I'm going to test this ASAP. How about splitting off the first, large but trivial, commit into its own PR already? |
I'm testing this under various scenarii, it's working great. Exciting! |
Thanks for your initial feedback.
I think that makes sense. I'll create a separate PR for it.
Yep, I think that's better and will avoid any error messages appearing. Will change it.
Agree, will change that.
Yep, I agree, will change that. |
bc35cda installer: move bitcoind steps to own module (jp1ac4) Pull request description: This was originally part of #630, but will be easier to see changes by moving to its own PR. It simply moves bitcoind-related content of `step/mod.rs` to its own module. ACKs for top commit: darosior: ACK bc35cda -- move-only + adapting includes Tree-SHA512: f3526c7d1d5325f1057f77dac9197d3548a796d860f8612577016138ac47dfab1f8a04b4063e74b69b337c6202999adfecb30c4a7e139e1419416a6906084b33
fd95805
to
ff2e88b
Compare
It looks like the latest version does not download a |
i'm trying build this PR on a fresh install Debian12 and got this issue:
|
it seems it's just about |
It should still download a binary if there isn't already one in the expected location ( |
Thanks for spotting that. Perhaps we could avoid adding this dependency by using a different HTTP client or one of Reqwest's features that uses rustls-tls. |
I'll look into this. |
In my testing it does not.
It did not download and unpack it. |
Actually, if the path to the Removing this would probably fix my bug from above as a side-effect. |
Ah, I'm not sure what was causing that, but there's a more recent version now where I've even removed the "Start bitcoind" button as it now starts automatically. I'll finish some final changes and then mark the PR as ready for review (with possible change to HTTP client pending) and you can check again.
That's true, except currently the location depends on the |
gui/Cargo.lock
Outdated
name = "tar" | ||
version = "0.4.40" | ||
source = "registry+https://github.com/rust-lang/crates.io-index" | ||
checksum = "b16afcea1f22891c49a00c751c7b63b2233284064f11a200fc624137c51e2ddb" | ||
dependencies = [ | ||
"filetime", | ||
"libc", | ||
"xattr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here it looks like we don't need xattr
, so maybe it should work to specify no-default-features
for tar
too?
I've done the move to reqwest with rustls along with addressing my comments above here: https://github.com/darosior/liana/tree/2308_jp_download_bitcoind. I'll perform a release build on this branch to make sure we haven't introduced new system dependencies. |
a6f78fe
to
d0ad3ff
Compare
Thanks for the suggested changes to dependencies. I've just committed those and dropped the I did those changes in two commits as EDIT: At the end, I can squash these commits further. |
This looks good to me from testing. I've also successfully built this using our reproducible build system for releases. No new dependency introduced. @kloaec is currently testing a reproducibly-built binary on Windows. |
From @kloaec's testing on Windows: |
Installing |
@@ -26,7 +26,9 @@ use crate::{ | |||
installer::{ | |||
context::Context, | |||
message::{self, Message}, | |||
prompt, Error, | |||
prompt, | |||
step::{DownloadState, InstallState}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I prefer to not have circular dependencies. Can you cut the view in multiple functions and call in the step method view the one according to the current step state ? Or would you prefer we do it in other PRs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that's a good point. I'll have a look now at how best to split it and if I can do it quickly, but otherwise it might be better in a follow-up PR in order not to delay the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be OK to define parts of the view within the step as an Element
and pass this to the view function? That way I could match
on different values of DownloadState
and InstallState
within the step and avoid the need to define different functions for each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followed up PR I would prefer finally
Note to self: make sure the wait for the cookie file is enough for mainnet. (Most likely not) EDIT: we should probably use try_wait as a less brittle way of detecting if it errored. |
e0d15f8
to
bf67f94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bf67f94 -- modulo a few changes we'll address in follow-ups. I've not reviewed the code but significantly tested it on both Windows and Linux.
Congrats @jp1ac4 on keeping up with the review (totaling above 200 review comments on boths PRs) and all the challenges. |
@edouardparis i'll let you have the final call on this |
…llet on Windows 5680ad2 lib: on Windows, migrate the watchonly wallet from bitcoind datadir (Antoine Poinsot) c9d86f1 lib: implement a superior workaround for the watchonly wallet on Windows (Antoine Poinsot) Pull request description: See the added comment for the details. No need to store the watchonly wallet under bitcoind's datadir anymore. 🎉 🎉 I've noticed this while working on fixing #630 on Windows which failed for the same root reason as why the watchonly wallet path didn't work on Windows. Fixes #653. ACKs for top commit: darosior: ACK 5680ad2 -- tested it on Windows edouardparis: utACK 5680ad2 Tree-SHA512: 52158340097e286d882e6503d8bc1fbd4653729c08055cd2609a120aabc409ed38cbd40c7cb0c3a6cb9c67f163f8084fd886daa979e56ccf6223eb51773500ef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bf67f94
edb2c51 Bump lianad version to 2.0 (Antoine Poinsot) e9a15fd CHANGELOG: update for v2 (Antoine Poinsot) Pull request description: This is assuming #630 is merged, but not the fix for #323 (can be added to the CHANGELOG if it does). ACKs for top commit: darosior: self-ACK edb2c51 -- simply docs and a version bump Tree-SHA512: f08270dc76d2ed3d33e57cef503902d256e5d1bd4d3c933b380f6b567e68feb0998e56ba39855511b33f3a3c2a7d654bc24193033705495311b5a7497f191c3d
This is a follow-up PR to #592 as part of #570 to download and install bitcoind.
I'm creating this draft PR now to facilitate discussion. Once #592 has been merged, I'll rebase on master.