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

Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled #12246

Merged
merged 2 commits into from Sep 27, 2018

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jan 23, 2018

Includes #5618 (which the reasons for rejecting no longer hold true)

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 13, 2018

This change is build-system heavy, @theuni can you take a look?

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 21, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14284 (build: Add MSVC project files for bitcoin-wallet-tool by ken2812221)
  • #14283 (WIP: Add wallet tool test by promag)
  • #13926 ([Tools] bitcoin-wallet - a tool for creating and managing wallets offline by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Sep 24, 2018

utACK the bugfix. Didn't look at the first commit.

@theuni
Copy link
Member

@theuni theuni commented Sep 26, 2018

Seems a bit unnecessary, but I can't come up with a justifiable reason not to.
-0 on the concept.

The changes themselves look sane, though. utACK a2a04a5.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK a2a04a5 both commits. I think configuration options like this are useful so you can choose the components you need in a set-and-forget config step, and then use normal make, make install, and make check commands.

A previous comment (#5618 (comment)) seemed to suggest that these autoconf options are useless because you could alternately run commands like make -C src bitcoin-tx to selectively build components. But that's cumbersome and unreliable and also doesn't work with build actions like "make check" and "make install".

Only minor suggestion I'd make is to rephrase "Make it possible to build only one of bitcoin-cli or bitcoin-tx" commit description so it doesn't sound like these are exclusive options. When I first saw this this PR I thought it was going to disallow building both tools at the same time.

@MarcoFalke MarcoFalke merged commit a2a04a5 into bitcoin:master Sep 27, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarcoFalke added a commit that referenced this pull request Sep 27, 2018
…abled

a2a04a5 Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled (Luke Dashjr)
92af71c configure: Make it possible to build only one of bitcoin-cli or bitcoin-tx (Luke Dashjr)

Pull request description:

  Includes #5618 (which the reasons for rejecting no longer hold true)

Tree-SHA512: f30a8e4a2f70166b7cabef77c4674163b3a9da14c6a547d34f00d1056a19bf4d23e22851eea726fad2afc8735d5473ae91122c770b65ac3886663dc20e2c5b70
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 17, 2020
Summary:
 * configure: Make it possible to build only one of bitcoin-cli or bitcoin-tx

 * Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled

This is a backport of Core [[bitcoin/bitcoin#12246 | PR12246]]

Test Plan:
Build with autotool and ninja

Ran ninja check-all to make sure that the change in autotool didn't mess up the ninja integration test suite.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5749
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
Summary:
 * configure: Make it possible to build only one of bitcoin-cli or bitcoin-tx

 * Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled

This is a backport of Core [[bitcoin/bitcoin#12246 | PR12246]]

Test Plan:
Build with autotool and ninja

Ran ninja check-all to make sure that the change in autotool didn't mess up the ninja integration test suite.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants