-
Notifications
You must be signed in to change notification settings - Fork 34
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
Improve .pro, improve readme and add Linux and Mac build instructions #9
base: 2.1.0
Are you sure you want to change the base?
Improve .pro, improve readme and add Linux and Mac build instructions #9
Conversation
Now compiles but crash on launch because of missing resources. Details: - removed or fixed all paths using /Users/joe/ using brew standard paths - add include path for .moc files - remove -no-pie option, which isn't supported by clang - removed -pthread form LFLAGS, which is ignored by clang
- Start grouping instructions - rename windows to win32 (windows doesn't exist) - show x86 vs x86_64 build on windows - remove some obsolete commented instructions - add some explanations and questions - fix formatting
Those are compiled files, they should not be commited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done fradow! Damn good to have you on the team!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally approved but will examine in further detail soon (it's a big PR). Please wait for comment before merging.
- remove obsolete info about exchanges - edit Win deps text
INCLUDEPATH +=C:\deps\qrencode-3.4.4 | ||
INCLUDEPATH += C:\deps\curl-7.40.0\include | ||
INCLUDEPATH += C:\deps\libevent-2.0.21-stable\include | ||
INCLUDEPATH += $$PWD/ex_lib/qrencode-3.4.1-1-mingw32-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fradow-cloak : not sure we need 3.4.1 since 3.4.4 gets included @line 145
Just need to have the qrcode path defines (QRENCODE_LIB_PATH, QRENCODE_INCLUDE_PATH) to be used for all platforms; it was already, not sure if you removed it.
Can you also make sure the QR functionality works with this included lib version? it was a while ago, but IIRC the build was broken so I replaced it with a newer one.
qmake cmd line should have the QRCODE flag set to have it available in the built binary:
qmake.exe cloakcoin-qt.pro -r -spec win32-g++ "CONFIG+=debug" "USE_QRCODE=1" "USE_UPNP=1" "USE_IPV6=1"
|
||
#PRECOMPILED_HEADER = src/stable.h | ||
# Current app version | ||
VERSION = 2.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this VERSION is different from the displayed version, believe it was the initial bitcoin-core version
DEFINES += BOOST_THREAD_USE_LIB BOOST_SPIRIT_THREADSAFE BOOST_THREAD_PROVIDES_GENERIC_SHARED_MUTEX_ON_WIN | ||
|
||
# What's that? The only Google match I found is form Berkely DB. We might not need that | ||
DEFINES += __NO_SYSTEM_INCLUDES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one instructs the compiler/linker to not look at default system include paths, for gcc it would be equivalent as -nostdinc
flag. Depending on the order of the paths and what libs you have installed on the computer it could include wrong headers (different version).
memenv.h and libmemenv.a are still needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, bitcoin-core shipped with a modified version of leveldb, as original LevelDB repo didn't support all platforms and other specifics. dev packages were updated at some point, not sure if it was done with the bitcoin leveldb version. for this reason, we are gonna keep memenv includes
I did read the comment but won't have time to act on it for a while. Quick answer:
Thanks a lot for your detailed review and insightful comments :) |
@fradow-cloak : I'd even leave the VERSION as is, not sure, but it might be used for checking against supported features somewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some looking into, changes done after this PR
This is a big PR because it wasn't easy to split work into different PRs.
I personally tested the build on:
The .pro here was the one used to create version 2.1.0.0 of the Mac wallet.
Here is a list of changes:
Windows instructions are not there yet. I figured it would be best to have instructions for the Mac build as well as working Mac compilation now rather than later.
Feel free to comment on things you want improved before merging.