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.

Sign up for GitHub

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

Jump to bottom

feat: optional download #72

Merged
merged 4 commits into from
Oct 18, 2023
Merged

feat: optional download #72

merged 4 commits into from
Oct 18, 2023

Conversation

realeinherjar
Copy link
Contributor

@realeinherjar realeinherjar commented Oct 13, 2023 •
edited
Loading

  • bump version to 0.26.0
  • normalize ENV vars as the same prefix bitcoind: ELECTRS_EXE -> ELECTRS_EXEC
  • optionally download when building to allow Nix builds
  • fix missnamed feature in README.md: bitcoind_23_0 -> bitcoind_23_1
  • introduces which to find executables
  • introduces CI to test without auto-download features

This makes a Nix CI pass, check: https://github.com/realeinherjar/bdk/actions/runs/6512925971/job/17691535222 (ignore the failed ones, MSRV, I am still working on those)

@realeinherjar realeinherjar changed the title Einherjar/optional download [WIP] Optional Download Oct 13, 2023
@realeinherjar realeinherjar marked this pull request as draft October 13, 2023 20:02
@realeinherjar realeinherjar mentioned this pull request Oct 13, 2023
Closed
45 tasks
@realeinherjar realeinherjar marked this pull request as ready for review October 13, 2023 20:46
@realeinherjar realeinherjar changed the title [WIP] Optional Download Optional Download Oct 13, 2023
@realeinherjar realeinherjar changed the title Optional Download feat: optional download Oct 13, 2023
notmandatory
notmandatory reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@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.

Concept ACK, this will also help reduce number of dependencies we need to pin for MSRV when download is disabled (eg. using nix to get the binaries).

Cargo.toml Outdated Show resolved Hide resolved
notmandatory
notmandatory reviewed Oct 13, 2023
View reviewed changes
build.rs Outdated Show resolved Hide resolved
@notmandatory
Copy link
Contributor

Unrelated, but you should also fix the README example which still says bitcoind_23_0 but should be bitcoind_23_1.

@realeinherjar
Copy link
Contributor Author

Unrelated, but you should also fix the README example which still says bitcoind_23_0 but should be bitcoind_23_1.

Good call, I've fixed as well

RCasatta
RCasatta reviewed Oct 16, 2023
View reviewed changes
Copy link
Owner

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

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

Thanks for this,
ideally, a CI job without features using ELECTRS_EXEC (downloaded by the CI) would be needed

src/lib.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
@realeinherjar
feat: optionally download
79f5e3d 
fix Nix builds

fix: missnamed const in lib.rs

fix Nix builds
@realeinherjar
Copy link
Contributor Author

ideally, a CI job without features using ELECTRS_EXEC (downloaded by the CI) would be needed

See if 6df6063 is enough

@realeinherjar
Copy link
Contributor Author

Ok, this should be ready to merge now :)

RCasatta
RCasatta approved these changes Oct 18, 2023
View reviewed changes
Copy link
Owner

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

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

ACK 2989905

looking great, thanks, especially for updating the readme.

Going to release in a while

src/lib.rs
env::set_var("ELECTRS_EXEC", "placeholder");
env::set_var("ELECTRS_EXE", "placeholder");
assert!(exe_path().is_err());
// unsetting because this errors everything in mod test!
Copy link
Owner

Choose a reason for hiding this comment

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

Since the tests are run in parallel I have issues with this locally even with removes, I'll add #[ignore] in a follow-up commit to avoid a roundtrip. (so that it can be launched individually with --include-ignored)

.github/workflows/cont_integration.yml
- uses: dtolnay/rust-toolchain@stable
- name: Install electrs
# Automatically cache installed binaries to avoid compiling them each run
uses: baptiste0928/cargo-install@v2
Copy link
Owner

Choose a reason for hiding this comment

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

I would have gone wgetting the executable from the same url the autodownload feature is getting it, however this is more useful for testing latest version

@RCasatta RCasatta merged commit 2989905 into RCasatta:master Oct 18, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCasatta RCasatta RCasatta approved these changes

@notmandatory notmandatory Awaiting requested review from notmandatory

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use which to detect electrs in the PATH
3 participants
@realeinherjar @notmandatory @RCasatta