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

CGO / Ledger: Fix build issues #1581

Closed
3 of 6 tasks
ValarDragon opened this issue Jul 6, 2018 · 23 comments
Closed
3 of 6 tasks

CGO / Ledger: Fix build issues #1581

ValarDragon opened this issue Jul 6, 2018 · 23 comments
Assignees

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 6, 2018

Currently the ledger requires CGO as a dependency. This isn't installed in all machines and should not be the default for the SDK. We need to make this an opt-in build flag, with associated make command.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 7, 2018

That doesn't appear to be related to the CGO issues. That issue appears (to me) to be a problem with your wireless software/hardware. Also moved your logs to a pastebin, so the issue is still easy to scroll through.

EDIT: The previous message from another person was deleted.

@greg-szabo
Copy link
Member

greg-szabo commented Jul 9, 2018

We need to make this an opt-in build flag, with associated make command.

Interesting choice of words. I just run into the following use-case:

As the developer leveraging the cosmos-sdk, I want to write an application that does not use ledger or any ledger-related code.

So in my case, getting ledger-related errors in a completely unrelated application is frustrating. Can the ledger code be a separate library that I pull in if I need but doesn't get built if I'm not using it? This is more than just a Makefile change, because in this case I'm adding cosmos-sdk as a vendor dependency (no Makefile execution).

@cwgoes
Copy link
Contributor

cwgoes commented Jul 9, 2018

So in my case, getting ledger-related errors in a completely unrelated application is frustrating. Can the ledger code be a separate library that I pull in if I need but doesn't get built if I'm not using it? This is more than just a Makefile change, because in this case I'm adding cosmos-sdk as a vendor dependency (no Makefile execution).

Can dep support some kind of build flag? What we want is a way for downstream SDK users to specify whether or not they want to build with Ledger support (which can be disabled by default, perhaps, if the errors are frustrating).

@greg-szabo
Copy link
Member

Maybe. Based on my current knowledge, I would keep ledger integration as a separate library and make cosmos-sdk apply an interface (in the Java term, I'm not sure if Go does it the same way) so you can plug it in as a separate modul. We'll need to do more research on this.

@NodeGuy
Copy link
Contributor

NodeGuy commented Jul 11, 2018

The Voyager team isn't able to build the SDK (v0.20.0) for macOS or Windows:

export GOOS=darwin
make get_vendor_deps
make install
...
go install -tags netgo -ldflags "-X github.com/cosmos/cosmos-sdk/version.GitCommit=080dd5b9" ./cmd/gaia/cmd/gaiad
# github.com/cosmos/cosmos-sdk/vendor/github.com/zondax/ledger-goclient
vendor/github.com/zondax/ledger-goclient/ledger.go:76:18: undefined: hid.Devices
vendor/github.com/zondax/ledger-goclient/ledger.go:99:18: undefined: hid.Devices
Makefile:40: recipe for target 'install' failed
make: *** [install] Error 2

export GOOS=windows
make get_vendor_deps
make install
...
go install -tags netgo -ldflags "-X github.com/cosmos/cosmos-sdk/version.GitCommit=080dd5b9" ./cmd/gaia/cmd/gaiad
# github.com/cosmos/cosmos-sdk/vendor/github.com/zondax/ledger-goclient
vendor/github.com/zondax/ledger-goclient/ledger.go:76:18: undefined: hid.Devices
vendor/github.com/zondax/ledger-goclient/ledger.go:99:18: undefined: hid.Devices
Makefile:40: recipe for target 'install' failed
make: *** [install] Error 2
cp: cannot stat '/go/bin/*_amd64': No such file or directory
error Command failed with exit code 1.

@cwgoes
Copy link
Contributor

cwgoes commented Jul 11, 2018

The Voyager team isn't able to build the SDK (v0.20.0) for macOS or Windows:

While we should add a build flag, we definitely want Ledger support on Mac OS and Windows - I'm not sure what flags the upstream library may need for that though - https://github.com/brejski/hid.

@alexanderbez alexanderbez self-assigned this Jul 12, 2018
@faboweb
Copy link
Contributor

faboweb commented Jul 12, 2018

Note:
I could build SDK 0.20 on Mac OS. The error we are experiencing happens when building inside Docker.

@alexanderbez
Copy link
Contributor

@faboweb, correct, the build-linux target fails. I believe its still worthwhile making ledger an opt-in via a build tag.

@alexanderbez
Copy link
Contributor

Actually, I built this fine from within a docker container. Seems to be an issue when invoked from a mac. Looking into it further.

@NodeGuy
Copy link
Contributor

NodeGuy commented Jul 12, 2018

Our problem is that we build on Linux using cross compilation. It looks like cross compilation is no longer supported.

@ValarDragon
Copy link
Contributor Author

Reopening, since not all items are finished.

@ValarDragon ValarDragon reopened this Jul 14, 2018
@alexanderbez
Copy link
Contributor

alexanderbez commented Jul 16, 2018

@ValarDragon / @NodeGuy what exactly do we want to do here? I don't recommend cross compiling when you have dynamic libraries linked (libs) (hence ledger disabled). Cross compiling with CGO is extremely cumbersome from my experience, also of which I do not have much of. Might need someones help on this.

@NodeGuy
Copy link
Contributor

NodeGuy commented Jul 16, 2018

@greg-szabo We started building our own binaries of the SDK when there weren't any for the new version and you were on vacation. Are you still building binaries for all three platforms? Is your process always current with new SDK releases?

@greg-szabo
Copy link
Member

I'm not. The ledger integration threw off cross-compiling enough that I wanted to wait until things settle and we decide how we want to move forward.

Right now, if I want to use the SDK as an SDK (where I don't need Ledger), I add the following things:

  1. CGO_ENABLED=false
  2. LEDGER_ENABLED=false
  3. BUILD_FLAGS=-tags "netgo ledger" -ldflags "-extldflags "-static""

This allows me to build a static binary with no Ledger support. Any cross-compilation efforts with Ledger enabled were met with error messages, so I use docker to cross-compile to linux on my Mac or I compile it directly on Mac for Mac binary.

@alexanderbez
Copy link
Contributor

What @greg-szabo outlined is pretty much the set process right now. Cross-compiling with Ledger support is a huge pain -- I'm not even sure it can be done?

@NodeGuy
Copy link
Contributor

NodeGuy commented Aug 10, 2018

Would someone please talk to the Ledger people about this?

@cwgoes
Copy link
Contributor

cwgoes commented Aug 11, 2018

Would someone please talk to the Ledger people about this?

We've brought it up - but it's a lower priority than a Ledger app for validators.

@greg-szabo What error messages did cross-compile attempts result in?

@faboweb
Copy link
Contributor

faboweb commented Aug 30, 2018

Linking #2157

Any updates?

@cwgoes
Copy link
Contributor

cwgoes commented Aug 30, 2018

Native Golang Ledger isn't presently slated for pre-launch - is it a substantial issue Voyager-side or is cross-compilation workable?

@faboweb
Copy link
Contributor

faboweb commented Aug 31, 2018

We would need cross-compilation (which isn't working right now) to make it work in Voyager. We already agreed that this is not a pre-launch necessity. I was exploring this again as it was a feature @zmanian was pushing for. I will drop research on it, until this issue is resolved.

@alexanderbez
Copy link
Contributor

@cwgoes I don't think this is actually game of steaks needed. I mistook it for an actual SDK build issue. Thoughts?

@cwgoes
Copy link
Contributor

cwgoes commented Sep 29, 2018

It's not even prelaunch I think, see @faboweb's last comment.

@jackzampolin
Copy link
Member

Going to close this and track progress here with the SRE team.

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

No branches or pull requests

7 participants