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

feat: add dockerfile that and script that automates node operation #72

Closed

Conversation

JeancarloBarrios
Copy link
Contributor

@JeancarloBarrios JeancarloBarrios commented Oct 11, 2023

Motivation

Fix Dockerfile to be able to build arm builds

Explanation of Changes

(Write your explanation here)

Testing

(Write your test plan here)

Related PRs and Issues

closes #69

(Link your related PRs and Issues here)

@JeancarloBarrios JeancarloBarrios force-pushed the JeancarloBarrios/fix-docker-build-for-arm-arch branch from d094123 to 3ad4c05 Compare October 11, 2023 14:09
@JeancarloBarrios JeancarloBarrios marked this pull request as ready for review October 11, 2023 14:09
Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It builds! 🎆

However, I tried to run it but seda-chaind seems not to be working.

$ docker run 6432088abd86
exec /usr/bin/seda-chaind: no such file or directory

From inside the container:

397b6b1416c9:~# seda-chaind
bash: /usr/bin/seda-chaind: cannot execute: required file not found

@JeancarloBarrios JeancarloBarrios marked this pull request as draft October 16, 2023 17:01
#!/bin/bash
set -e

# Basic Setup Configurations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule of thumb when there is a doubt, it is better not to hardcode default values.

Some values I might like to have as default are:

  • chain id
  • network

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This are here just for testing purposes final PR will require environmental variables.


# Start the node
echo "Starting node ..."
$BIN start
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this into the dockerfile

Suggested change
$BIN start

COPY scripts/validator_setup/validator_setup.sh /usr/local/bin/validator_setup.sh
RUN chmod +x /usr/local/bin/validator_setup.sh

ENTRYPOINT ["validator_setup.sh"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ENTRYPOINT ["validator_setup.sh"]
ENTRYPOINT ["validator_setup.sh"]
CMD ["seda-chaind", "start"]

@JeancarloBarrios JeancarloBarrios changed the title Jeancarlo barrios/fix docker build for arm arch feat: add dockerfile that and script that automates node operation Oct 17, 2023
Copy link
Member

@mariocao mariocao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works! 👏

Don't forget to squash and do some minor cleaning before merging! 😊

@JeancarloBarrios JeancarloBarrios force-pushed the JeancarloBarrios/fix-docker-build-for-arm-arch branch 2 times, most recently from a3419c0 to 81d6de7 Compare October 18, 2023 13:34
@JeancarloBarrios JeancarloBarrios force-pushed the JeancarloBarrios/fix-docker-build-for-arm-arch branch from 81d6de7 to dcecbe3 Compare October 18, 2023 13:35
@JeancarloBarrios JeancarloBarrios marked this pull request as ready for review October 18, 2023 13:36
Comment on lines +126 to +135
ifeq (badgerdb,$(findstring badgerdb,$(COSMOS_BUILD_OPTIONS)))
BUILD_TAGS += badgerdb
endif
# handle rocksdb
ifeq (rocksdb,$(findstring rocksdb,$(COSMOS_BUILD_OPTIONS)))
ifneq ($(ENABLE_ROCKSDB),true)
$(error Cannot use RocksDB backend unless ENABLE_ROCKSDB=true)
endif
CGO_ENABLED=1
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these DB checks?

@JeancarloBarrios JeancarloBarrios changed the base branch from main to JeancarloBarrios/add-dockerfile-node-automation October 18, 2023 14:47
@JeancarloBarrios JeancarloBarrios changed the base branch from JeancarloBarrios/add-dockerfile-node-automation to main October 18, 2023 14:47
Comment on lines +71 to +115
build_tags = netgo

# experimental feature
ifeq ($(EXPERIMENTAL),true)
build_tags += experimental
endif

ifeq ($(LEDGER_ENABLED),true)
ifeq ($(OS),Windows_NT)
GCCEXE = $(shell where gcc.exe 2> NUL)
ifeq ($(GCCEXE),)
$(error gcc.exe not installed for ledger support, please install or set LEDGER_ENABLED=false)
else
build_tags += ledger
endif
else
UNAME_S = $(shell uname -s)
ifeq ($(UNAME_S),OpenBSD)
$(warning OpenBSD detected, disabling ledger support (https://github.com/cosmos/cosmos-sdk/issues/1988))
else
GCC = $(shell command -v gcc 2> /dev/null)
ifeq ($(GCC),)
$(error gcc not installed for ledger support, please install or set LEDGER_ENABLED=false)
else
build_tags += ledger
endif
endif
endif
endif

ifeq (secp,$(findstring secp,$(COSMOS_BUILD_OPTIONS)))
build_tags += libsecp256k1_sdk
endif

whitespace :=
whitespace += $(whitespace)
comma := ,
build_tags_comma_sep := $(subst $(whitespace),$(comma),$(build_tags))

ldflags = -X github.com/cosmos/cosmos-sdk/version.Name=seda-chain \
-X github.com/cosmos/cosmos-sdk/version.AppName=seda-chaind \
-X github.com/cosmos/cosmos-sdk/version.Version=$(VERSION) \
-X github.com/cosmos/cosmos-sdk/version.Commit=$(COMMIT) \
-X "github.com/cosmos/cosmos-sdk/version.BuildTags=$(build_tags_comma_sep)" \
-X github.com/tendermint/tendermint/version.TMCoreSemVer=$(TMVERSION)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have this already, no?

Comment on lines +168 to +177
build: BUILD_ARGS=-o $(BUILDDIR)/
build-linux:
@if [ "${ENABLE_ROCKSDB}" != "true" ]; then \
echo "RocksDB support is disabled; to build and test with RocksDB support, set ENABLE_ROCKSDB=true"; fi
GOOS=linux GOARCH=$(if $(findstring aarch64,$(shell uname -m)) || $(findstring arm64,$(shell uname -m)),arm64,amd64) LEDGER_ENABLED=false $(MAKE) build

$(BUILD_TARGETS): go.sum $(BUILDDIR)/
@if [ "${ENABLE_ROCKSDB}" != "true" ]; then \
echo "RocksDB support is disabled; to build and test with RocksDB support, set ENABLE_ROCKSDB=true"; fi
go $@ -mod=readonly $(BUILD_FLAGS) $(BUILD_ARGS) ./...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better if build-linux gives us seda-chaind-linux to be distinguished from the output of build?

@mariocao
Copy link
Member

replaced by #77

@mariocao mariocao closed this Oct 18, 2023
@hacheigriega hacheigriega deleted the JeancarloBarrios/fix-docker-build-for-arm-arch branch May 2, 2024 16:05
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.

Docker build fails on ARM architecture 🐛
3 participants