Fix Cross Compile Build/Ledger Build Tag #1674

alexanderbez · 2018-07-13T18:17:49Z

Changelog

Cleaned up Makefile formatting a bit
Added check-ledger target used by install and build-linux
Fixed cross-compilation by having ledger support turned off by default (build-linux only)
Refactored ledger/device implementation in crypto/ to use an interface and built tag.

To build with ledger support: $ LEDGER_ENABLED=true|false make install

closes: #1581

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

cwgoes · 2018-07-13T18:20:31Z

This should be non-breaking, correct?

alexanderbez · 2018-07-13T18:23:38Z

@cwgoes ledger is disabled by default. We can have it enabled by default except when cross-compiling (aka, build-linux). Is this preferable?

cwgoes · 2018-07-13T18:58:30Z

We can have it enabled by default except when cross-compiling (aka, build-linux). Is this preferable?

Yes, I think so.

cwgoes · 2018-07-13T19:45:07Z

Makefile

+ifdef GCC
+BUILD_TAGS += ledger
+else
+$(error "gcc not installed for ledger support, please install")


Will clang work too?

(or does CGO require gcc anyways?)

it requires gcc afaik.

cwgoes · 2018-07-13T19:46:48Z

crypto/ledger.go

+// If ledger support (build tag) has been enabled, automically attempt to load
+// and set the ledger device, ledgerDevice, if it has not already been set.
+func init() {
+	if ledgerDevice == nil {


Why do we need to check this - init() will only be called once?

Yes, init will only be called once. Alternatively, you could wrap it in a once.Do(...) call.

Nvm, I see you're point. The old code was this way -- I'll remove it 👍

cwgoes · 2018-07-13T19:48:13Z

crypto/ledger_test.go

 )

-func TestRealLedgerSecp256k1(t *testing.T) {
+var ledgerEnabledEnv = "LEDGER_ENABLED"


Hmm, do we want to use the same environment variable here? e.g. we want CI to build with Ledger support, but it can't run the tests that require having a Ledger plugged in

codecov · 2018-07-13T20:51:15Z

Codecov Report

Merging #1674 into master will decrease coverage by 0.07%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   62.15%   62.07%   -0.08%     
==========================================
  Files         117      116       -1     
  Lines        6938     6929       -9     
==========================================
- Hits         4312     4301      -11     
- Misses       2384     2385       +1     
- Partials      242      243       +1

cwgoes · 2018-07-13T20:55:29Z

TEST_WITH_LEDGER=1 go test ./crypto/... fails with:

--- FAIL: TestRealLedgerSecp256k1 (0.00s)
	Error Trace:	ledger_test.go:23
	Error:      	Expected nil, but got: &errors.errorString{s:"failed to create PrivKeyLedgerSecp256k1: missing ledger device"}
	Test:       	TestRealLedgerSecp256k1
	Messages:   	failed to create PrivKeyLedgerSecp256k1: missing ledger device
FAIL
FAIL	github.com/cosmos/cosmos-sdk/crypto	0.026s

Ledger app is running, and gaiacli keys add --ledger works fine.

ValarDragon · 2018-07-13T20:58:54Z

Makefile

+
+check-ledger: 
+ifeq ($(LEDGER_ENABLED),true)
+ifndef GCC


Should we add a check here for the linux udev rules?
https://support.ledgerwallet.com/hc/en-us/articles/115005165269-Connection-issues-with-Windows-or-Linux

Mhmmm, not sure? But feel free to update it if it makes sense :-)

alexanderbez · 2018-07-13T21:00:57Z

@cwgoes ahh indeed. `init() isn't being called. Fixing...

EDIT: Ahh, wait. That test needs to run with the ledger build tag :-/

ValarDragon · 2018-07-13T21:00:42Z

crypto/ledger.go

@@ -0,0 +1,18 @@
+// +build ledger


Can this be cgo,ledger just so that this doesn't work if ledger flag is set but cgo isn't installed.

cwgoes · 2018-07-13T21:18:29Z

Ahh, wait. That test needs to run with the ledger build tag

Any idea how to do that?

ValarDragon · 2018-07-13T21:20:35Z

Yeah, you just add // +build ledger at the top of the test file, and run go test -v tags=ledger when running tests in that directory. Output:

=== RUN   TestKeyEncodings
--- PASS: TestKeyEncodings (0.05s)
=== RUN   TestNilEncodings
--- PASS: TestNilEncodings (0.00s)
=== RUN   TestRealLedgerSecp256k1
--- FAIL: TestRealLedgerSecp256k1 (0.15s)
        Error Trace:    ledger_test.go:29
	Error:      	Expected nil, but got: &errors.errorString{s:"invalid status 6a80"}
	Test:       	TestRealLedgerSecp256k1
=== RUN   TestRealLedgerErrorHandling
--- FAIL: TestRealLedgerErrorHandling (0.15s)
        Error Trace:    ledger_test.go:67
	Error:      	An error is expected but got nil.
	Test:       	TestRealLedgerErrorHandling
=== RUN   ExamplePrintRegisteredTypes
--- PASS: ExamplePrintRegisteredTypes (0.00s)
FAIL
exit status 1
FAIL	github.com/cosmos/cosmos-sdk/crypto	0.478s=== RUN   TestKeyEncodings
--- PASS: TestKeyEncodings (0.05s)
=== RUN   TestNilEncodings
--- PASS: TestNilEncodings (0.00s)
=== RUN   TestRealLedgerSecp256k1
--- FAIL: TestRealLedgerSecp256k1 (0.15s)
        Error Trace:    ledger_test.go:29
	Error:      	Expected nil, but got: &errors.errorString{s:"invalid status 6a80"}
	Test:       	TestRealLedgerSecp256k1
=== RUN   TestRealLedgerErrorHandling
--- FAIL: TestRealLedgerErrorHandling (0.15s)
        Error Trace:    ledger_test.go:67
	Error:      	An error is expected but got nil.
	Test:       	TestRealLedgerErrorHandling
=== RUN   ExamplePrintRegisteredTypes
--- PASS: ExamplePrintRegisteredTypes (0.00s)
FAIL
exit status 1
FAIL	github.com/cosmos/cosmos-sdk/crypto	0.478s

alexanderbez · 2018-07-13T21:22:59Z

@ValarDragon you don't have to add it to the test file file. Doing the following should suffice:

$ TEST_WITH_LEDGER=1 GOCACHE=off go test -tags "ledger" -timeout 30s github.com/cosmos/cosmos-sdk/crypto -run ^TestRealLedgerSecp256k1$

…into bez/1581-ledger-build

cwgoes · 2018-07-14T00:44:14Z

@ValarDragon and I made a few minor modifications to update the testsuite for the real Ledger.

ValarDragon

LGTM
Has cross compilation been tested?

alexanderbez · 2018-07-14T02:04:50Z

@ValarDragon cross compilation works (without ledger), but could use some actual testing. Maybe running a local test net?

cwgoes · 2018-07-14T02:16:03Z

Maybe running a local test net?

Good idea - also, we need to backport this to v0.21.0 - #1683.

cwgoes

Tested ACK

Merging so Voyager can build (we can definitely test on an internal testnet too).

* Merge pull request #1674: Fix Cross Compile Build/Ledger Build Tag * Merge pull request #1674: Fix Cross Compile Build/Ledger Build Tag * Remove incorrect Ledger test

alexanderbez requested review from cwgoes, ebuchman and rigelrozanski as code owners July 13, 2018 18:17

alexanderbez requested a review from greg-szabo July 13, 2018 18:17

alexanderbez added prelaunch labels Jul 13, 2018

alexanderbez force-pushed the bez/1581-ledger-build branch from d937864 to dd911de Compare July 13, 2018 18:22

alexanderbez force-pushed the bez/1581-ledger-build branch from dd911de to 53787f5 Compare July 13, 2018 18:24

alexanderbez force-pushed the bez/1581-ledger-build branch from 53787f5 to f4ade7b Compare July 13, 2018 18:31

alexanderbez added ready-for-review and removed wip labels Jul 13, 2018

cwgoes reviewed Jul 13, 2018

View reviewed changes

Merge pull request #1674: Fix Cross Compile Build/Ledger Build Tag

dcd1994

alexanderbez force-pushed the bez/1581-ledger-build branch from f4ade7b to dcd1994 Compare July 13, 2018 20:35

Merge branch 'master' into bez/1581-ledger-build

0683844

ValarDragon reviewed Jul 13, 2018

View reviewed changes

ValarDragon requested changes Jul 13, 2018

View reviewed changes

Merge pull request #1674: Fix Cross Compile Build/Ledger Build Tag

c9da1f6

alexanderbez force-pushed the bez/1581-ledger-build branch from 0683844 to c9da1f6 Compare July 13, 2018 21:25

cwgoes added 3 commits July 14, 2018 02:35

Remove incorrect Ledger test

07c558d

Merge branch 'cwgoes/fix-ledger-tests' into bez/1581-ledger-build

de95be4

Merge branch 'bez/1581-ledger-build' of github.com:cosmos/cosmos-sdk …

1b4bdd7

…into bez/1581-ledger-build

ValarDragon approved these changes Jul 14, 2018

View reviewed changes

cwgoes approved these changes Jul 14, 2018

View reviewed changes

cwgoes merged commit bb1f1a2 into master Jul 14, 2018

cwgoes deleted the bez/1581-ledger-build branch July 14, 2018 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Cross Compile Build/Ledger Build Tag #1674

Fix Cross Compile Build/Ledger Build Tag #1674

alexanderbez commented Jul 13, 2018 •

edited

Loading

cwgoes commented Jul 13, 2018

alexanderbez commented Jul 13, 2018

cwgoes commented Jul 13, 2018

cwgoes Jul 13, 2018

cwgoes Jul 13, 2018

alexanderbez Jul 13, 2018

cwgoes Jul 13, 2018

alexanderbez Jul 13, 2018

alexanderbez Jul 13, 2018

cwgoes Jul 13, 2018

codecov bot commented Jul 13, 2018 •

edited

Loading

cwgoes commented Jul 13, 2018

ValarDragon Jul 13, 2018

alexanderbez Jul 13, 2018

alexanderbez commented Jul 13, 2018 •

edited

Loading

ValarDragon Jul 13, 2018

alexanderbez Jul 13, 2018

cwgoes commented Jul 13, 2018

ValarDragon commented Jul 13, 2018

alexanderbez commented Jul 13, 2018 •

edited

Loading

cwgoes commented Jul 14, 2018

ValarDragon left a comment

alexanderbez commented Jul 14, 2018

cwgoes commented Jul 14, 2018

cwgoes left a comment

Fix Cross Compile Build/Ledger Build Tag #1674

Fix Cross Compile Build/Ledger Build Tag #1674

Conversation

alexanderbez commented Jul 13, 2018 • edited Loading

Changelog

cwgoes commented Jul 13, 2018

alexanderbez commented Jul 13, 2018

cwgoes commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2018 • edited Loading

Codecov Report

cwgoes commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Jul 13, 2018

ValarDragon commented Jul 13, 2018

alexanderbez commented Jul 13, 2018 • edited Loading

cwgoes commented Jul 14, 2018

ValarDragon left a comment

Choose a reason for hiding this comment

alexanderbez commented Jul 14, 2018

cwgoes commented Jul 14, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

alexanderbez commented Jul 13, 2018 •

edited

Loading

alexanderbez commented Jul 13, 2018 •

edited

Loading