Modernize CMake Build #2579
Modernize CMake Build #2579
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20180717 - 16:26:58 Test Results
|
CMakeLists.txt
Outdated
| @@ -877,7 +885,7 @@ set (CMAKE_CURRENT_BINARY_DIR ${save_CBD}) | |||
|
|
|||
| add_library (pbufs STATIC ${PROTO_SRCS} ${PROTO_HDRS}) | |||
| if ((NOT TARGET protobuf::libprotobuf) AND (NOT Protobuf_LIBRARIES)) | |||
| message ("protobuf lib not found - adding unity source for lib") | |||
| message (STATUS "protobuf lib not found - adding unity source for lib") | |||
There was a problem hiding this comment.
Would it be possible to remove the whole vendored protobuf code from this repository and have it as an external dependency instead? The vendored version is ancient...
There was a problem hiding this comment.
@MarkusTeufelberger I agree..and it's only used for windows which is confusing. Now that I've refactored the build, I'm going to look at the NIH stuff more closely, but any changes to that will happen in a future PR
Codecov Report
@@ Coverage Diff @@
## develop #2579 +/- ##
===========================================
- Coverage 70.83% 70.71% -0.12%
===========================================
Files 698 696 -2
Lines 54367 54595 +228
===========================================
+ Hits 38511 38608 +97
- Misses 15856 15987 +131
Continue to review full report at Codecov.
|
|
tagging @JoeLoser (at your convenience) for additional set of eyes on this. |
|
@mellery451 I'm travelling for work this week so my nights are busy with after work activities with colleagues, but I'll take a look this weekend. If I get some time before then, I'll start taking a look. Thanks for tagging me! |
CMakeLists.txt
Outdated
| include (CMakeFuncs) | ||
| if (target) | ||
| message (WARNING | ||
| "The target option is deprecated and will be removed in a future release") |
There was a problem hiding this comment.
Can you share the deprecation rollout plan? Or is it just TBD future release after this lands in master?
There was a problem hiding this comment.
it's TBD - typically this would happen "a few releases" after this change lands. Since it's just a build system change, I doubt it will cause much stir.
CMakeLists.txt
Outdated
|
|
||
| setup_build_boilerplate() | ||
| cmake_minimum_required (VERSION 3.7.0) |
There was a problem hiding this comment.
Was this version arbitrarily chosen or are you truly relying on features that only came in version 3.7.0?
There was a problem hiding this comment.
I guess I'm actually relying on 3.9 because of the protobuf find module (imported targets and static lib support): https://cmake.org/cmake/help/latest/release/3.9.html. I will update the required version here. I believe VS ships with some modified version of 3.9, so that's about as high as we can go right now I think.
| option (beast_force_debug | ||
| "Force BEAST_DEBUG regardless of DEBUG settings" | ||
| OFF) | ||
| # NOTE - THIS OPTION CURRENTLY DOES NOT COMPILE : |
There was a problem hiding this comment.
Maybe remove it and add a GitHub issue detailing to look into this?
CMakeLists.txt
Outdated
| endif () | ||
| if (san STREQUAL "memory" AND NOT (is_clang AND | ||
| CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 7)) | ||
| message (FATAL_ERROR "memory sanitizer only available for clang 7+") |
There was a problem hiding this comment.
Is this message meant to apply to AppleClang 7 or higher, or LLVM 7 (unreleased yet)? I would assume the former since I think even LLVM 4 or so has MSAN.
There was a problem hiding this comment.
good point - this is a weak check anyhow. I'm going to convert this to check_cxx_compiler_flag which is more directly what we want
There was a problem hiding this comment.
Agreed, leveraging check_cxx_compiler_flag is a good call.
CMakeLists.txt
Outdated
| endif () | ||
| endif () | ||
|
|
||
| # the remaining options are obscure and rarely used |
There was a problem hiding this comment.
Do we need to support them still? Who uses them?
CMakeLists.txt
Outdated
| INTERFACE | ||
| $<$<CONFIG:Debug>:_DEBUG> | ||
| $<$<AND:$<BOOL:profile>,$<NOT:$<BOOL:assert>>>:NDEBUG> | ||
| OPENSSL_NO_SSL2 # << WHAT DOES THIS DO ? |
There was a problem hiding this comment.
Still a TODO to look into this?
There was a problem hiding this comment.
it looks like it tries to disable SSLv2. This is also an option when configuring/building OpenSSL, but I guess if we do this it probably hides any v2 implementations (?). In any event, it belongs on the SSL lib, so I've added it as an interface compile def to the imported lib.
There was a problem hiding this comment.
It seems to hide the implementations of a given version number just by #ifdef like you mentioned. "SSLv2 is completely broken, and you should disable it during configuration." from
the openSSL wiki doesn't make me too worried about this flag though 😆
CMakeLists.txt
Outdated
| $<$<CONFIG:Debug>:_DEBUG> | ||
| $<$<AND:$<BOOL:profile>,$<NOT:$<BOOL:assert>>>:NDEBUG> | ||
| OPENSSL_NO_SSL2 # << WHAT DOES THIS DO ? | ||
| _SILENCE_STDEXT_HASH_DEPRECATION_WARNINGS) |
There was a problem hiding this comment.
I would be quite surprised if we need this. Is it left-over from before or did you verify this is actually needed?
There was a problem hiding this comment.
most of the flags here are legacy and have been carried down from previous build systems. I'm not sure what's the best way to go about deciding when to remove some of these.
There was a problem hiding this comment.
..and it turns out you need this if you build the (very old) protobuf library sources (which effectively only happens on windows right now...). SO, I've narrowed the scope of this define down to just the protobuf lib
There was a problem hiding this comment.
Always something. :)
Thanks for tightening the scope on it and making it more explicit to future readers. Without seeing it as a Windows-only-thing, I had no way of knowing what is cross-platform vs. what is Windows only.
CMakeLists.txt
Outdated
| endif () | ||
|
|
||
| if (static) | ||
| target_link_libraries (common INTERFACE -static-libstdc++) |
There was a problem hiding this comment.
This and the other things messing with CMAKE_CXX_FLAGS are good contenders for a toolchain file. But I do think it can be easily phased as the next step as this is already a big PR as is. Maybe just put it as a GH issue or are you already tracking these things internally in JIRA?
There was a problem hiding this comment.
I agree about the toolchain (see comment line 188). we track issues internally right now, but this might change in the future
| target_include_directories (opts SYSTEM INTERFACE ${JEMALLOC_INCLUDE_DIRS}) | ||
| target_link_libraries (opts INTERFACE ${JEMALLOC_LIBRARIES}) | ||
| get_filename_component (JEMALLOC_LIB_PATH ${JEMALLOC_LIBRARIES} DIRECTORY) | ||
| ## TODO see if we can use the BUILD_RPATH target property (is it transitive?) |
There was a problem hiding this comment.
Don't know the immediate answer to this, but in general, properties of an INTERFACE thing are transitive which is why modern CMake like them ;)
There was a problem hiding this comment.
@mellery451 I went ahead and set the target property appropriately. Feel free to cherry pick at JoeLoser@b55220d
| NIH dep: boost | ||
| #]===================================================================] | ||
|
|
||
| if ((NOT DEFINED BOOST_ROOT) AND (DEFINED ENV{BOOST_ROOT})) |
There was a problem hiding this comment.
Can you help me understand why we support both CMake defines and environment variables? I've been wondering it since I first jumped into the codebase. It seems like we could just get away with CMake defines at configure time.
There was a problem hiding this comment.
Sounds like legacy leftovers from scons and Travis builds.
There was a problem hiding this comment.
yes, it's legacy...but I think some devs find it easier to set this ENV whenever they upgrade boost and not have to pass it each time they configure.
There was a problem hiding this comment.
I see. Configure time is short enough that, from my perspective, it's hard to justify the extra code maintenance of handling both mechanisms. But, it's not too much code and it already exists, so probably not a big deal.
CMakeLists.txt
Outdated
| if (WIN32 OR CYGWIN) | ||
| # Workaround for MSVC having two boost versions - x86 and x64 on same PC in stage folders | ||
| if (DEFINED BOOST_ROOT) | ||
| if (CMAKE_SIZEOF_VOID_P EQUAL 8 AND IS_DIRECTORY ${BOOST_ROOT}/stage64/lib) |
There was a problem hiding this comment.
I think we can remove CMAKE_SIZEOF_VOID_P EQUAL 8 since rippled doesn't support non-64-bit Windows platforms, right?
There was a problem hiding this comment.
I think you're right...I'll make the previous 64 bit check FATAL and remove this check
There was a problem hiding this comment.
Ah, I missed that it was only treated as a warning. I think I've seen other checks in the codebase for asserting 64-bit builds, so we may be able to go clean those out if we require 64-bit across the board on all platforms at CMake configure rather than compile time.
| endif () | ||
| set (OPENSSL_MSVC_STATIC_RT ON) | ||
| set (_ssl_min_ver 1.0.2) | ||
| # HACK for travis |
There was a problem hiding this comment.
Why can't we just update the package version of openssl on Travis?
There was a problem hiding this comment.
https://docs.travis-ci.com/user/reference/overview/ - Travis still only offers Precise and Trusty.
Trusty only offers OpenSSL 1.0.1f (https://packages.ubuntu.com/source/trusty/openssl)
Travis seems unwilling/unable to update their base system (travis-ci/travis-ci#5821), a workaround would be to run docker inside a VM instance and build/test in there.
Alternatively you could download/install a deb package from a newer Ubuntu version and hope it works.
CMakeLists.txt
Outdated
| #[=========================================================[ | ||
| depending on how openssl is built, it might depend | ||
| on zlib. In fact, the openssl find package should | ||
| figure this out for us, but it does not currently... |
There was a problem hiding this comment.
Did you file or look for an issue upstream with Kitware in adding support for this?
There was a problem hiding this comment.
yes: https://gitlab.kitware.com/cmake/cmake/issues/16885 ...I can add a comment linking to that issue
| threads attempt to use the same database connection at | ||
| the same time. | ||
|
|
||
| VFALCO NOTE: This is the preferred threading model. |
There was a problem hiding this comment.
Have we played with using SQLITE_THREADSAFE=2 then?
| src/soci/src | ||
| src/soci/include | ||
| #[=========================================================[ | ||
| HACK for ninja..which doesn't properly see |
There was a problem hiding this comment.
Hmm, not sure I follow here. Mind elaborating? It's only a problem for Ninja generator too?
There was a problem hiding this comment.
yes - it's only a problem for ninja, and I don't know the root cause. The upshot is that this lib builds before the sqlite POST_BUILD step and thus can't see the headers in the expected location. All other generators work properly and it's TBD to investigate why ninja doesn't order this correctly. I even added, BYPRODUCTS to sqlite in an attempt to make this work but no luck.
CMakeLists.txt
Outdated
| #[=========================================================[ | ||
| this unity file is our interpretation of | ||
| what sources comprise soci-lib. | ||
| (this must be investigated/confirmed whenever updated) |
There was a problem hiding this comment.
Did you investigate/confirm this since you're the lastest to update it by porting it to modern CMake? ;)
There was a problem hiding this comment.
I think comment is confusing and I'll remove it. What I meant is that we (at some point) decided we knew what files constitute the SOCI library and we made a unity file to represent this. Whenever you update the underlying project, it's possible files could be added/renamed/removed/etc. and thus the unity file should be reviewed for accuracy. I plan in the near future to make soci and rocksdb external projects and get out of the business of deciding what files (and settings) are needed for these projects -- they have their own build systems to make these decisions.
CMakeLists.txt
Outdated
| #]===================================================================] | ||
|
|
||
| include (CheckCXXCompilerFlag) | ||
| CHECK_CXX_COMPILER_FLAG ("-std=c++11" COMPILER_SUPPORTS_CXX11) |
There was a problem hiding this comment.
I think we can safely remove this as rippled only supports the following min compilers as far as I can tell:
- GCC 5+
- Clang x+ (3.7ish probably based on features used that I've seen?)
- MSVC 2017?
In any regard, all of these compilers support cpp11 flag. I think we should guarantee our expectations of min compiler support very early on (I think we do already) -- well before NIH 3rd party lib CMake stuff.
There was a problem hiding this comment.
I agree..common sets the standard to c++14, so I think this is not needed
CMakeLists.txt
Outdated
| PRIVATE | ||
| $<$<BOOL:${is_gcc}>:-w> | ||
| PUBLIC | ||
| $<$<AND:$<BOOL:${is_clang}>,$<VERSION_GREATER_EQUAL:CMAKE_CXX_COMPILER_VERSION,7>>: |
There was a problem hiding this comment.
Same comment as earlier as I think this is imposing AppleClang versioning numbers which won't map well to LLVM version numbers. For example, I compile using LLVM Clang 6.
There was a problem hiding this comment.
good point - I'll change this to a strict Clang compiler check
| NIH dep: nudb | ||
|
|
||
| NuDB is header-only, thus is an INTERFACE lib in CMake. | ||
| TODO: move this into NuDB repo, add proper targets and |
There was a problem hiding this comment.
Did you consider contacting the library author and asking him to add exported targets for his or her library?
There was a problem hiding this comment.
Upstream is https://github.com/vinniefalco/NuDB and Vinnie seems focused on boost/beast these days. NuDB hasn't been touched in over a year even though it has some design issues (https://github.com/vinniefalco/NuDB/issues/46) and some trivial PRs that could be easily merged (https://github.com/vinniefalco/NuDB/pull/62).
|
|
||
| #[=========================================================[ | ||
| this one header is added as source just to keep older | ||
| versions of cmake happy. cmake 3.10+ allows |
There was a problem hiding this comment.
There are several nice features that came in 3.10 and 3.11 family. Any particular reason why 3.7 was chosen as the min version to support?
Jenkinsfile
Outdated
| @@ -551,7 +561,8 @@ set -ex | |||
| log_file=''' + "${bldlabel}.txt" + ''' | |||
| exec 3>&1 1>>${log_file} 2>&1 | |||
| ccache -s | |||
| source /opt/rh/devtoolset-6/enable | |||
| ##source /opt/rh/devtoolset-6/enable | |||
There was a problem hiding this comment.
Do we want to support CentOS 6 and CentOS7 or just one?
There was a problem hiding this comment.
this is for the CI build only - and it's centos 7.
CMakeLists.txt
Outdated
| > | ||
| $<$<BOOL:${is_clang}>: | ||
| -Wno-mismatched-tags | ||
| -Wno-deprecated-register |
There was a problem hiding this comment.
We can remove this I believe. I tested with LLVM Clang 6 locally.
CMakeLists.txt
Outdated
| -Wno-deprecated-register | ||
| > | ||
| $<$<AND:$<BOOL:${is_clang}>,$<COMPILE_LANGUAGE:CXX>>: | ||
| -Wno-redeclared-class-member |
There was a problem hiding this comment.
Think we can gut this one too. Removed and tested locally with Clang 6.
CMakeLists.txt
Outdated
| $<$<CONFIG:Debug>:-g>) | ||
| target_compile_definitions (common | ||
| INTERFACE | ||
| _FILE_OFFSET_BITS=64) |
There was a problem hiding this comment.
Do we need to specify this? The GNU manual states: "On 64 bit systems this macro has no effect since the *64 functions are identical to the normal functions."
CMakeLists.txt
Outdated
| $<$<BOOL:${werr}>:-Werror> | ||
| $<$<COMPILE_LANGUAGE:CXX>: | ||
| -frtti | ||
| -Wno-invalid-offsetof |
There was a problem hiding this comment.
We can remove this I believe. I tested with LLVM Clang 6 locally.
| @@ -20,6 +20,9 @@ | |||
| #include <sys/param.h> | |||
| #define DONNA_INLINE inline __attribute__((always_inline)) | |||
| #define DONNA_NOINLINE __attribute__((noinline)) | |||
| #ifdef ALIGN | |||
| #undef ALIGN | |||
| #endif | |||
There was a problem hiding this comment.
Do we really need to check if it's defined before the undef? Why not just #undef ALIGN? Also, which library is conflicting with this? It would be nice to fix this outside the header.
There was a problem hiding this comment.
agree - will fix.
| if (target) | ||
| message (WARNING | ||
| "The target option is deprecated and will be removed in a future release") | ||
| parse_target() |
There was a problem hiding this comment.
Removing target pretty much requires calling cmake from a script for anything other than a default build. Compare:
cmake -Dtarget=gcc.debug.unity ..
cmake -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_BUILD_TYPE=Debug -Dunity=ON ..
No one is going to type the second command more than once before writing a script. So we do simplify the build script by removing this, but at the cost of pushing the complexity onto other developers.
No change needed, but we should address this when we do remove target
There was a problem hiding this comment.
just want to note that alternative to what you've written:
CC=gcc CXX=g++ cmake -DCMAKE_BUILD_TYPE=Debug
(since unity defaults to on, you don't have to specify it). Also, we could choose to make the build type default to Debug since the "unspecified" build type is cmake is pretty useless except for validating that compiling works)
There was a problem hiding this comment.
I'm in favor of defaulting the CMAKE_BUILD_TYPE to Debug. Also, one thing to keep in mind is how a lot of these variables you're passing to configure at CMake time will be in your CMakeCache. I rarely need to reconfigure, and even if I do, I am typically using the same compiler. So, it suffices to just have an out of source build folder, configure once, and then invoke Ninja many times without rerunning CMake.
There was a problem hiding this comment.
I would rather default to "release". There are so extremely few external developers compared to people running the software that it seems like the most common use case is to build a release build.
There was a problem hiding this comment.
I agree with @MarkusTeufelberger I'd rather it default to Release
There was a problem hiding this comment.
Fair point @MarkusTeufelberger
CMakeLists.txt
Outdated
| #]===================================================================] | ||
|
|
||
| add_library (secp256k1 | ||
| src/secp256k1/src/secp256k1.c) |
There was a problem hiding this comment.
This changes the way rippled is built from a large set of object files all linked into a single executable to a set of libraries that are linked together. I don't object to that. However, these libraries are built as shared libraries (for non-static configurations). Do we want to force these libraries to be statically linked? I can't think of any benefits to a shared library for these sub-libraries, are there any?
There was a problem hiding this comment.
I think it's generally best left to the user to decide this - and that's why CMake has the BUILD_SHARED_LIBS setting. We should probably take the advice there and expose this as an explicit option() and we could then choose the default to be OFF (i.e. build static ripple lib(s) by default).
We already have the static option and we'd need to decide how, if at all, we want that to interact with BUILD_SHARED_LIBS. The static option currently determines how we link to our external/NIH dependencies. I propose that they should be orthogonal so that you can choose to do static or dynamic linking to deps while building shared or static ripple lib(s). I also think the default should be static=ON and BUILD_SHARED_LIBS=OFF, but I'm open to suggestions.
There was a problem hiding this comment.
I'm not sure what problem making these libraries dynamic solves. I think of these libraries as a "convenient way to set compile options on a set of files" and not really as stand alone libraries. I'd rather keep them internal to rippled.
However, I don't object too strongly to allowing dynamic linking, but lets think about two things before we close this issue:
-
Do we have to worry about version mismatches - i.e. running a new version of rippled with an old version of the dynamic libraries? Should we add a version number to the libraries?
-
Do we have to worry about name conflicts? We have libraries called
libbeast_legacy.so libcorepp.so libed25519-donna.so libsecp256k1.so. Some of those sound generic enough that another library might have the same name. Maybe add arippled_prefix? Also, we should renamelibcorepp.soat any rate.
If the above two things can be addressed then I'm OK with allowing dynamic linking of these new libraries.
There was a problem hiding this comment.
I try to stay out of the way when folks are working on tools, since I really appreciate not having to do it myself. But I'm going to side with @seelabs on this one. I have historically, at other work places, been a strong proponent for static linking as much as possible. It moves complexity (and potential for error) to link time rather than at (the beginning of) runtime. That gives a developer a better chance at catching errors and reduces the number of errors that users potentially have to deal with.
So, unless there are strong reasons to switch to dynamic linking, I'd prefer that the libraries be statically linked. Just my take...
There was a problem hiding this comment.
linking
@seelabs which libraries specifically are you referring to? corepp and
beast_legacy are most definitely NOT just a set of compile options - they
contain executable code that is reusable for certain applications (e.g.
signing). Any of the INTERFACE libraries here are indeed just bundles
of settings, but then those libraries don't have any installed artifacts other
than being part of the config files that are installed by our build and used by
clients of the libs. I should say, also, that I consider beast_legacy and
corepp really one library conceptually in that they serve a singular purpose
of exposing some of the core rippled functionality to client applications. The
fact that there are two libraries is just a reflection of the fact that
beast_legacy is code that we'd actually like to delete and/or replace (there
was a specific task to "break out" the old beast code to make it more obvious
what old code we still rely on and need to look at replacing. This is more
organizational than logical at this point).
As far as keeping things internal to rippled, I was under the impression that
we want to expose some functionality in the form of library. ripple-libpp was
one take on this. corepp (to be better named) is a second take on the same
concept and one that is properly installed/configured and can be rightly
included in a -devel package, for instance. Do you object fundamentally to the
concept of ripple-libpp?
If you agree that we want to provide a library for some client applications
to use, then comes the question of how to install and link to it. I'm saying
the linking type (dynamic vs. static) can be left to the user. Most libraries
out there leave this to the user. Even OpenSSL allows dynamic linking if you
want to, although they tend to make static the default. I'm saying we can do
the same - default to static, but allow dynamic if that's what you want. If
I were charged with building a -devel RPM, I'd probably just install a static
lib, but I could be convinced otherwise. Maybe we leave the dynamic lib option
to "advanced" users, which means you'd have to build from source.
There is also the somewhat related concept of how to link to third-party (NIH)
dependencies. Up until now, we've taken a bizarre hybrid approach of direct
source inclusion for some libraries (sqlite, secp256k, protobuf on windows,
etc.) and standard configure time discovery of system libs (openssl and
boost). The direct inclusion deps are implicitly statically linked (really,
object file linked - but it's all very non-transparent what's going on when
you do that). I find this duplicity somewhat confusing personally, and I've
attempted in this change to at least treat all NIH, as much as possible, the same.
Even if we subtree'd the dependency, I wanted to build it separately
becase it is fundamentally a third party library. It's not our code and yet
we just dropped it right into our exe build previously.
If we want to make the decision that all NIH deps are linked statically, that's
certainly one approach, but we would be rolling back
#2291, for instance. Or perhaps we only
want to allow optional dynamic linking to certain third party libs? If so,
which ones? Just boost? How about OpenSSL? If you force NIH to be static,
but allow corepp to be built dynamically, then you will have a dynamic lib
that depends on a static lib, which I always think is a little weird, but
still feasible. Again, if we want to make the sweeping decision that ALL
libs involved in rippled (both NIH and our libs) are static, then it
simplifies things somewhat but ties users' hands with respect to
link-type.
versioning
If you are going to have multiple versions of rippled + libs installed
on your system, then you'd need to manage that like you
would any other libs. How would you handle multiple versions of OpenSSL or
boost? You'd probably install into separate prefixes. Same thing applies here -
for example:
/opt/rippled/1.0/
.../bin
.../lib
.../include
/opt/rippled/1.1/
.../bin
.../lib
.../include
Remember that cmake links using rpath, so the binary should have a pretty
strong link to the library it was installed with.
CMake supports SOVERSION,
so if we think that's the best practice, we can certainly do that for our
library. This is effectively version 1 of our library and we've not made any
attempt to version our API up to this point. If we want to start lib/API
versioning now, then I guess SOVERSION is a place to start.
One other note, the cmake config package that is created for our library supports
version checking and compatibility and I've chosen the "SameMajorVersion" compat check
for now, but we can certainly change that if we want:
https://github.com/ripple/rippled/blob/735c8d0486b0dc69683a123e6c4f520c505a4fcc/CMakeLists.txt#L1948-L1951
This version checking covers you at build time if you are building a client
application of some kind using the packages/libs we install.
name conflicts
I think the NIH libs are the most likely to potentially conflict (sqlite, for
example), but that's mostly a reflection of our decision to build those
libs ourselves instead of relying on installed/system packages. Given that
we do have these private builds of several NIH libs, I'd be inclined to
advise against installing rippled in common paths like /usr or /usr/local
because of the potential to collide with system libs. Again, rpath will
help us here, although universally switching to static linking also obviates
this issue. I think in its present form, rippled should be generally be
installed using it's own prefix like /opt/rippled, for example.
naming
Now the hardest part of this entire project: naming the library. Here are some
ideas, but I'm not married to anything (please suggest something better...):
libripplepp
libledgersign
libxrpproto
If you want a different name for beast_legacy, just say so - I consider it less
public facing so I'm not quite as concerned about that one.
There was a problem hiding this comment.
I had assumed that all these libraries were internal to rippled and were not meant to be used by themselves. If corepp is meant to be used stand alone then I agree that the user should be in charge of the link type and we should provide both static and dynamic options.
I agree is conceptually simpler to treat everything the same and in a dynamic build not force some libraries to be static. I do think we want to static link by default, but given corepp is meant to be used stand alone what you have here is best.
I don't know what the best way to handle versioning is, but since these are handled through rpath and presumably won't be on LD_LIBRARY_PATH I guess it doesn't matter.
For names:
I'd use the same prefix for all of them, maybe xrpl for xrp ledger? I wouldn't use pp, it's too cryptic. Maybe xrpl_core?
There was a problem hiding this comment.
ah, sorry if that wasn't clear. Yes, corepp is intended to supplant https://github.com/ripple/ripple-libpp. I like the xrpl prefix, and I want to see if I can just roll beast_legacy into an object lib so we don't have to deal with an extra artifact.
There was a problem hiding this comment.
I'm only just starting this review, but I want to share a few things I've had problems with already:
Windows, cmake version 3.11.0
I am running cmake with cmake -G"Visual Studio 15 2017 Win64" -Dassert=true ../../.., and building with ./Builds/Test.py -q -v --dir msvc --target rippled rippled_classic --config Debug Release --testjobs 6 -- -m, I get
c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.13.26128\include\hash_map(16): fatal error C1189: #error: <hash_map> is deprecated and will be REMOVED. Please use <unordered_map>. You can define _SILENCE_STDEXT_HASH_DEPRECATION_WARNINGS to acknowledge that you have received this warning. [C:\Dev\rippled-merge\build\cmake\msvc\pbufs.vcxproj]
Linux, cmake version 3.11.1:
With cmake -Dtarget=clang.debug -Dassert=true -DBOOST_ROOT=$BOOST_ROOT ../../.., the generation seems to work, and cmake --build ./build/cmake/clang.debug -- -j8 builds, but ./Builds/Test.py -q -v --dir clang.debug clang.release gcc.debug gcc.release --target rippled rippled_classic --testjobs=8 -- -j8fails with
You have changed variables that require your cache to be deleted.
Configure will be re-run and you may have to reset some variables.
The following variables have changed:
CMAKE_C_COMPILER= /home/eah/dev/rippled-merge/build/cmake/clang.debug/clang
CMAKE_CXX_COMPILER= /home/eah/dev/rippled-merge/build/cmake/clang.debug/clang++
-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
CMake Error at CMakeLists.txt:11 (project):
The CMAKE_C_COMPILER:
/home/eah/dev/rippled-merge/build/cmake/clang.debug/clang
is not a full path to an existing compiler tool.
etc.
Builds/Test.py
Outdated
| @@ -213,6 +213,7 @@ def shell(cmd, args=(), silent=False): | |||
| stdin=subprocess.PIPE, | |||
| stdout=subprocess.PIPE, | |||
| stderr=subprocess.STDOUT, | |||
| env=cust_env | |||
There was a problem hiding this comment.
Missing , (comma) at the end of this line.
CMakeLists.txt
Outdated
| #]===================================================================] | ||
|
|
||
| get_property (is_multiconfig GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) | ||
| ## this property is only in cmake 3.9+, so have a fallback/best-guess |
There was a problem hiding this comment.
Since you changed the cmake min required version from 3.7 to 3.9, I think we can simplify this to not need the fallback.
There was a problem hiding this comment.
@mellery451 here's a commit you can pick if you want that should be safe: JoeLoser@f39f9df
CMakeLists.txt
Outdated
| src/ripple/beast/*.h | ||
| src/ripple/beast/*.hpp) | ||
|
|
||
| add_library (xrpl_legacy |
There was a problem hiding this comment.
Names are hard. I don't love xrpl_legacy. No change needed, just registering that we could improve on the legacy name if someone had a better idea. Maybe xrpl_beast?
There was a problem hiding this comment.
Since you are probably the only ones using this library and maintaining it, why not completely rename it to something like libvinnie?
There was a problem hiding this comment.
yeah, naming is hard ;). Another option is to just fold these files into xrpl_core since they are really just the legacy parts of core. The physical separation into two libraries was just to make it easier to replace/remove/deprecate parts of beast/legacy/vinnie moving forward, but I'm not sure it's worth the extra component at this point
CMakeLists.txt
Outdated
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src> | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src/ripple> | ||
| $<INSTALL_INTERFACE:include>) | ||
| target_compile_options (xrpl_core | ||
| PUBLIC | ||
| $<$<BOOL:${is_gcc}>:-Wno-maybe-uninitialized>) |
There was a problem hiding this comment.
I know you didn't touch this here, but just commenting that this seems a bit odd that this flag is applied only to GCC.
|
First of all, GREAT work, @mellery451. So glad to see the CMake improvements. I've put a lot of comments as I came across things, but do have one question that need not be addressed in this PR. I see that we only allow the option of turning on profiling flags and sanitizers for GCC builds. Is there any particular reason to not relax that to also support clang builds? The compiler flags should map 1-1 AFAIK. |
CMakeLists.txt
Outdated
| -Gy- # Function level linking: disabled | ||
| -MP # Multiprocessor compilation | ||
| -openmp- # pragma omp: disabled | ||
| #-FS # forces PDB writes through server.-MP enables this by default |
There was a problem hiding this comment.
Perhaps just remove this line entirely?
|
It looks like we do not seem to need to define Since Rippled only allows builds using 64-bit systems, we do not need to define this. Here is a commit that removes it: JoeLoser@36f3e2b |
Jenkinsfile
Outdated
| shortbld = shortbld.replace('release', 'rel') | ||
| shortbld = shortbld.replace('unity', 'un') | ||
| // this is just an attempt to shorten the | ||
| // summary text lable to the point of absurdity.. |
There was a problem hiding this comment.
Typo here. I think you mean table. I like the nature of the comment though ;)
Switch to target-oriented dependencies. Use imported targets for dependencies (openssl, boost). Localize FindBoost to remove cmake version dependence for latest boost support. Logically separate "ripple-libpp" core sources and add install targets. Add ninja build for msvc. Add two clang sanitizer builds. Misc script changes to work with latest modernized cmake.
Switch to target-oriented dependencies. Use imported targets for installed dependencies (openssl, boost). Localize FindBoost to remove cmake version dependence for latest boost support. Logically separate "ripple-libpp" core sources and add install targets. Update CI scripts to use proper cmake conventions. Logically separate NIH dependencies as static libs.