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 support for wormchain and update the docker files to better support a custom build dir #134

Merged
merged 7 commits into from
Aug 24, 2023

Conversation

misko9
Copy link
Contributor

@misko9 misko9 commented Jun 13, 2023

Updated to pull the wasmvm version in heighliner code instead of in dockerfile which speeds things up.

@misko9 misko9 requested a review from agouin June 13, 2023 01:25
Comment on lines 51 to 53
if [ ! -z "$BUILD_TARGET" ]; then\
if [ ! -z "$BUILD_DIR" ]; then cd "${BUILD_DIR}"; fi;\
fi;\
Copy link
Contributor

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?

Copy link
Contributor Author

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.


WORKDIR /go/src/${REPO_HOST}/${GITHUB_ORGANIZATION}/${GITHUB_REPO}

ADD . .
Copy link
Member

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?

Copy link
Contributor Author

@misko9 misko9 Jun 25, 2023

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.

Copy link
Member

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.

Copy link
Member

@agouin agouin left a 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.

@misko9
Copy link
Contributor Author

misko9 commented Jun 25, 2023

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 go list will work correctly.


WORKDIR /go/src/${REPO_HOST}/${GITHUB_ORGANIZATION}/${GITHUB_REPO}

# Download dependencies and CosmWasm libwasmvm if found.
ADD go.mod go.sum ./
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@agouin agouin left a 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!

@misko9 misko9 merged commit 212c0c9 into main Aug 24, 2023
2 checks passed
@misko9 misko9 deleted the steve/wormchain branch August 24, 2023 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants