avoid setting {BITCOIND,ELEMENTSD}_EXE in setup #88
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The logic of setting the env var inside the setup prevents user of other OS like nixos to run the tests (because the included binary are not working on nixos). Setting those env vars is meant to be done in the dev environment not in the code. With this users with `bitcoind` and `elementsd` in their PATH will work too. The CI script will set the variables only if they are not already set. (Allowing Nixos users to override and run it locally)
Member
|
This seems fine to me, but I wouldn't say it's "better" since the upstream one was designed so that |
Collaborator
Author
in upstream you must override BITCOIND_EXE or it would not work in nixos? |
Member
|
Oh, nice, I see. |
Member
|
FYI I am holding off on merging until #88 gets in because I think I will be able to run my full local CI setup on the merge commit once that's done. |
apoelstra
added a commit
that referenced
this pull request
Aug 9, 2024
e151a85 ci: bump fuzz MSRV to 1.65 (Andrew Poelstra) b10c7a2 ci: add one more pin (Andrew Poelstra) 7c977b5 ForEachKey: clean up Concrete impl, remove Semantic impl (Andrew Poelstra) 8bc9311 doc: improve list of fields covered by sighash (Andrew Poelstra) 97b2229 clippy: unnecessary vecs (Andrew Poelstra) 8894215 clippy: miscellaneous simple fixes (Andrew Poelstra) fcb2c3d clippy: canonical partial_cmp implementations (Andrew Poelstra) bb88c18 clippy: replace fold calls with try_fold (Andrew Poelstra) 9815ca7 rustc: silence some "dead code" warnings (Andrew Poelstra) 4bab769 rustfmt: change to new notation (Andrew Poelstra) de4e2ec cargo: lint unused cfg parameters (Andrew Poelstra) Pull request description: Probably needs #88 to go in in parallel. ACKs for top commit: RCasatta: utACK e151a85 Tree-SHA512: 80260fa8b38c543b437230b1f6e2464ef97a1755ff3565ae8204f7fbfe55fe0b5a197632de48dce8bc2e0170d0e30f5a3676314f62ce61cee8b5b0c6b7c83487
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The logic of setting the env var inside the setup prevents user of other OS like nixos to run the tests (because the included binary are not working on nixos) and also to run other specific versions.
Setting those env vars is meant to be done in the dev environment not in the code, so this remove the setting of the env vars in the code.
With this users with
bitcoindandelementsdin their PATH will work too.The CI script will set the variables with the included binary only if they are not already set. This allows the CI to run while allowing other users to override with specific versions and run it locally.
Note this is different from what has been done upstream rust-bitcoin/rust-miniscript#538 but I think it's better