-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add Depends to monero #3430
Add Depends to monero #3430
Conversation
2afad21
to
4886588
Compare
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.
I have to say, my eyes glazed over quick :)
.travis.yml
Outdated
- $HOME/.ccache | ||
env: | ||
global: | ||
- MAKEJOBS=-j3 |
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.
That should not be overridden (or there should be a user override).
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 is a travis CI instruction set. Why is the override problematic? I only included it, because it is quite useful, but basically it does not much more than the current build bot.
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.
Because building monero gobbles up a lot of RAM and 3 jobs means at least 6 GB.
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.
Removed MAKEJOBS.
cmake/FindLibunwind.cmake
Outdated
@@ -18,9 +18,10 @@ find_path(LIBUNWIND_INCLUDE_DIR libunwind.h | |||
|
|||
find_library(LIBUNWIND_LIBRARIES NAMES unwind ) | |||
if(NOT LIBUNWIND_LIBRARIES STREQUAL "LIBUNWIND_LIBRARIES-NOTFOUND") | |||
if (CMAKE_COMPILER_IS_GNUCC) | |||
if (CMAKE_COMPILER_IS_GNUCC AND NOT DEPENDS) |
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.
That might be better off done in the caller, since it's find macro ?
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 was actually not needed anymore. Removed.
src/common/stack_trace.cpp
Outdated
@@ -119,7 +119,6 @@ void log_stack_trace(const char *msg) | |||
int status; | |||
const char *log = stack_trace_log.empty() ? NULL : stack_trace_log.c_str(); | |||
#endif | |||
|
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.
Please remove :)
5adf3a1
to
ac66997
Compare
I removed the travis yml from the pr. A lot of packages are required (like gcc > v5) that are not available on the standard travis distro (ubuntu trusty). This could be solved by starting a docker container, that then runs a more up to date distro, but this goes over the scope of this pr. In general using travis with depends should be encouraged, since it allows cross platform CI builds on all repository forks. |
Is it worth removing it just to fix the -j setting ? |
It's not just the -j setting. The entire CI needs to be able to run in a docker container, since the current travis distro is out of date (cmake and gcc are too old). |
Rebased. I removed the depends recipes for unbound and miniupnp, instead the build flags are passed to the submodules. Windows builds will work, as soon as the unbound cmake does not check for the winpthread dll anymore. |
Opened #3 on the unbound fork to remove the winpthread check. |
HI @TheCharlatan could you give an ETA for this PR? |
@erciccione still working on it. Currently the following issues are still remaining, and the reasons why my local travis builds are failing:
|
It would be very, very nice if we could have this in time for the 0.13 builds at least for x86_64. Even just Linux would be better than nothing if windows is being a pain. Since libpcsc is being replaced, that just leaves the icu stuff. Any chance this can be done soon ? |
Depends cross compiles project dependencies for linux, mac and windows and multiple architectures. Depends is original work by Cory Fields and used in bitcoin and a wide range of bitcoin related projects.
Add readline, ldns, graphviz, unbound to depends packages Add a cmake toolchain file to depends that is uniquely created for every build and placed in triple/share/toolchain.cmake This file is then passed to cmake with -DCMAKE_TOOLCHAIN_FILE=/path/to/triple/share/toolchain.cmake Add the boost locale package to depends In the depends cmake toolchain file, a DEPENDS flag is added to exclude, or change cmake checks done that are required for depends Link miniupnpc and unwind from depends and not external Add libiconv and icu4c to depends, required for mingw32 builds. Headers (winsock) need to be lower case in order to compile on unix systems. This should not affect building on windows.
Add pcsc-lite to linux builds Fixup windows icu4c linking with depends, the static libraries have an 's' appended to them Compiling depends arm-linux-gnueabihf will allow you to compile armv6zk monero binaries
Explain the role of the SDK in the darwin build. Add instructions to compile depends to the basic readme.
Fix builds for native linux and windows The architecture flag was set incorrectly. It needs to be set only when compiling arm6.
Drop miniupnp and unbound depends builds. Make sure that build variables are propageted properly to unbound and miniupnp. Rebase to after the v0.12 release
This includes a minimal qt build without gui
b1f4317
to
cbbf4d2
Compare
Rebased.
|
CMakeLists.txt
Outdated
STAMP_DIR ${LRELEASE_PATH} | ||
CMAKE_ARGS -DLRELEASE_PATH=${LRELEASE_PATH} | ||
INSTALL_COMMAND cmake -E echo "") | ||
#>>>>>>> b1f43170... Add lrelease to the depends |
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.
Conflict markers
CMakeLists.txt
Outdated
@@ -812,8 +832,12 @@ endif() | |||
include_directories(SYSTEM ${Boost_INCLUDE_DIRS}) | |||
if(MINGW) | |||
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -Wa,-mbig-obj") | |||
set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi;crypt32) | |||
set(ICU_LIBRARIES ${Boost_LOCALE_LIBRARY} icuio icuin icuuc icudt icutu iconv) | |||
set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi) |
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.
crypt32 ?
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.
crypt32 is needed on Windows, yes.
NO_QT ?= | ||
NO_WALLET ?= | ||
NO_UPNP ?= | ||
FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources |
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.
Does this mean the build process actually downloads stuff ? If it does, does it actually execute it ?
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.
getmonero should make the sources available as well, so we can change the download path from bitcoincore.org to getmonero.org .
Each package is configured and patched so that it will yield the same | ||
build-results with each consequent build, within a reasonable set of | ||
constraints. Some things like timestamp insertion are unavoidable, and are | ||
beyond the scope of this system. Additionally, the toolchain itself must be |
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.
Does this not defeat the point or being able to check a hash, rather than diff binaries and work out that all differences are down to timestamps (which may be pretty hard for a non expert) ?
|
||
Each package must define its source location and checksum. The build will fail | ||
if the fetched source does not match. Sources may be pre-seeded and/or cached | ||
as desired. |
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.
That probably answers my question above, seems good.
For 64-bit Windows, you need mingw-w64-x86_64-hidapi in MSYS2. /Edit: Err, i just realised what I wrote... all the HID stuff is already included in mingw crosscompiler. Not sure at this point if source code changes are necessary. |
Nevermind my last comment, it was wrong. Looks like MSYS2 also uses libhidapi from here. |
Fixup arm toolchain file.
d51546c
to
ecaf5b3
Compare
icu tex support is not required, so just disable it. Re-add mistakingly removed crypt32 lib.
Set the architecture in the toolchain file correctly
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.
Thanks :)
4239735 Fixup 32bit arm build (TheCharlatan) a06d258 Fix Windows build (TheCharlatan) ecaf5b3 Add libsodium to the packages, the arm build was complaining about it. (TheCharlatan) cbbf4d2 Adapt translations to upstream changes (TheCharlatan) db57154 Updated pcsc url (TheCharlatan) f0ba19f Add lrelease to the depends (TheCharlatan) cfb3046 Add Miniupnp submodule (TheCharlatan) 5f7da00 Unbound is now a submodule. Adapt depends for this. (TheCharlatan) d6b9bdd Update readmes to reflect the usage of depends (TheCharlatan) 56b6e41 Add support for apple and arm building (TheCharlatan) 29311fd Disable stack unwinding for mingw32 depends build. (TheCharlatan) 8db3d57 Modify depends for monero's dependencies (TheCharlatan) 0806a23 Initial depends addition (TheCharlatan)
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.
Reviewed
The required packages are the names for each toolchain on apt. Depending on your distro, they may have different names. | ||
Then go back to the source dir and type for example for windows 64bit: | ||
|
||
* ```cmake -DCMAKE_TOOLCHAIN_FILE=`pwd`/contrib/depends/x86_64-w64-mingw32``` |
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 part seems incomplete, I think it's supposed to be cmake -DCMAKE_TOOLCHAIN_FILE=`pwd`/contrib/depends/x86_64-w64-mingw32/share/toolchain.cmake
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.
Yes, this is wrong. Do you want to correct it? If you do, please move the section back up to ### Building portable statically linked binaries (Cross Compiling)
. It seemed to have slipped down on my final rebase. Bit annoyed that the pr was still not in the best of states when it got merged.
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.
I'll get it done tomorrow :)
Depends is a localized cross compilation environment originally developed by Cory Fields. It allows its users to create cross compiled binaries for 32 and 64 bit linux and windows, arm hf, aarch64 and macos.
This pull request places depends into contrib/depends. The required monero dependencies are generated for each platform triplet (for example
x86_64-w64-mingw32
) and saved in their own directory in depends. Recipes for each package can be found incontrib/depends/packages
,packages.mk
defines the packages used for each platform triplet. The allowed ${triplet}'s are:i686-w64-mingw32
,x86_64-w64-mingw32
,x86_64-apple-darwin11
,x86_64-linux-gnu
,i686-linux-gnu
,arm-linux-gnueabihf
andaarch64-linux-gnu
.Libraries and tools are generated in depends by running
make HOST=${triplet}
. This step first downloads tar archives of the required dependency source files and places them in sources/. These archives are then checked for their sha256sum hash and compared with a hardcoded value in the package recipe. They are then extracted and placed into work/build during a build process. After building the package binaries/libraries are installed in work/staging . Once completed their staging directory is compressed and placed into built/ , after which their content in work/ is deleted. When every package is in built/ their archives are decompressed into ${triple}/ . This is where the monero build system will pick them up from.A cmake toolchain file is uniquely generated for ${triple} and placed in
contrib/depends/${triplet}/share/toolchain.cmake
. They can then be accessed by the monero build system by executing for example:cmake -DCMAKE_TOOLCHAIN_FILE=`pwd`/contrib/depends/${triplet}/share/toolchain.cmake
. The toolchain file also sets the DEPENDS variable to identify a depends build to the monero build system. This is used to exclude some additions from external as well as correcting some make flags.General usage is also briefly reflected in the Readme.md under "Building portable statically linked binaries".
Example of a depends build for 64 bit windows:
Install build essentials, nsis and g++-mingw-w64-x86-64 on your host system. Then:
Once it is finished building go back to the root directory and execute:
Advantages of using depends:
One of the things changed that also affect builds other than with the depends system is lowercasing the winsock header include.
Another is that in this pull request the usage of unwind on windows is disable in the source code. Some discussion needs to be had about it. In its current form it only allows builds from depends.
Further topics that need to be discussed are the combinations of system and processor architecture pairs that need to be able to be passed to the monero build system for armv6, v7 and v8.
I realize this is hard to review, and if there is anything I can do to make it easier, do not hesitate to leave suggestions. So far I tested binaries for linux and windows 64bit.