Add missing includes #2338
Add missing includes #2338
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20180120 - 12:37:09 Test Results
|
MarkusTeufelberger
changed the title
|
There are more includes missing, I'll add them as I find them. |
The whole header wasn't included, though maybe <vector> should be included here too.
|
Sorry to jump in on this...
But I joined ripple to learn Blockchain and distributed computing like a
peer-to-peer network.
Does this project involve all that including learning things like the CAP
theorem etc.
…On Sat, Jan 20, 2018 at 6:08 PM, MarkusTeufelberger < ***@***.***> wrote:
#2339 <#2339> seems to be dead
conde anyways, #2340 <#2340> is
now resolved.
I'm not so sure about SystemStats.cpp, it seems that it is built a bit
weirdly by beast (a hardcoded unity file etc.).
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2338 (comment)>, or mute
the thread
<https:/notifications/unsubscribe-auth/AaHL9QD934yCkvd69AtCZC1eqOrXoDozks5tMd5UgaJpZM4RkKRC>
.
|
|
Ask your colleagues at Ripple then. How did you even get through interviews?! O_o |
|
Hi @MarkusTeufelberger , are these change still relevant? Can you share details of the build you were doing that required the additional includes? |
|
I started building rippled with bazel (https://bazel.build) and one of the features there is that compilation units are sandboxed from each other. This means that files must actually include the headers etc. that they are using, otherwise the build would fail (since the missing file wouldn't be available in the sandbox). Unity style builds kinda break that assumption and have other downsides too, so I focused on standard builds for now. Still a handful of files couldn't be compiled, because there were some includes missing, this PR is an attempt to fix this. There are still other weirdnesses in the build process (the way defines for libsecp256k1 are hardcoded in https:/ripple/rippled/blob/develop/src/ripple/unity/secp256k1.cpp for example or the whole mess that is SystemStats.cpp), but not being able to build some files because they didn't include things they use was a show stopper for now. Feel free to take a look and check if you are actually including the headers/libraries you are using in each compilation unit, if you are interested in a more automated approach or some arguments why this is a desirable property for your code, you can also check out https://include-what-you-use.org. I didn't yet move forward a lot with this due to time constraints and also since you recently switched to Boost 1.66. Also #2158 also added a few missing headers and wasn't merged yet, so I didn't want to start a whole project around this like with https:/MarkusTeufelberger/rippled-distrotest |
|
Very cool you are trying bazel! As you note, there are definitely impediments to getting there now. I'm happy to help get this type of fix into a release, but unless we commit to policing it, there is no guarantee we won't break it in the future. I don't see us committing to that soon. If you want to rebase after the 0.90.0 boost changes get out, we can try to push this in. Thanks for your continued interested in improving rippled and its development. |
|
@bachase I'm already working on a pull request to get this introduced into our code base. It should be ready today or Monday. |
|
@scottschurr Awesome! |
|
By "this" I only mean the missing includes. |
|
@MarkusTeufelberger I'm closing this pull request since I believe all of the proposed changes are now in this branch: #2368. They are on a branch I submitted so they run through our continuous integration suite. The commit on that branch, I believe, retains appropriate attribution to you. Please verify that #2368 comprises all of the requested changes. Thank you. And thanks for the contribution! |
No description provided.