Less contentious restructure #4966
Conversation
thejohnfreeman
requested review from
godexsoft,
legleux,
ximinez and
scottschurr
thejohnfreeman
force-pushed
the
backport
branch
from
5104259
to
42e783e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4966 +/- ##
=========================================
- Coverage 71.0% 70.9% -0.0%
=========================================
Files 796 796
Lines 66727 66727
Branches 10973 10978 +5
=========================================
- Hits 47348 47336 -12
- Misses 19379 19391 +12
... and 5 files with indirect coverage changes |
There was a problem hiding this comment.
I like the changes 👍
Just one thing that i don't necessarily agree with is renaming to .h instead of going the other way and renaming everything to .hpp. Do you have C code in the project? Or why was .h preferred?
To me it feels like a bit of a step backwards.
One side-effect of using .h is that github will count and report some files as C code if the header is simple enough that it could pass for a C header.
There was a problem hiding this comment.
This restructure looks great to me.
FWIW, unlike @godexsoft, I think changing the few *.hpp files to *.h files is a good approach. The code base has a ton of *.h files. If we're going for uniformity (and I think that's a good idea) this smaller change is better. Switching all of the *.h files in the build to *.hpp would be much more disruptive.
Almost everything in this pull request built for me out of the box. But I had to make two small changes to the source to get the build to complete. Neither file that I had to change is currently in this change set. So I'll describe the issues here.
- In src/ripple/protocol/impl/SecretKey.cpp:194 I get the following error:
error: 'secp256k1_ec_privkey_tweak_add' is deprecated: Use secp256k1_ec_seckey_tweak_add instead [-Werror,-Wdeprecated-declarations]
if (secp256k1_ec_privkey_tweak_add(
^
If I change the secp256k1_ec_privkey_tweak_add on that line to secp256k1_ec_seckey_tweak_add then that file builds and links.
- I get the following error on src/test/protocol/MultiApiJson_test.cpp:55:
error: unused variable 'index' [-Werror,-Wunused-const-variable]
constexpr static auto index =
^
And, indeed, if I remove the five lines that declare this static lambda then everything builds and links just fine.
Once those small things are patched up everything builds, links, passes unit tests, and syncs on my Intel Mac.
|
@scottschurr I have no objections either way. If moving to |
That's a fair question, @godexsoft. In terms of disruption I was thinking of folks, like me, who work in the rippled code base. Renaming fewer files means I don't have to retrain that part of my brain. I also frequently have trouble tracing history through renamed files in git. Reducing the number of renamed files will increase the likelihood that looking at history using git continues to work for more files. But neither of those are killer arguments. It's mostly about my personal convenience. So, yeah, I'm still leaning toward the current approach that @thejohnfreeman has taken. |
|
I prefer The next PR is about to rename every file in |
Two that I know about are: |
external/README.md
Outdated
| These directories come from outside. | ||
| The subdirectories in this directory contain either copies or Conan recipes | ||
| of external libraries used by rippled. | ||
| The Conan recipes include patches we have not yet pushed upstream. | ||
|
|
||
| | Folder | Upstream Repo | Description | | ||
| | Folder | Upstream | Description | | ||
| |:----------------|:---------------------------------------------|:------------| | ||
| | `ed25519-donna` | https://github.com/floodyberry/ed25519-donna | [Ed25519](http://ed25519.cr.yp.to/) digital signatures | | ||
| | `rocksdb` | https://github.com/conan-io/conan-center-index/tree/master/recipes/rocksdb | Fast key/value database. (Supports rotational disks better than NuDB.) | | ||
| | `secp256k1` | https://github.com/bitcoin-core/secp256k1 | ECDSA digital signatures using the **secp256k1** curve | | ||
| | `snappy` | https://github.com/conan-io/conan-center-index/tree/master/recipes/snappy | "Snappy" lossless compression algorithm. | | ||
| | `soci` | https://github.com/conan-io/conan-center-index/tree/master/recipes/soci | Abstraction layer for database access. | | ||
| | `ed25519-donna` | [Project](https://github.com/floodyberry/ed25519-donna) | [Ed25519](http://ed25519.cr.yp.to/) digital signatures | | ||
| | `rocksdb` | [Recipe](https://github.com/conan-io/conan-center-index/tree/master/recipes/rocksdb) | Fast key/value database. (Supports rotational disks better than NuDB.) | | ||
| | `secp256k1` | [Project](https://github.com/bitcoin-core/secp256k1) | ECDSA digital signatures using the **secp256k1** curve | | ||
| | `snappy` | [Recipe](https://github.com/conan-io/conan-center-index/tree/master/recipes/snappy) | "Snappy" lossless compression algorithm. | | ||
| | `soci` | [Recipe](https://github.com/conan-io/conan-center-index/tree/master/recipes/soci) | Abstraction layer for database access. | |
There was a problem hiding this comment.
I like how you updated this!
There was a problem hiding this comment.
I verified that the following commits do exactly what they claim to do, and can be added to .git-blame-ignore-revs
fa3e44cc07 Consolidate external libraries
95491c0cce Rename .hpp to .h
4de37264d4 Format formerly .hpp files
ccacc94aba Rewrite includes
I would like to know for sure that the removal of packaging files won't hurt the users - I assumed they are used to build .deb and .rpm but I guess was wrong ?
|
@Bronek the packaging files have been moved to a separate, internal (Ripple GitLab) project. They were never used outside of Ripple. They drive our internal package pipelines. As a separate project, they can be developed outside of the rippled review and release process. I've already worked with @legleux to ensure that the new project can successfully package this branch. |
|
Now that this PR has two approvals, I will work on the finishing steps outlined at the end of the PR description. |
thejohnfreeman
force-pushed
the
backport
branch
from
195194b
to
a320c51
Compare
$ find src/ripple/ src/test/ -type f -exec sed -i 's:include\s*["<]ripple/\(.*\)\.h\(pp\)\?[">]:include <ripple/\1.h>:' {} +
thejohnfreeman
force-pushed
the
backport
branch
from
a320c51
to
b84f7e7
Compare
This PR implements half of the changes planned for the physical redesign. These changes, described below, are in files touched infrequently by other contributors, so I expect them to be less disruptive or contentious. (The second half of the changes will come in a later PR that I expect to be more contentious.)
Builds/containers, which I've been helping @legleux over the last couple of weeks exfiltrate to a separate project, to decouple its development cadence from that of rippled. As far as I know, only Ripple uses these files, and only to build and test packages.external/by movingsecp256k1anded25519-donnaout ofsrc/. From now on,src/will contain only first-party sources..has the only extension for first-party headers. This could require an importer to change its include paths, but the only affected headers are part ofbeast/{unit_,}test/, so I don't think any other projects are affected (validator-keys-toolis not).Before this PR is merged, we should:
develop. The commit sequence that predates the review should be preserved. Fixes made since the review opened should be merged into earlier commits..git-blame-ignore-revs.