Skip to content

Conversation

@lclc
Copy link

@lclc lclc commented Dec 15, 2016

This PR adds a git submodule for libbase58, pointing to the last tagged release (0.1.4, same as in the Ubuntu repo), as suggested in the comments by @cdecker in #64.

Should fix #64 & make building on various systems easier (atm probably everything that is not Ubuntu) and the dependency version required/supported more clear.

I’m not very familiar with plain make files, so please let me know what I need to do different.

I would also write a cmake build system over Christmas / New Year if you're interested.
A list of some of the advantages of CMake: https://cmake.org/Wiki/Really_Cool_CMake_Features
I especially like the ootb creation of Debian and RPM packages.
Also cross-compiling gets much easier: http://www.vtk.org/Wiki/CMake_Cross_Compiling
It's very easy to read & understand and well documented.
I've already done the same for the Open Transactions library (https://raw.githubusercontent.com/lclc/opentxs/develop/CMakeLists.txt) and the Digital Bitbox hardware wallet code (https://raw.githubusercontent.com/digitalbitbox/mcu/master/CMakeLists.txt).

Lucas Betschart added 2 commits December 15, 2016 16:08
Pointing to the latest release (0.1.4)
@cdecker
Copy link
Member

cdecker commented Dec 16, 2016

CI reports some of the test binaries breaking:

valgrind -q --error-exitcode=7 --track-origins=yes --leak-check=full --show-reachable=yes bitcoin/test/run-tx-encode
bitcoin/base58.o: In function `to_base58':
/builds/ElementsProject/lightning/bitcoin/base58.c:31: undefined reference to `b58_sha256_impl'
/builds/ElementsProject/lightning/bitcoin/base58.c:32: undefined reference to `b58check_enc'
bitcoin/base58.o: In function `from_base58':
/builds/ElementsProject/lightning/bitcoin/base58.c:57: undefined reference to `b58_sha256_impl'
/builds/ElementsProject/lightning/bitcoin/base58.c:60: undefined reference to `b58tobin'
/builds/ElementsProject/lightning/bitcoin/base58.c:61: undefined reference to `b58check'
bitcoin/base58.o: In function `key_to_base58':
/builds/ElementsProject/lightning/bitcoin/base58.c:121: undefined reference to `b58check_enc'
bitcoin/base58.o: In function `key_from_base58':
/builds/ElementsProject/lightning/bitcoin/base58.c:132: undefined reference to `b58tobin'
/builds/ElementsProject/lightning/bitcoin/base58.c:133: undefined reference to `b58check'
collect2: error: ld returned 1 exit status
<builtin>: recipe for target 'daemon/test/run-maxfee' failed
make: *** [daemon/test/run-maxfee] Error 1

The object file should not be built inside the submodule, as that can
confuse git.

Not everything depends on the libbase58 header (CCAN doesn't), so
move that to the everything-else depends line.

The BITCOIN_SRC etc should also move to bitcoin/Makefile, but that's
a bigger change.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor

rustyrussell commented Dec 20, 2016 via email

@cdecker
Copy link
Member

cdecker commented Dec 20, 2016

Finally figured out how to push onto a PR created by another user 😉

I took the liberty of pushing the changes by @rustyrussell and me onto your branch @lclc, I'll try to get the CI to accept this. It was complaining about the unit tests. make full-check is your friend 😉

Trailing whitespace and include ordering was broken.
@lclc
Copy link
Author

lclc commented Dec 23, 2016

Ok great, thanks!

@cdecker cdecker requested a review from rustyrussell December 23, 2016 12:51
@cdecker
Copy link
Member

cdecker commented Dec 23, 2016

If @rustyrussell is ok with this I'll marge it 😄

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Rebased and merged.

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.

Install instructions for debian?

3 participants