-
Notifications
You must be signed in to change notification settings - Fork 70
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
cmd: move dev/snapshots and common to cli sublibraries #2574
Conversation
canepat
commented
Dec 10, 2024
•
edited by battlmonstr
Loading
edited by battlmonstr
- support "cli" sublibraries in silkworm_library
- reorg cmd/common as infra/cli
- move module-specific options to db/cli, node/cli, rpc/cli and sentry/cli
- move dev/snapshots sources to db/cli (executable is still defined in cmd)
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.
great job 👍 , see comments
ASAN failure is not related to this PR, it was broken on master since #2561 |
69205f5
to
0616349
Compare
fd99b89
to
8d67bea
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2574 +/- ##
==========================================
- Coverage 51.56% 51.20% -0.36%
==========================================
Files 762 771 +9
Lines 51724 52092 +368
Branches 8023 8047 +24
==========================================
+ Hits 26669 26672 +3
- Misses 22750 23121 +371
+ Partials 2305 2299 -6 ☔ View full report in Codecov by Sentry. |
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.
see 1 comment, otherwise LGTM 👍
cmake/common/targets.cmake
Outdated
# cli subdirectories with CMakeLists.txt belong to silkworm_*_cli libraries | ||
if(NOT "${CMAKE_CURRENT_SOURCE_DIR}" MATCHES "/cli$") | ||
list(FILTER SRC EXCLUDE REGEX "\/cli\/") | ||
endif() |
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.
This should be placed right after list_filter_dirs(SRC SUB_LIBS)
line, otherwise TEST_SRC will get "cli/*" sources included. Perhaps we should rename the initial SRC to ALL_SRC, and then set(SRC ${ALL_SRC})
to avoid this confusion. Sorry for this.
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.
I've moved it.
@canepat dev/snapshots.cpp is moved to db/cli, but |