Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 2, 2023

This PR makes the bitcoin-wallet binary path customizable in the same way how it can be done now with other ones, including bitcoind, bitcoin-cli and bitcoin-util.

@hebasto hebasto added the Tests label May 2, 2023
@DrahtBot
Copy link
Contributor

DrahtBot commented May 2, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto hebasto force-pushed the 230502-toolwallet branch from 018c8bc to 0079c55 Compare May 2, 2023 16:35
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK ef01337

Nice little refactoring (modulo being able to now set BITCOINWALLET which is new behaviour), less code duplication and functions with smaller scope.

@hebasto hebasto force-pushed the 230502-toolwallet branch from ef01337 to de8b7ab Compare May 5, 2023 12:01
@hebasto
Copy link
Member Author

hebasto commented May 5, 2023

Updated ef01337 -> de8b7ab (pr27554.03 -> pr27554.04, diff):

hebasto added 2 commits May 5, 2023 13:35
This change factors out the repeated code into a new `set_binary_paths`
function.
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.
@hebasto hebasto force-pushed the 230502-toolwallet branch from de8b7ab to f6d7636 Compare May 5, 2023 12:35
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

re-ACK f6d7636

@fanquake fanquake merged commit 5566405 into bitcoin:master May 5, 2023
@hebasto hebasto deleted the 230502-toolwallet branch May 5, 2023 16:36
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2023
@hebasto hebasto mentioned this pull request May 5, 2023
16 tasks
fanquake added a commit to bitcoin-core/gui that referenced this pull request May 23, 2023
…BITCOINUTIL` and `BITCOINTX`

4f2f615 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov)

Pull request description:

  This PR is a continuation of changes to our testing frameworks (bitcoin/bitcoin#27554, bitcoin/bitcoin#27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](bitcoin/bitcoin#25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories.

  The commit has been pulled from hebasto/bitcoin#15 and it seems [useful](hebasto/bitcoin#15 (comment)) by itself as:
  > I believe the rationale for allowing to drop in the executables via env var is to allow to test the guix-produced, or other third-party-produced executables...

  The current implementation of the `test/functional/test_framework/test_framework.py` script uses the same approach: https://github.com/bitcoin/bitcoin/blob/09351f51d279612973ecd76811dc075dff08209f/test/functional/test_framework/test_framework.py#L231-L246

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4f2f615
  TheCharlatan:
    ACK 4f2f615
  stickies-v:
    ACK 4f2f615

Tree-SHA512: 99ee9a727b266700649d8f2ec528dfaeb04a1e48f0cb1d4eeaece65917165be647c10c4548429a9e8b30d094597f67e860c1db03ac689ebb409b223ce1b63aa9
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 23, 2023
…TIL` and `BITCOINTX`

4f2f615 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov)

Pull request description:

  This PR is a continuation of changes to our testing frameworks (bitcoin#27554, bitcoin#27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](bitcoin#25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories.

  The commit has been pulled from hebasto#15 and it seems [useful](hebasto#15 (comment)) by itself as:
  > I believe the rationale for allowing to drop in the executables via env var is to allow to test the guix-produced, or other third-party-produced executables...

  The current implementation of the `test/functional/test_framework/test_framework.py` script uses the same approach: https://github.com/bitcoin/bitcoin/blob/09351f51d279612973ecd76811dc075dff08209f/test/functional/test_framework/test_framework.py#L231-L246

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4f2f615
  TheCharlatan:
    ACK 4f2f615
  stickies-v:
    ACK 4f2f615

Tree-SHA512: 99ee9a727b266700649d8f2ec528dfaeb04a1e48f0cb1d4eeaece65917165be647c10c4548429a9e8b30d094597f67e860c1db03ac689ebb409b223ce1b63aa9
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.

Github-Pull: bitcoin#27554
Rebased-From: f6d7636
@bitcoin bitcoin locked and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants