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.
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
Add Makefile for automating localnet setup #3718
Add Makefile for automating localnet setup #3718
Changes from 11 commits
e5449c2
650c589
e3a3fb5
97dd342
599c9fb
4417e33
548fa41
c347d9c
5fb4b21
eb2d6b3
1453025
234c228
ed40afb
7d16890
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Makefile didn't specify the SHELL variable so mine defaulted to /bin/sh which doesn't support the array syntax here. Not sure if there is a shell agnostic way to do it, but adding SHELL=/bin/bash to the Makefile fixed it locally.
The iterative single-node behavior also doesn't seem to work quite right. I checked out an older version of the code, killed bob, ran
make bob
and it just restarted without a build.I also ran
make desktop/build
and no changes, but./gradlew :desktop:build
had something to do.Even making alice & bob have a dependency on
desktop/build
instead of justsetup
didn't work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks @julianknutsen. Commit 234c228 removes the offending bashism such that
/bin/sh
should run without error.You're right, and I should have mentioned this in #3718 (comment). The rationale for why things need to be this way is laid out in 5fb4b21. Search for the first occurrence of the word 'contention' there. I'm updating the comment above now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the commit link and it makes sense w.r.t. contention. Verified the bashism issue is fixed as well and will do 1.2.4 testing today with your latest code and call out any other glaring issues. So expect an ACK by EOD from me if everything works out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbeams thanks for your instructive commit messages!
I understand your reasoning behind removing
build
from PHONY. I'm just wondering wether there is a better solution. Doing a clean-rebuild takes a very long time and is not practical for quick iteration cycles. Essentially the problem you have solved is a race condition. Couldn't we ensure somehow that the build runs just once before deploying the individual nodes. Perhaps running the individual node commands shouldn't depend on the setup/build commands. That way we could have both a 1 time setup + iteration.Is there a significant advantage of having all the individual node commands depend on setup/build that I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I'm doing in practice as I develop stuff. The following assumes I have already done a
make deploy
, and that now I'm iterating on something, using myalice
desktop node to see my changes live:This picks up and rebuilds just the changes I've made and then deploys those new bits as
alice
.It does effectively run just once. The individual node deployment targets, e.g.
alice
andbob
depend onbuild
. Note that build is, in the latest commits, actually included once again in PHONY, so it does run every time, but that target just depends on the more specificdesktop/build
andseednode/build
targets, which in turn run only if their respective directories do not already exist. The effect here is that when we deploy more than one node or all of them viamake deploy
, the{desktop|seednode}/build
targets get run once and only once, avoiding the above mentioned race condition and inefficient contention for Gradle resources. If you want to cause a fast (incremental) rebuild, it's simply necessary to drop down to calling gradle directly like I've shown above. callingmake clean-build
should not be necessary in any case, unless you actually want to blow away all the build directories.It just ensures that deploying any given node causes
build
andlocalnet
to run if they have not already done so. So someone can come along in a clean checkout and run onlymake bitcoind
andmake seednode
and everything will work as they expect, meaning that the .localnet dir will get created and the seednode build will run. If that's already happened, then those targets are no-ops.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could use another sentence or so in the preamble. You end up needing to create blocks to test features like governance so helping new users fix common errors like "I created a proposal from Alice, but it isn't visible on Bob. Why not?" may help the onboarding.