Clarify logic around dev builds and rebuilding #72
Conversation
Tristan-Wilson
commented
Aug 7, 2024
•
Tristan-Wilson
commented
test-node.bash
Outdated
| @@ -177,7 +189,7 @@ while [[ $# -gt 0 ]]; do | |||
| echo $0 script [SCRIPT-ARGS] | |||
| echo | |||
| echo OPTIONS: | |||
| echo --build rebuild docker images | |||
| echo --build rebuild docker images. --build false disables rebuild | |||
There was a problem hiding this comment.
change that to --no-build
not necessarily better, but that's how we currently do all boolean options
There was a problem hiding this comment.
(other option is to change no-tokenbridge, no-simple etc..)
There was a problem hiding this comment.
--no-build is cleaner, I'll change it to that.
test-node.bash
Outdated
| @@ -200,17 +212,17 @@ while [[ $# -gt 0 ]]; do | |||
| esac | |||
| done | |||
|
|
|||
| if $force_init; then | |||
| if ! $skip_build && $force_init; then | |||
There was a problem hiding this comment.
It seems there's currently a lot of disarray in force_build, dev_build_X.. in that way skip_build is reasonable but it adds to the mess instead of removing from it.
I can merge this as I don't have a clear better way - but see if you can come up with something.
I would like the solution to parse things in order, so e.g.:
--init --no-build > will not build, but
--init --no-build --build > will build
(it's a good property for scripts run by other scripts, so you can always change behaviour by adding an argument at the end)
I think some of the mess comes from the duality of build being used for building contracts/scripts vs used to build local nitro.. maybe there should be different args for that.
A final note: external users don't generally use --dev so I don't mind introducing changes that will break usage that does build local nitro.
There was a problem hiding this comment.
I think I've come up with something that clarifies it, PTAL.
The concepts of using the dev builds for nitro and blockscout and rebuilding are now split into different variables internally to the test-node.bash script and can be controlled independently. The --dev flag by itself means to use the dev builds and to rebuild them. --no-build-dev-nitro and -no-build-dev-blockscout disable rebuilding the respective dev images. --no-build disables rebuilding the dev images and also rebuilding contracts related images, and the node images. This finer grained control saves time in the development cycle by allowing the user to rebuild only what is needed. An example command line to reinitialize the blockchain, use the dev images, but not rebuild anything except blockscout would be: ./test-node.bash --init --dev --no-build --build-dev-blockscout --blockscout Flags are read from left to right; flags to the right cancel the effects of flags to the left.
Tristan-Wilson
changed the title
test-node.bash
Outdated
| build_dev_blockscout=false | ||
| shift | ||
| ;; | ||
| --build-contracts) |
There was a problem hiding this comment.
I like the solution.
Small comment:
This isn't just contracts, but also scripts and tokenbridge. Can we rename it? build-iternal? build-utils? something similar..
Complicating things:
contracts and tokenbridge are weird dockers. They both fetch external code and build it according to NITRO_CONTRACTS_BRANCH, TOKEN_BRIDGE_BRANCH env variables. It means that it's very possible that rebuild is necessary even though docker doesn't recognize it. So we need an option to force rebuilding them. (docker build --force? I'm not sure how to do it)
I think a manual --build-contracts/internal/other should force rebuilding, but --init and --init-force should just do a normal build.
So if someone wants to force rebuild they'll do --init --build-contracts
There was a problem hiding this comment.
the option name could have force inside, like --force-build-utils or something
There was a problem hiding this comment.
Took your suggestions. The way to force rebuild with docker is --no-cache
| docker pull $BLOCKSCOUT_VERSION | ||
| docker tag $BLOCKSCOUT_VERSION blockscout-testnode | ||
| fi | ||
| fi | ||
|
|
||
| if $force_build; then | ||
| if $build_node_images; then |
There was a problem hiding this comment.
I'm not sure if we need this as a separate step?
isn't everything this can do covered by previous builds?
There was a problem hiding this comment.
I think this is the only place we're building $NODES that I could find in the code.
