-
Notifications
You must be signed in to change notification settings - Fork 93
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
fix(release): fix the Dockerfile #211
Conversation
This commit fixes the broken Dockerfile that is used in release CI. It adds wasmedge installation step in the build, uses `xx-cargo` instead of cargo, and copies the `sed` command from Docker's build script that allows xx-cargo to be used with pkg-config in build scripts. Signed-off-by: jiaxiao zhou <[email protected]>
Dockerfile
Outdated
@@ -25,18 +30,25 @@ COPY --link Cargo.toml ./ | |||
COPY --link Cargo.lock ./ | |||
ARG CRATE="" | |||
ARG TARGETOS TARGETARCH TARGETVARIANT | |||
RUN curl -sSf https://raw.githubusercontent.com/WasmEdge/WasmEdge/master/utils/install.sh | bash -s -- \ |
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.
We don't need to install WasmEdge manually anymore. That's take care automatically during the build process.
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.
done.
Dockerfile
Outdated
RUN --mount=type=cache,target=/usr/local/cargo/git/db \ | ||
--mount=type=cache,target=/usr/local/cargo/registry/cache \ | ||
--mount=type=cache,target=/usr/local/cargo/registry/index \ | ||
--mount=type=cache,target=/build/src/target,id=runwasi-cargo-build-cache-${TARGETOS}-${TARGETARCH}${TARGETVARIANT} <<EOT | ||
set -e | ||
export "CARGO_NET_GIT_FETCH_WITH_CLI=true" | ||
export RUSTFLAGS='-Clink-arg=-Wl,-rpath,$ORIGIN' |
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.
We are by default statically linking against wasmedge now. You can remove this.
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.
done.
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 working on this!
Just a few comments.
Looks like just one comment from @jprendes but otherwise LGTM. |
Signed-off-by: jiaxiao zhou <[email protected]>
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.
Dockerfile
Outdated
|
||
FROM base AS build | ||
SHELL ["/bin/bash", "-c"] | ||
ARG BUILD_TAGS TARGETPLATFORM | ||
RUN xx-apt-get install -y gcc g++ libc++6-dev zlib1g | ||
RUN xx-apt-get install -y pkg-config libsystemd-dev libdbus-glib-1-dev build-essential libelf-dev libseccomp-dev libclang-dev | ||
RUN xx-apt-get install -y libsystemd-dev libdbus-1-dev libseccomp-dev | ||
RUN rustup target add $(xx-info march)-unknown-$(xx-info os)-$(xx-info libc) |
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.
nit: Remove. xx-cargo
runs rustup target add
(for target platform) for you.
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.
Done.
@@ -36,7 +41,7 @@ RUN --mount=type=cache,target=/usr/local/cargo/git/db \ | |||
if [ -n "${CRATE}" ]; then | |||
package="--package=${CRATE}" | |||
fi | |||
cargo build --release --target=$(xx-info march)-unknown-$(xx-info os)-$(xx-info libc) ${package} | |||
xx-cargo build --release ${package} |
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.
nit: Remove lines 39 and 40. xx-cargo
already sets those variables for you.
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.
Done,
…bring Signed-off-by: jiaxiao zhou <[email protected]>
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.
LGTM.
Thanks @Mossaka !
This commit fixes the broken Dockerfile that is used in release CI. It adds wasmedge installation step in the build, uses
xx-cargo
instead of cargo, and copies thesed
command from Docker's build script that allows xx-cargo to be used with pkg-config in build scripts.Reference
FYI @cpuguy83 @jprendes