-
Notifications
You must be signed in to change notification settings - Fork 50
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 support for wormchain and update the docker files to better support a custom build dir #134
Conversation
a custom build dir
dockerfile/cosmos/Dockerfile
Outdated
if [ ! -z "$BUILD_TARGET" ]; then\ | ||
if [ ! -z "$BUILD_DIR" ]; then cd "${BUILD_DIR}"; fi;\ | ||
fi;\ |
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.
Is there any chance this would be backwards breaking?
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.
It shouldn't. If a build directory is defined, then we need to enter it to use the go list to find a wasm version in the next step. The current and only cosmos repo (gravitybridge) with a build dir set, does not use wasm so it wasn't an issue.
dockerfile/cosmos/local.Dockerfile
Outdated
|
||
WORKDIR /go/src/${REPO_HOST}/${GITHUB_ORGANIZATION}/${GITHUB_REPO} | ||
|
||
ADD . . |
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.
Moving the ADD
up here removes all of the caching optimizations that were done for local builds in #107 . Is there a reason it's necessary?
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 optimization did not account for a build dir. When a build dir is defined, we would need to pull the go.mod and go.sum from that build directory for the wasmvm search, not from the root. If other modules from that repo are also used, i.e. wormchain's sdk module, the go list
will also fail which is a second reason the ADD was moved up. Perhaps we need a new way to find the wasmvm version to keep the optimization? As it stands, the build will fail when using build dir with a chain that supports wasmvm and/or other modules in that repo.
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.
What if we just move up the ARG BUILD_DIR
so that it is available when adding the go.mod and go.sum, and then ADD ${BUILD_DIR}/go.mod ${BUILD_DIR}/go.sum ./
?
Alternatively, what if we move the wasm version detection into the heighliner go code and add ARG WASM_VERSION
to the dockerfiles? Heighliner already parses the go.mod to determine the go version. That would probably help caching the most so that libwasm wouldn't need to be re-downloaded when changing the go.mod, only when the wasm version changed.
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.
It seems like the only thing gained by doing the cd $BUILD_DIR
earlier is that the directory is set for the PRE_BUILD
step. The pre-build
step does not assume that it's within the build-dir
, and a cd
can be added if necessary to the pre-build
script.
Changing to the build dir earlier is necessary for chains that use wasmvm so the |
|
||
WORKDIR /go/src/${REPO_HOST}/${GITHUB_ORGANIZATION}/${GITHUB_REPO} | ||
|
||
# Download dependencies and CosmWasm libwasmvm if found. | ||
ADD go.mod go.sum ./ |
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.
why was this and L22 removed? We should be able to cache deps still, but it could be done as a phase after the libwasmvm download since that no longer depends on the go.mod being present.
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.
Pulling those files does not work when there is a custom build directory
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.
One question, otherwise LGTM!
custom build directory
Updated to pull the wasmvm version in heighliner code instead of in dockerfile which speeds things up.