-
Notifications
You must be signed in to change notification settings - Fork 27
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
[3.2] Add extract-blocks option to eosio-blocklog #535
Conversation
@@ -1,14 +1,22 @@ | |||
add_executable( eosio-blocklog main.cpp ) | |||
|
|||
find_package(progressbar 2.0 QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding an (albeit optional) dependency for a progress bar seems.. a bit wasteful. I can't even figure out what library provides this.. it looks like maybe this https://github.com/doches/progressbar but it's lacking progressbar_new_percent_with_format_and_tumbler
and progressbar_update_percent
.
And doing a google or github search for progressbar_new_percent_with_format_and_tumbler
turns up completely empty and searching for ubuntu packages with "progressbar" files in them likewise comes up empty.
Is anyone even going to have this library? Is this better suited as a submodule or copy/pasta job instead? (the one linked above adds a dependency on to ncurses, which is something we'll want to avoid as a hard requirement though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's my version, which I haven't pushed to Github yet. And it's just for me, because I'll be running the thing 261 times on Mainnet and wanted to see what it's doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An optional dependency on a private library doesn't really align with our dependency goals (which is more along the lines of reducing build dependencies; new dependencies should bring a non-trivial amount of functionality).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a public library, now found here. It builds cleanly and silently without it, only using it if it's present and detected by CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be judicious on the number of dependencies we pick up and this doesn't seem like a worthwhile dependency addition. It only adds trivial functionality "just for @jgiszczak" via a niche package that realistically nobody will ever have nor will be tested. I also would like to avoid hard dependencies on ncurses so bundling the code as-is isn't an option either.
and completely eliminate progress reporting from block log extraction.
You can still have a progress indicator 🤷 And since this is an application that isn't run as a daemon it's possible even the "\r trick" is acceptable. |
Is there anything else which needs to be done? I'm finished with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, that you can fix if you agree.
Adds
--extract-blocks
option to theeosio-blocklog
tool, along with an accompanying--output-dir
option. This option extracts the range of blocks specified by the-f
or--first
block option and the-l
or--last
block option and writes a new block log with accompanying index file to the directory specified by the--output-dir
option. At least one of--first
or--last
must be specified.