fix(Makefile)!: add ledger build flag to build with ledger support by default #2014
Conversation
| @@ -14,7 +47,7 @@ ldflags = -X github.com/cosmos/cosmos-sdk/version.Name=celestia-app \ | |||
| -X "github.com/cosmos/cosmos-sdk/version.BuildTags=$(build_tags_comma_sep)" | |||
There was a problem hiding this comment.
[note for reviewers] this var (build_tags_comma_sep) was never set but was still used here.
Makefile
Outdated
| LEDGER_ENABLED ?= true | ||
|
|
||
|
|
||
| # process build tags | ||
| build_tags = | ||
| 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 | ||
|
|
||
| whitespace := | ||
| whitespace := $(whitespace) $(whitespace) | ||
| comma := , | ||
| build_tags_comma_sep := $(subst $(whitespace),$(comma),$(build_tags)) |
There was a problem hiding this comment.
[note for reviewers] Basically copy/pasted from cosmos-sdk/osmosis' Makefile. Linking here for comparison:
https://github.com/osmosis-labs/osmosis/blob/6ad2827c67cee185f2b7652ed7ae4cdea955f979/Makefile#L21-L43
Only diff:
Drop the netgo flag. I assume it was mainly there to ensure a statically linked net lib (compare: https://go.dev/src/net/netgo.go). it's orthogonal to ledger support and I am not sure we need it at all.
|
Ideally, we had a test to ensure that the build flag works as intended. I think there is a mock-ledger (at least when coding the KMS that was what we used; albeit in Rust). |
Codecov Report
@@ Coverage Diff @@
## main #2014 +/- ##
==========================================
+ Coverage 21.54% 25.05% +3.51%
==========================================
Files 126 121 -5
Lines 14285 13811 -474
==========================================
+ Hits 3077 3460 +383
+ Misses 10915 9995 -920
- Partials 293 356 +63 |
| 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 |
There was a problem hiding this comment.
[not blocking][question] does anyone have a Windows machine to test this?
| @@ -5,6 +5,39 @@ DOCKER_BUF := $(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace bu | |||
| IMAGE := ghcr.io/tendermint/docker-build-proto:latest | |||
| DOCKER_PROTO_BUILDER := docker run -v $(shell pwd):/workspace --workdir /workspace $(IMAGE) | |||
| PROJECTNAME=$(shell basename "$(PWD)") | |||
| LEDGER_ENABLED ?= true | |||
There was a problem hiding this comment.
[note for reviewers] ?= is the optional variable assignment operator. It sets the LEDGER_ENABLED variable if it has not previously been set
Source: https://www.gnu.org/software/make/manual/html_node/Setting.html
|
@liamsi has this PR been tested? In the absence of a test, can you please document in the PR description how it was manually verified? |
I did only test if I can import a key from a ledger: |
strip var Co-authored-by: Rootul P <[email protected]>
|
this LGTM! should we be worried about the docker build failing? should we set the default to false to avoid this for now until we fix docker? |
|
I tho I we can simply disable it for docker. Can look into that tmrw |
| # linux-headers are needed for Ledger support | ||
| linux-headers \ |
There was a problem hiding this comment.
[note to reviewers] this fixes the Docker workflow. Tested in rootulp#9
|
Circling back on this. I verified that I can add a key from Ledger |
… default (#2014) ## Overview Mainly add ledger flag to add ledger support by default. fixes #1658 --------- Co-authored-by: Rootul P <[email protected]>
#2415) … default (#2014) ## Overview Mainly add ledger flag to add ledger support by default. fixes #1658 --------- <!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> ## Checklist <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords Co-authored-by: Rootul P <[email protected]>
Overview
Mainly add ledger flag to add ledger support by default.
fixes #1658