-
Notifications
You must be signed in to change notification settings - Fork 214
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
Simplify dependencies to Linera binaries #1107
Conversation
.await | ||
.unwrap(); | ||
assert!( | ||
util::resolve_binary("linera-spaceship", env!("CARGO_PKG_NAME")) |
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.
🤩 🚀
assert!( | ||
util::resolve_binary("linera-spaceship", env!("CARGO_PKG_NAME")) | ||
.await | ||
.is_err() |
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.
😞
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.
lol
linera-service/src/util.rs
Outdated
{ | ||
features.push("scylladb"); | ||
let components = current_binary_path |
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 is a little harder to read, consider: https://doc.rust-lang.org/std/path/struct.Path.html#method.ends_with
if path.ends_with(Path::new("target/debug/deps")) || path.ends_with(Path::new("target/release/deps")) {
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.
nice
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.
Actually Path::new
is not even needed
.join(name) | ||
.canonicalize() | ||
.unwrap() | ||
.map_err(|e| { |
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.
You can just use with_context
here (since formatting is expensive and you want to do it lazily). You don't need the map_err
as the error will be shown anyway:
Error: Failed to execute and retrieve version from the binary {name} in directory {}
Caused by:
{e}
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.
with_context
seems to only work if you have an anyhow::Result
already.
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.
The documentation says the error type just has to be StdError + Send + Sync + 'static
.
.trim() | ||
.split(' ') | ||
.last() | ||
.ok_or_else(|| { |
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.
Same here I think?
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.
Nothing blocking, just a few suggestions. LGTM :)
…ing binary .. but does check version numbers
Motivation
We have three ways to obtain Linera binaries at the moment: release mode (look up in CARGO_HOME), debug mode (cargo build), test-runner (look up in CARGO_HOME + cargo install if needed).
This is rather complicated and create some subtle limitations:
cargo install
from the network would break CI.Proposal
In the long term,
cargo
should eventually provide a way to depend on the binaries of another crate (rust-lang/cargo#9096). In the meantime, we only have partial solutions. At least we're going for a simple one here (removing lots of code such as the feature detection).resolve_binary
function what all the cases.target/{debug,release}/deps
, it will look at the directory above.There will be two
cargo install
documented for users (test runner and linera-service).For tests,
cargo test
always build the binaries of the current crate(s). So, the only potential remaining issue is when a test depends on the binary of another crate. This is rare as most e2e tests are insidelinera-service
. In CI, we run cargo build anyway first.StructOpt
(or at least recognize--version
in the same way)--version
inschema_export
Test Plan
CI
Release Plan
cargo install linera-sdk
explicitlyLinks
stacked on top of #1106