Conversation
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on masihyeganeh, metalarm10, and ysv).
masihyeganeh
left a comment
There was a problem hiding this comment.
@masihyeganeh reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on metalarm10, TxCorpi0x, and ysv).
build/tx-chain/build.go line 217 at r1 (raw file):
// targetPlatform is used for all Path() lookups so local and in-Docker builds use the same toolchain. func linuxMuslToolchainFor(targetPlatform txcrusttools.TargetPlatform) (linuxMuslToolchain, error) { switch targetPlatform {
This is already checked:
targetPlatform.OS == txcrusttools.OSLinux
So, I think this part is not needed.
build/tx-chain/build.go line 229 at r1 (raw file):
switch targetPlatform.Arch { case txcrusttools.ArchAMD64: gccBin, wasmLib, wasmSubdir = "bin/x86_64-linux-musl-gcc", "lib/libwasmvm_muslc.x86_64.a", "/x86_64-linux-musl/lib/libwasmvm_muslc.x86_64.a"
Again, everything except x86_64 and aarch64 are duplicated. We can simply get the architecture in a variable and format those paths
TxCorpi0x
left a comment
There was a problem hiding this comment.
@TxCorpi0x made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on masihyeganeh, metalarm10, and ysv).
build/tx-chain/build.go line 217 at r1 (raw file):
Previously, masihyeganeh (Masih Yeganeh) wrote…
This is already checked:
targetPlatform.OS == txcrusttools.OSLinuxSo, I think this part is not needed.
Done.
build/tx-chain/build.go line 229 at r1 (raw file):
Previously, masihyeganeh (Masih Yeganeh) wrote…
Again, everything except
x86_64andaarch64are duplicated. We can simply get the architecture in a variable and format those paths
Done.
masihyeganeh
left a comment
There was a problem hiding this comment.
@masihyeganeh reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on metalarm10 and ysv).
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on metalarm10 and TxCorpi0x).
build/tx-chain/build.go line 264 at r2 (raw file):
case txcrusttools.OSLinux: // Linux builds must use muslc + static linking so the binary runs in Alpine (txd Docker image). // Using glibc or dynamic wasmvm leads to SIGABRT in wasmvm/cgo when the binary runs in-container.
But SIGABRT only happens under specific circumstances right ?
Like high-load etc ?
Because it is a bit strange compilation bug fails only under high-load
TxCorpi0x
left a comment
There was a problem hiding this comment.
@TxCorpi0x made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on metalarm10 and ysv).
build/tx-chain/build.go line 264 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
But SIGABRT only happens under specific circumstances right ?
Like high-load etc ?Because it is a bit strange compilation bug fails only under high-load
The issue comes up in CI since when we usle local built binary instead of docker-built binary. this fix the issue related to txd binary which will be transferred to the container which is built using muslc.
ysv
left a comment
There was a problem hiding this comment.
@ysv resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on metalarm10).
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on metalarm10).
Description
This pull request refactors and simplifies the logic for setting up the musl toolchain when building for Linux platforms in
build/tx-chain/build.go. The most important change is the introduction of a new helper struct and function to encapsulate the configuration for musl-based toolchains, reducing code duplication and improving maintainability. The main build logic is updated to use this new abstraction.Refactoring and toolchain configuration:
linuxMuslToolchainstruct and thelinuxMuslToolchainForfunction to encapsulate all musl toolchain paths and configuration for different Linux architectures, replacing duplicated logic and hardcoded values.buildTXdInDockerfunction to use the newlinuxMuslToolchainabstraction, greatly simplifying the logic for setting up compiler and library paths, Docker volumes, and environment variables for musl-based static builds.Build logic improvements:
FIXES
Incompatible build wasm causes SIGABORT panics in
wasmvmwhen the blockchain node is built without the needed muslc flags and properties which is needed in the Alpine blockchain node.Reviewers checklist:
Authors checklist
This change is