Move rippled dependencies into ripple-libpp #16

xasos · 2017-06-26T20:28:37Z

Moved most non-core dependencies into ripple-libpp.

codecov-io · 2017-06-26T20:33:40Z

Codecov Report

Merging #16 into master will decrease coverage by 16.83%.
The diff coverage is 72.82%.

@@             Coverage Diff             @@
##           master      #16       +/-   ##
===========================================
- Coverage     100%   83.16%   -16.84%     
===========================================
  Files           1       76       +75     
  Lines         143     3997     +3854     
===========================================
+ Hits          143     3324     +3181     
- Misses          0      673      +673

Impacted Files	Coverage Δ
.../extras/beast/unit_test/detail/const_container.hpp	`100% <100%> (ø)`
src/beast/extras/beast/unit_test/suite_list.hpp	`100% <100%> (ø)`
src/beast/extras/beast/unit_test/global_suites.hpp	`100% <100%> (ø)`
src/beast/extras/beast/unit_test/amount.hpp	`100% <100%> (ø)`
src/beast/extras/beast/unit_test/match.hpp	`44.44% <44.44%> (ø)`
src/beast/extras/beast/unit_test/runner.hpp	`64.61% <64.61%> (ø)`
src/beast/extras/beast/unit_test/suite.hpp	`66.94% <66.94%> (ø)`
src/beast/extras/beast/unit_test/suite_info.hpp	`80.64% <80.64%> (ø)`
src/beast/extras/beast/unit_test/reporter.hpp	`83.33% <83.33%> (ø)`
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaa1e36...15b3c28. Read the comment docs.

xasos · 2017-07-25T18:28:48Z

@ximinez @scottschurr I think this is ready for full review. Mind taking a look?

ximinez · 2017-07-25T18:39:13Z

Will do as soon as I can.

scottschurr · 2017-07-27T23:05:48Z

Travis build failed. Expected?

ximinez · 2017-07-28T18:01:00Z

CI builds should not fail. It looks like the install-dependencies.sh script needs to be brought over.

$ ( cd extras/rippled && bin/ci/ubuntu/install-dependencies.sh ../.. )

/home/travis/.travis/job_stages: line 57: cd: extras/rippled: No such file or directory

xasos · 2017-07-28T23:03:52Z

Rebased with working Travis CI builds.

scottschurr · 2017-08-01T01:40:32Z

src/test/ripple-libpp_unittest.cpp

+            bool (vm.count ("quiet")),
+            bool (vm.count ("unittest-log")));
+    }
+}


Clang suggests that an integer should be returned here. Zero?

scottschurr · 2017-08-01T01:43:25Z

There are a lot of changes here, clearly more than I could realistically review to any depth. Do you have some guidance for where I should spend my review time? What was the basic process you followed to make these changes? Thanks.

xasos · 2017-08-01T04:52:53Z

@scottschurr basically avoid any of the dependencies in the src/ folder. Here are some of the main files:

scottschurr

Generally things look pretty good. It seems like you're getting better about tagging things const when you can. Keep that up! Still, I pointed out a few things you might want to tend to.

In terms of testing, I believe the only consumer of this library right now is ripple-sign-serialize. You may have already tried this, but is there a way to test that with these modifications ripple-sign-serialize still links and runs? It would be comforting to see that work.

scottschurr · 2017-08-01T17:51:07Z

src/test/ripple-libpp_unittest.cpp

@@ -0,0 +1,154 @@
+//------------------------------------------------------------------------------


I did a search and it looks like all of the other test-like files are named *_test.cpp. There may be a good reason for this file to violate that pattern, but if there isn't I'd suggest naming this file ripple-libpp_test.cpp. Consistency may be the hobgoblin of little minds, but it makes programming easier 😸

scottschurr · 2017-08-01T19:48:30Z

src/test/ripple-libpp_unittest.cpp

+#include <boost/filesystem.hpp>
+#include <boost/format.hpp>
+#include <boost/program_options.hpp>
+#include <test/quiet_reporter.h>


For me, using clang on mac OS, quite a few of these #includes are not needed. Here's what was left after I removed the un-needed includes:

#include <BeastConfig.h> // #include <ripple/beast/core/SemanticVersion.h> // #include <ripple/basics/CheckLibraryVersions.h> #include <ripple/beast/unit_test.h> // #include <ripple/core/Config.h> // #include <ripple/core/ConfigSections.h> // #include <ripple/core/TerminateHandler.h> // #include <ripple/protocol/SystemParameters.h> #include <ripple/protocol/BuildInfo.h> #include <beast/unit_test/dstream.hpp> // #include <boost/algorithm/string/trim.hpp> // #include <boost/filesystem.hpp> // #include <boost/format.hpp> #include <boost/program_options.hpp> #include <test/quiet_reporter.h>

scottschurr · 2017-08-01T20:06:14Z

src/test/ripple-libpp_unittest.cpp

+    po::variables_map vm;
+
+    // Set up option parsing.
+    po::options_description desc ("General Options");


For your amusement, try running the following command:

./ripplelibpptest --help

The response I'm getting is:

rippled: Incorrect command line syntax. Use '--help' for a list of options.

😸

scottschurr · 2017-08-01T20:19:02Z

src/test/ripple-libpp_unittest.cpp

+    // Set up option parsing.
+    po::options_description desc ("General Options");
+    desc.add_options ()
+    ("unittest,u", po::value <std::string> ()->implicit_value (""), "Perform unit tests.")


They can also run a subset of the unit tests. For example try:

./ripplelibpptest --unittest=STAmount

That capability might be nice to document.

scottschurr · 2017-08-01T21:15:55Z

src/test/ripple-libpp_unittest.cpp

+            argument = vm["unittest-arg"].as<std::string>();
+        return runUnitTests(
+            vm["unittest"].as<std::string>(), argument,
+            bool (vm.count ("quiet")),


I spotted four casts to bool in this file. They are all okay, but I'll express my preference. There are two options:

You could remove the four casts entirely. C++ has legacy rules from C. Parameters in a function call go through implicit conversions. One of these conversions is from integer to boolean. So if you remove the casts then the compiler will automatically convert a non-zero count to true and a zero count to false.

If you like to be explicit about conversions, my personal preference is to use C++style casts, rather than C-style or function-style casts. So if you choose to retain the casts, I would prefer they look like:

static_cast<bool>(vm.count ("quiet"))

One of the reasons for that preference is that a C-style or function-style cast can silently become a reinterpret_cast during maintenance. See C++ Coding Standards by Sutter and Alexandresu, item 95 for a full justification.

scottschurr · 2017-08-01T21:58:42Z

src/BeastConfig.h

@@ -0,0 +1,165 @@
+//------------------------------------------------------------------------------


Unfortunately fixing the BeastConfig.h file is outside the scope of this project.

It has the wrong name.

I'm pretty sure that most of the #defines it contains are obsolete/unneeded.

It would be great to get rid of it altogether. It gives all of our *.cpp files a certain fragility.

But I'm guessing you did the right thing by just hauling it along in its current form...

Maybe I could bring it up in #cpp-informal? Would be good to remove it.

Agreed, it would be good to remove. I'm unsure who you're getting your marching orders from, or how much longer your internship lasts. If you have time and permission, then I think either removing or reducing and renaming BeastConfig.h would be a nice step forward. But it would want to be in a separate pull request.

ximinez · 2017-08-01T23:08:19Z

@scottschurr https://github.com/ripple/validator-keys-tool also uses the lib.

Resolves: RIPD-1469 Adds tests for protocol, beast, and basics with accompanying test binary. Includes accompaying dependencies and test directories from rippled.

xasos · 2017-08-03T01:02:45Z

Squashed and updated commit message.

ximinez

Sorry for leaving this to sit for so long.

Since most of the new files here are coming over from rippled, I started reviewing this by running diff -r -x .git -x build ripple-libpp rippled | less and looking for discrepancies. Obviously, there are a lot of files only in rippled, those are to be expected. There are also a lot of files with differences simply because the corresponding rippled code has changed, which is to be expected in a moving target like this, and can be updated in a later PR, but there were some other problems.

There are still a bunch of files from rippled that don't belong in the library, and aren't used. I pushed a few commits to my https://github.com/ximinez/ripple-libpp/commits/np-RIPD-1469 branch which remove about 250.

I also pushed a few commits to update the default behavior of the test app to run tests by default (it doesn't make much sense to me to require a parameter to do the thing the app is meant to do). I also added some more of the existing test cases into the test app - if the files live in the library, they should be tested. I doubt I got them all. And we can probably get a lot higher than 86.01% test coverage, though it may require writing some new tests to do it.

We are still going to have to decide what to do about some of the dependencies like beast and secp256k1.

Go ahead and check out my changes, and cherry-pick any you don't have problems with into your branch. Please do not squash the commits, though until the review passes.

ximinez · 2017-08-02T20:59:36Z

bin/ci/windows/ubuntu/build-and-test.sh

@@ -0,0 +1,96 @@
+#!/bin/bash -u


This file is a duplicate of the one in bin/ci/ubuntu and should be removed.

ximinez · 2017-08-02T20:59:50Z

bin/ci/windows/ubuntu/install-dependencies.sh

@@ -0,0 +1,82 @@
+#!/bin/bash -u


This file is a duplicate of the one in bin/ci/ubuntu and should be removed.

xasos requested a review from ximinez June 26, 2017 20:28

xasos assigned ximinez Jun 26, 2017

xasos requested a review from scottschurr June 26, 2017 20:29

xasos assigned scottschurr Jun 26, 2017

xasos force-pushed the RIPD-1469 branch from 8da69c4 to c24414c Compare July 5, 2017 18:14

xasos force-pushed the RIPD-1469 branch 2 times, most recently from dcd7dfe to e78c1eb Compare July 13, 2017 21:35

xasos force-pushed the RIPD-1469 branch 3 times, most recently from fd673f0 to 65abd05 Compare July 25, 2017 18:12

xasos force-pushed the RIPD-1469 branch 6 times, most recently from 715bcc0 to d795e35 Compare July 28, 2017 23:03

scottschurr reviewed Aug 1, 2017

View reviewed changes

Add ripple-libpp test binary + rippled libs

15b3c28

Resolves: RIPD-1469 Adds tests for protocol, beast, and basics with accompanying test binary. Includes accompaying dependencies and test directories from rippled.

xasos force-pushed the RIPD-1469 branch from d795e35 to 15b3c28 Compare August 3, 2017 01:02

ximinez requested changes Aug 22, 2017

View reviewed changes

		@@ -0,0 +1,154 @@
		//------------------------------------------------------------------------------

		@@ -0,0 +1,165 @@
		//------------------------------------------------------------------------------

Move rippled dependencies into ripple-libpp #16

Are you sure you want to change the base?

Move rippled dependencies into ripple-libpp #16

Uh oh!

Conversation

xasos commented Jun 26, 2017

Uh oh!

codecov-io commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xasos commented Jul 25, 2017

Uh oh!

ximinez commented Jul 25, 2017

Uh oh!

scottschurr commented Jul 27, 2017

Uh oh!

ximinez commented Jul 28, 2017

Uh oh!

xasos commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottschurr commented Aug 1, 2017

Uh oh!

xasos commented Aug 1, 2017

Uh oh!

scottschurr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximinez commented Aug 1, 2017

Uh oh!

xasos commented Aug 3, 2017

Uh oh!

ximinez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jun 26, 2017 •

edited

Loading

ximinez left a comment •

edited

Loading