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

Support boost 1.69 #2804

Closed
wants to merge 4 commits into from
Closed

Support boost 1.69 #2804

wants to merge 4 commits into from

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented Dec 12, 2018

No description provided.

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

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

I think our local FindBoost.cmake should be updated with latest: https://github.com/Kitware/CMake/blob/master/Modules/FindBoost.cmake

@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Dec 12, 2018

Jenkins Build Summary

Built from this commit

Built at 20190130 - 16:33:38

Test Results

Build Type Log Result Status
msvc.Debug logfile 1142 cases, 0 failed, t: 637s PASS ✅
gcc.Release
-Dassert=ON,
MANUAL_TESTS=true
logfile 915 cases, 0 failed, t: 6m12s PASS ✅
docs,
TARGET=docs
logfile 1 cases, 0 failed, t: 0m2s PASS ✅
gcc.Debug
-Dcoverage=ON,
TARGET=coverage_report,
SKIP_TESTS=true
logfile 1145 cases, 0 failed, t: 17m15s PASS ✅
clang.Debug logfile 1145 cases, 0 failed, t: 3m12s PASS ✅
msvc.Debug,
NINJA_BUILD=true
logfile 1142 cases, 0 failed, t: 656s PASS ✅
rpm logfile 1145 cases, 0 failed, t: n/a PASS ✅
gcc.Debug logfile 1145 cases, 0 failed, t: 3m28s PASS ✅
clang.Debug
-Dunity=OFF
logfile 1145 cases, 0 failed, t: 10m53s PASS ✅
clang.Release
-Dassert=ON
logfile 1145 cases, 0 failed, t: 5m43s PASS ✅
gcc.Debug
-Dunity=OFF
logfile 1145 cases, 0 failed, t: 10m20s PASS ✅
msvc.Debug
-Dunity=OFF
logfile 1142 cases, 0 failed, t: 1287s PASS ✅
gcc.Debug
-Dstatic=OFF
logfile 1145 cases, 0 failed, t: 3m28s PASS ✅
gcc.Release
-Dassert=ON
logfile 1145 cases, 0 failed, t: 6m8s PASS ✅
gcc.Debug
-Dstatic=OFF -DBUILD_SHARED_LIBS=ON
logfile 1145 cases, 0 failed, t: 3m28s PASS ✅
gcc.Debug,
NINJA_BUILD=true
logfile 1145 cases, 0 failed, t: 3m2s PASS ✅
msvc.Release logfile 1142 cases, 0 failed, t: 418s PASS ✅
clang.Debug
-Dunity=OFF -Dsan=address,
PARALLEL_TESTS=false,
DEBUGGER=false
logfile 1145 cases, 0 failed, t: 4m49s PASS ✅
clang.Debug
-Dunity=OFF -Dsan=undefined,
PARALLEL_TESTS=false
logfile 1145 cases, 0 failed, t: 7m57s PASS ✅

@HowardHinnant
Copy link
Contributor

It isn't looking good on macOS:

In file included from /Users/howardhinnant/Development/rippled/src/ripple/app/main/Main.cpp:49:
In file included from /Users/howardhinnant/Development/boost_1_69_0/boost/process.hpp:24:
In file included from /Users/howardhinnant/Development/boost_1_69_0/boost/process/async_system.hpp:22:
In file included from /Users/howardhinnant/Development/boost_1_69_0/boost/process/child.hpp:21:
In file included from /Users/howardhinnant/Development/boost_1_69_0/boost/process/detail/child_decl.hpp:30:
/Users/howardhinnant/Development/boost_1_69_0/boost/process/detail/posix/wait_for_exit.hpp:60:7: error: expected unqualified-id
    ::sigemptyset(&sigset);
      ^
/usr/include/signal.h:125:26: note: expanded from macro 'sigemptyset'
#define sigemptyset(set)        (*(set) = 0, 0)
                                ^

I've got a comment in cpplang.slack.com #boost about this. Waiting for responses.

@seelabs
Copy link
Collaborator Author

seelabs commented Dec 12, 2018

Pushed a commit to update FindBoost.cmake to the latest, as suggested by @mellery451

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Despite the macOS failure for boost 1.69, this PR tests out fine with boost 1.68 and 1.67.

Copy link
Contributor

@miguelportilla miguelportilla left a comment

Choose a reason for hiding this comment

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

👍 with boost 1.69.0 on Windows. I did see the following Cmake warning:

CMake Warning (dev) at C:\ripple\rippled\CMakeLists.txt:599 (find_package):
Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
command to set the policy and suppress this warning.

Line 599 in CMakeLists.txt find_package (OpenSSL ${_ssl_min_ver} REQUIRED)

Policy: https://cmake.org/cmake/help/git-stage/policy/CMP0074.html

@mellery451
Copy link
Contributor

@miguelportilla I would expect that warning for cmake 3.12.4 or greater, but I don't think it has anything to do with this PR. Maybe you recently upgraded your cmake version (or visual studio if it includes CMake)? We should go ahead and explicitly set policy CMP0074 in our cmake, but I don't think it needs to be in this PR.

@MarkusTeufelberger
Copy link
Collaborator

Anything missing from this PR?

Arch Linux ships boost 1.69 by now (https://www.archlinux.org/packages/extra/x86_64/boost/) and this would be nice to have.

@seelabs
Copy link
Collaborator Author

seelabs commented Jan 24, 2019

@MarkusTeufelberger There is an issue with the mac build that we don't have a good solution for so we decided not to merge this yet. I wasn't aware any distros were shipping with boost 1.69, so it seemed like a good idea to delay this.

However, this change should be backward's compatable with older boosts, so it should be OK to merge. Since arch ships with boost 1.69, I'll rebase/retest this and see if it can be marked passed. Thanks for the info about arch.

@seelabs seelabs force-pushed the boost-1.69 branch 2 times, most recently from 496bd9b to 58de894 Compare January 24, 2019 22:13
@seelabs
Copy link
Collaborator Author

seelabs commented Jan 24, 2019

Rebased, squashed and retested locally on boost 1.68 and 1.69. Unless there are objections from the team or CI issues I'll mark this passed when CI finishes.

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 25, 2019
@seelabs seelabs removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 30, 2019
@HowardHinnant
Copy link
Contributor

Still 👍 from me.

@miguelportilla
Copy link
Contributor

Latest beast changes LGTM. On Windows, all targets build and tests pass. 👍

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 30, 2019
@MarkusTeufelberger
Copy link
Collaborator

Fedora 30 will ship with Boost 1.69 too: https://apps.fedoraproject.org/packages/boost

@seelabs
Copy link
Collaborator Author

seelabs commented Feb 24, 2019

Thanks for the info @MarkusTeufelberger. BTW, this PR will go into the first 1.3.0 beta;

@nbougalis nbougalis mentioned this pull request Mar 7, 2019
@seelabs seelabs deleted the boost-1.69 branch April 24, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants