Conversation
| "\n", | ||
| "* [Bitcoin Core build notes](https://github.com/bitcoin/bitcoin/tree/master/doc#building)" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Everything above here should be in README.md (no point in telling people how to install python/jupyter and run a notebook when they're already running a notebook).
There was a problem hiding this comment.
I was assuming a starting point with github preview, which renders the notebook as run with a notebook server. Agree to move it to readme.
There was a problem hiding this comment.
Overlapping instructions between this, testnotebook and README.md are being consolidated and streamlined by Schmidty here.
| "\n", | ||
| "**Overview of required repositories**\n", | ||
| "\n", | ||
| "" |
There was a problem hiding this 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/
There was a problem hiding this comment.
Corrected. @bitschmidty, do you want to use this for the consolidated setup guide?
There was a problem hiding this comment.
It doesn't look like this has been committed to the branch
| "\n", | ||
| "It should look something like this:\n", | ||
| "\n", | ||
| "* `/.../bitcoin/test/functional`" |
There was a problem hiding this comment.
What does /... do? I think it'd be better to make this a full absolute path eg /home/optech/bitcoin or whatever.
There was a problem hiding this comment.
This is no longer necessary with setup.py
| "import sys\n", | ||
| "\n", | ||
| "# Make sure your insert your full path.\n", | ||
| "sys.path.insert(0, '/.../bitcoinops/bitcoin/test/functional') \n" |
There was a problem hiding this comment.
take a look at the setup.py file I added in #4. I think that's the way to go since users can add the path once and it'll work for all notebooks.
There was a problem hiding this comment.
This has now been incorporated in 0.1 setup chapter and 1.1 Schnorr-Intro.
| "\n", | ||
| "**Things to keep in mind:**\n", | ||
| "* 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", |
There was a problem hiding this comment.
We should use a singleton pattern on the TestWrapper to ensure that only one can be instantiated. We should also check whether there are already bitcoind processes running in the setup method (see https://github.com/bitcoinops/bitcoin/blob/19c8b5671d1f654109ac7c1baea8cb9555dae0e2/test/functional/test_runner.py#L330)
There was a problem hiding this comment.
This is being addressed in TestWrapper here, which I propose to combine with setup.py into a util.py file.
| "**Things to keep in mind:**\n", | ||
| "* 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", |
There was a problem hiding this comment.
These instructions should work for linux/mac. Do we also need instructions for windows?
There was a problem hiding this comment.
Yeah I think that would be good. I have very little experience with Windows however. @bitschmidty, do you have some bitcoin/python setup experience with windows machines?
There was a problem hiding this comment.
@jachiang Nope, but I can fire up a VM and test!
I prefer to do that a bit later when the ground isnt shifting below.
|
Superseded by #8 |
Has some overlap with readme.