Skip to content

0. Toolchain Setup - #7

Merged
jnewbery merged 3 commits into
bitcoinops:masterfrom
bitschmidty:2019-08-test-notebook-combined
Sep 4, 2019
Merged

jnewbery merged 3 commits into
bitcoinops:masterfrom
bitschmidty:2019-08-test-notebook-combined

Conversation

@bitschmidty

@bitschmidty bitschmidty commented Aug 29, 2019 •

Copy link
Copy Markdown
Contributor

Combined James and Johns PRs to start.

Work in progress, more to come.

  • Fix the check on bitcoind version. Currently fails.

@jachiang

jachiang commented Aug 31, 2019 •

Copy link
Copy Markdown
Contributor

ACK, notebook runs without errors with v0.1. Suggest adopting import util and util.TestWrapper once this is merged.

@jachiang jachiang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Many thanks for merging setup documents and making the on-boarding process much better. I am trying to get resolution on the TestWrapper PR soon, so it can be adopted here as well.

Comment thread 0.1-test-notebook.ipynb Outdated
}
],
"source": [
"import setup"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Update to import util when TestWrapper PR is resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done. I've rebased this PR on the TestWrapper changes.

Comment thread 0.1-test-notebook.ipynb Outdated
}
],
"source": [
"from test_framework.test_wrapper import TestWrapper\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If using util.py, this import can be removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done. I've rebased this PR on the TestWrapper changes.

@jnewbery
jnewbery marked this pull request as ready for review September 2, 2019 18:25
@jnewbery
jnewbery force-pushed the 2019-08-test-notebook-combined branch from 2316e0a to 24446a6 Compare September 2, 2019 18:27
@jnewbery jnewbery mentioned this pull request Sep 2, 2019
@jnewbery jnewbery changed the title WIP: 0. Toolchain Setup 0. Toolchain Setup Sep 2, 2019

@jnewbery jnewbery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've pushed a new version that rebases this on master now that #8 is merged.

There are still a couple of review comments outstanding from #5.

Comment thread README.md
branch, v0.1](https://github.com/bitcoinops/bitcoin/releases/tag/v0.1) which
supports schnorr and taproot.

![workshop_repositories](files/0-repositories-diagram.jpg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comment: #5 (comment)

  • I think the 'imports' arrow is going the wrong way. Either change the text to 'imported by' or reverse the arrow.
  • s/1.0-Workshop-Setup.iypnb/1.0-Workshop-Setup.ipynb/

Comment thread 0.1-test-notebook.ipynb
"* Separate test wrapper `setup`, `shutdown` and your code into different cells. This ensures that an error in your code does not affect the test wrapper object and prevents orphaned subprocesses.\n",
"* Only run one test wrapper object at a time.\n",
"* If the subprocesses do become orphaned somehow, you can run the following:\n",
" * `ps -ax | grep bitcoind` to see outstanding bitcoind processes.\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we also need instructions for windows users here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tracked here: #14

@jnewbery
jnewbery force-pushed the 2019-08-test-notebook-combined branch from 24446a6 to 91e8972 Compare September 4, 2019 13:52
@jnewbery

jnewbery commented Sep 4, 2019

Copy link
Copy Markdown
Contributor

I've rebased on master and pushed a commit with a bunch of minor nits.

I'm going to merge this now. Any further feedback can be addressed in follow-up PRs.

@jnewbery
jnewbery merged commit 64ecb16 into bitcoinops:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants