make mining_device into submodule of integration_tests_sv2 crate#355
Conversation
4ceaf22 to
1c2b77a
Compare
xyephy
left a comment
There was a problem hiding this comment.
cargo publish --dry-run still fails after this change:
error: all dependencies must have a version specified when publishing.
dependency `key-utils` does not specify a version
These git deps need version specifiers:
key-utils = { version = "1.2.0", git = "https://github.com/stratum-mining/stratum", rev = "v1.5.0" }
stratum-common = { version = "4.0.1", git = "https://github.com/stratum-mining/stratum", rev = "v1.5.0", features = ["with_network_helpers"] }Both versions match the v1.5.0 tag and exist on crates.io.
integration-tests/Cargo.toml
Outdated
| key-utils = { git = "https://github.com/stratum-mining/stratum", rev = "v1.5.0" } | ||
| stratum-common = { git = "https://github.com/stratum-mining/stratum", rev = "v1.5.0", features = ["with_network_helpers"] } |
There was a problem hiding this comment.
Why can't we get these two from the (already imported) stratum-apps crate?
There was a problem hiding this comment.
yeah, this aligns with @xyephy comment above
I was able to remove key-utils and use stratum-apps for that piece
unfortunately though, stratum-common is the part that is still not covered by stratum-apps today.... the newly embedded mining_device module still relies on stratum_common::network_helpers_sv2::noise_connection::Connection plus a fair amount of stratum_common::roles_logic_sv2::{...} surface, and stratum-apps does not currently re-export that.
I think the paths ahead are:
- keep
stratum-commonas an explicit dependency for now (current state of this PR); - extend
stratum-appsto re-exportstratum-common/ the relevant facade, which would let us avoid declaring it explicitly here; - refactor the migrated
mining_devicemodule so it no longer depends onstratum-commonat all.
this PR is currently following 1 because it is the lowest hanging fruit, but happy to pivot towards 2 or 3 (although scope is a bit bigger)
There was a problem hiding this comment.
wait, just found some compilation errors
maybe the analysis above is incomplete
There was a problem hiding this comment.
ok ended up pivoting
keeping stratum-common as an explicit dep was not enough, because it still pulled the older published stack and cargo publish --dry-run failed while verifying it (framing_sv2 v5.0.1).
so I went with the stronger version of your suggestion:
key-utilsnow comes fromstratum-appsmining_devicewas refactored to stop depending onstratum-commonentirely- it now uses the
stratum-apps/stratum_corestack directly
with that change, cargo publish --dry-run passes
ec10697 to
7b6ea98
Compare
thanks, should be addressed |
7b6ea98 to
0365d72
Compare
0365d72 to
52f5265
Compare
7b6ea98 to
d56ac1a
Compare
d56ac1a to
7f3af6d
Compare
this is one potential (and opinionated) approach to close #354
one alternative is to start publishing
mining_deviceto crates.io