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

Add Makefile for automating localnet setup #3718

Merged
merged 14 commits into from
Dec 6, 2019

Commits on Nov 30, 2019

  1. Add Makefile for automating localnet setup

    Problem: contributors old and new must read and follow many manual steps
    spread across three documents (docs/{build,dev-setup,dao-setup}.md) in
    order to get up and running with a local regtest Bisq network deployment
    suitable for isolated development and end-to-end testing. This process
    is not only manual, but requires considerable trial and error for most
    contributors, and can amount to hours of effort. Perhaps most
    detrimental is that this friction makes it much less likely that we get
    "all hands on deck" to cover test scenarios at release time. Getting up
    and running with what this change refers to as a "localnet" should be
    among the very first things a new contributor does. It should be fast
    and easy, maximizing the contributor's ability to get productive right
    away.
    
    Solution: this commit introduces a simple and well-documented makefile
    to the root of the source tree. It instructs the user to issue a series
    of simple `make` commands, at the end of which they'll have a fully
    functional localnet deployment.
    
    Caveats:
    
     - No support for Windows unless the user is running Git Bash, Cygwin or
       similar. In any case, the makefile serves as clear documentation
       about what a Windows user would need to do manually, i.e. without the
       benefit of `make` automating it all.
    
     - The aforementioned setup documents should be updated to point to this
       makefile instead of explaining everything in prose. The dev-setup.md
       and dao-setup.md documents may actually be candidates for deletion if
       this new approach proves successful.
    
     - These changes do not include passing the new -peerbloomfilters=1
       option to bitcoin versions 0.19 and above. Those who have already
       upgraded should take care to add that option.
    
    Notes:
    
     - The introduction of this makefile has no impact on Bisq's use of
       Gradle as a build system. Everything there is as it has been. This
       makefile is a completely optional convenience being added into the
       mix. It has the added benefit of being a "friendly face" to those not
       familiar with the Java / JVM ecosystem. Developers from many
       different backgrounds are familiar with make and makefiles, and they
       may find this one a pleasant and inviting surprise.
    cbeams committed Nov 30, 2019
    Configuration menu
    Copy the full SHA
    e5449c2 View commit details
    Browse the repository at this point in the history
  2. Use STATE_DIR := .localnet in makefile

    Justin Carter committed Nov 30, 2019
    Configuration menu
    Copy the full SHA
    650c589 View commit details
    Browse the repository at this point in the history
  3. Make make localnet command idempotent

    Sometimes when running setup something goes wrong and the ./dao-state
    dir is still hanging around, requiring manual cleanup nad preventing from simply
    re-running the command.
    Justin Carter committed Nov 30, 2019
    Configuration menu
    Copy the full SHA
    e3a3fb5 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    97dd342 View commit details
    Browse the repository at this point in the history

Commits on Dec 2, 2019

  1. Merge pull request #3 from bodymindarts/makefile-improvements

    Makefile improvements
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    599c9fb View commit details
    Browse the repository at this point in the history
  2. Partially revert "Make make localnet command idempotent"

    This partially reverts commit e3a3fb5, removing the dependency from
    the 'localnet' target to the 'clean-localnet' target. The reason for
    this is that a number of higher level targets that deploy nodes, e.g.
    the 'alice' and 'bob' targets depend on 'localnet' and, prior to this
    reversion, therefore also depended on 'clean-localnet'. The effect was
    that every time a node is deployed, the .localnet directory was removed
    and re-created, destroying the state of any and all nodes that had been
    deployed and modified thus far.
    
    The change in the original commit that removes the temporary 'dao-setup'
    directory in case of partial failures has been preserved.
    
    This is a follow-up to #3.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    4417e33 View commit details
    Browse the repository at this point in the history
  3. Review "Use STATE_DIR := .localnet in makefile"

    This change follows up on commit 650c589, which:
    
      1. Renamed the 'localdir' directory to '.localdir' to better follow
      convention with how local data directories are often managed, e.g.
      .git and .gradle.
    
      2. Introduced the STATE_DIR variable to avoid duplication of the
      '.localdir' string throughout the Makefile, and at least in concept to
      allow this value to be customized via setting an environment variable.
    
    The changes in (1) are preserved, while the changes in (2) have been
    backed out. Rationale:
    
     - The STATE_DIR name introduces a new concept to the reader. They must
       reason about its meaning, and this works against the intention of the
       Makefile, which is to maximize understandability for the uninitiated.
    
     - The name, if we were to preserve the variable, probably should have
       been something like DATA_DIR_ROOT. 'STATE_DIR' is not conceptually
       incorrect, but industry convention is to refer to such directories as
       "data directories", e.g. Bitcoin Core's `datadir` option, LND's
       `datadir` option and Bisq's `userDataDir` and `appDataDir` options.
    
     - The variable, whatever its name, introduces a layer of indirection,
       which while convenient to the makefile maintainer, is a barrier to
       comprehension for the reader / contributor. For example, if a user
       wished to copy and paste the recipe for a target, say 'bob' from the
       makefile, with the varible in place, the user would have to figure
       out its correct value and replace it before they could paste and use
       the copied command. Like in the first note above, the idea with the
       makefile is to maximize understanding for the uninitiated, i.e.
       working code as executable documentation. It is reasonable given this
       goal to increase the burden on a few maintainers in order to ease the
       potentially many contributors.
    
    Finally, this change follows up on the renaming of the 'localnet'
    directory to '.localnet' by reflecting this change in the name of the
    associated target as well. This is order to avoid dependent targets e.g.
    'bitcoind', 'alice' or 'bob' constantly re-running the localnet target.
    In turn it also adds an 'alias' target named 'localnet' (without the
    leading dot) because targets with a leading dot are (I believe) treated
    as "implicit targets". In any case, they do not show up in a tab
    completion context, so introducing the normally-named alias fixes that.
    
    This is a follow-up to #3.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    548fa41 View commit details
    Browse the repository at this point in the history
  4. Update .editorconfig to use hard tabs in Makefile

    Problem: we use soft 4-space tabs throughout the Bisq codebase, and the
    new makefile is a break to this rule due to make's default requirement
    for hard tabs in recipes.
    
    Solution: This commit updates our Editorconfig settings to reflect this
    exception.
    
    For vim users, it is also recommended that you add the following entry
    to your .vimrc:
    
         au FileType make set tw=72 noet cc=72
    
    It will ensure that you wrap (documentation) lines at 72 chars. It also
    sets noexpandtab explicitly. Even though .editorconfig should already be
    doing this for you when working in Bisq, this more general vim
    configuration will ensure you use tabs correctly in any makefile. The
    `cc=72` setting adds a visual right margin at 72 characters.
    
    This commit also updates the existing makefile, wrapping lines of
    documentation that had exceeded the 72-char margin.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    c347d9c View commit details
    Browse the repository at this point in the history
  5. Refine deploy target for better use of screen

    Problem: Prior to this change, it was necessary to first create and
    attach to a screen session and then to run `make deploy` within it. This
    meant extra steps for the user and was generally error-prone.
    
    Solution: Usage of screen has been refined such that a screen session
    named 'localnet' is created on the users behalf without any need to
    attach to it. Individual node deployment targets such as `make
    bitcoind`, `make alice`, et al. are issued to new windows within the
    localnet screen session, and the user is free to attach or not whenever
    they choose. The result is that a new user can clone the repository and
    type nothing more than `make deploy` to get up and running with their
    localnet.
    
    This also reverts the changes in commit 97dd342 ("Make build target
    phony") for the following reasons:
    
     - As mentioned in that commit message, Gradle was not deleting the its
       'build' directory when running `gradle clean`, meaning that the
       'build' target was always up-to-date, even after running `make
       clean`. This made it impossible to get a correct rebuild workflow. On
       analysis, howewer, this situation was because of a badly behaving
       Kotlin plugin not cleaning up after itself, leaving a subdirectory at
       build/kotlin and preventing the build directory itself from being
       deleted altogether. To address this, the `make clean` target has been
       updated to `rm -rf build` instead of calling `build gradle`. While
       it's a workaround until we back out the Kotlin changes that caused
       this, it does have the added benefit of being faster than invoking
       `gradle clean`.
    
     - By making the 'build' target PHONY, this meant that `./gradlew build`
       was getting invoked every time a dependent target was called. For
       example, `make alice` depends on the 'setup' target, which in turn
       depends on the 'build' target. When calling such targets in
       isolation, this arrangement works out fine, because the phony 'build'
       target always runs, invoking `./gradle build`, and the Gradle build
       completes quickly assuming everything is up-to-date. The problem
       arises when calling a number of these targets in rapid succession, as
       we do when calling `make deploy` and running each individual node
       target in its own screen window. This causes contention in two ways.
       The first is that these multiple, simultaneous Gradle processes
       compete for access to an available Gradle daemon, and because each
       process needs its own, it ends up that as many Gradle daemons get
       created as Bisq nodes we need to deploy (5 in total). This is a big
       waste of time and resources. The second way it causes not only
       contention but outright failure is that each of these builds are
       operating in the same directory, and while most aspects of the build
       are in fact up-to-date and therefore not modified in any way, there
       are exceptions to this rule. The result is that build artifacts, e.g.
       jars are getting deleted and rebuilt from underneath competing Gradle
       processes, and all manner of chaos ensues, such as NoClassDefFound
       errors and much more. This change (reverting 'build' back to a
       normal, non-phony target) avoids these problems entirely. When
       running `make deploy`, we run the 'build' target once as a function
       of the 'deploy' target depending on it. At this point, the 'build'
       directory exists, and all subsequent node deployment targets, e.g.
       'alice', 'bob', etc do not re-run the build target because it is
       up-to-date. For workflows where the user definitely wants to rebuild
       prior to redeploying a given node, they can either run `make
       clean-build`, or drop down to issuing Gradle build commands directly,
       e.g. `./gradlew :desktop:build` followed by `make desktop`.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    5fb4b21 View commit details
    Browse the repository at this point in the history
  6. Enable 'peerbloomfilters' option on localnet bitcoind

    Problem: Bitcoind Core v0.90.0 changed the default value of its
    'peerbloomfilters' option from 1 to 0, now disabling them by default.
    Bisq requires bloom filters be enabled on the Bitcoin node(s) it
    communicates with, so users who are running >= v0.90 would get errors
    when attempting to run `make bitcoind` with that target's current
    recipe.
    
    Solution: This change explicitly sets the 'peerbloomfilters' option to
    1, ensuring it is enabled in any case. Note that this option has existed
    in Bitcoin Core since v0.12.0, so there is no real concern for this new
    option breaking users that are still on 0.18.x or even much earlier.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    eb2d6b3 View commit details
    Browse the repository at this point in the history
  7. Make 'build' target phony once again

    In commit 5fb4b21 ("Refine deploy target..."), the 'build' target was
    made normal, i.e. non-phony, but on further review it does in fact make
    sense to declare 'build' phony, such that it is run no matter the status
    of the root-level 'build' directory, but for different reasons.
    
    Previously, we had been considering the presence of 'build' directory as
    a reasonable proxy for determining whether the `./gradlew build` had
    been run. If the directory was present, we considered the 'build' target
    up-to-date. If not, then we would re-run `./gradlew build`. This is all
    sensible enough, except for the fact that the root-level 'build'
    directory has almost nothing to do with the actual output of `./gradlew
    build`. Gradle does output 'build' directories, but in the respective
    subdirectory for each module of the project. After `./gradlew build` has
    been run, we would see a 'desktop/build' directory, a 'seednode/build'
    directory and so forth. It just so happens that a root-level 'build'
    directory was getting created at all due to idiosyncracies of a
    particular Kotlin plugin.
    
    This commit updates the makefile to better respect this reality by:
    
     - preserving the 'build' target but marking it once again as PHONY
    
     - introducing new 'seednode/build' and 'desktop/build' targets that
       trigger './gradlew :seednode:build` and ./gradlew :desktop:build`
       commands respectively.
    
     - making 'build' depend on these two new targets
    
    In light of this realization of flawed thinking about the root-level
    build dir, this change also restores `make clean` to calling `./gradlew
    clean` instead of `rm -rf build`.
    cbeams committed Dec 2, 2019
    Configuration menu
    Copy the full SHA
    1453025 View commit details
    Browse the repository at this point in the history

Commits on Dec 3, 2019

  1. Avoid bash-specific syntax in makefile

    This fixes the problem described at [1] by replacing bash-specific array
    syntax with a simpler sh-friendly for loop.
    
    [1]: bisq-network#3718 (review)
    cbeams committed Dec 3, 2019
    Configuration menu
    Copy the full SHA
    234c228 View commit details
    Browse the repository at this point in the history
  2. Add 'make undeploy' target to kill all running nodes

    Problem: previously, in order to completely shut down a running
    localnet, users had to attach to their 'localnet' screen and kill (^C)
    each process, then quit and kill the entire screen session.
    
    Solution: this change introduces an 'undeploy' target that automates
    sending the ^C to each screen window followed by sending screen's 'kill'
    command to any remaining windows, thus killing the entire 'localnet'
    screen session.
    
    The result is that users may now run the following two commands in
    succession any number of times to bring their localnet up and down (to
    'deploy' and 'undeploy' their localnet).
    
        # bring up localnet
        $ make deploy
    
        # use localnet to test, develop, etc...
    
        # bring down localnet
        $ make undeploy
    cbeams committed Dec 3, 2019
    Configuration menu
    Copy the full SHA
    ed40afb View commit details
    Browse the repository at this point in the history

Commits on Dec 5, 2019

  1. Update docs/README.md with link to new Makefile

    The old dev-setup.md and dao-setup.md docs have been marked as
    deprecated for now and may be removed after we've gotten sufficient
    feedback on the Makefile-based approach.
    cbeams committed Dec 5, 2019
    Configuration menu
    Copy the full SHA
    7d16890 View commit details
    Browse the repository at this point in the history