Skip to content
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

Convert all of the launcher crates to use prost. #6105

Merged
merged 3 commits into from
Feb 6, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ $(foreach component,$(ALL),$(eval $(call BUILD,$(component))))

define UNIT
unit-$1: image ## executes the $1 component's unit test suite
$(run) sh -c 'cd components/$1 && TESTING_FS_ROOT=$(TESTING_FS_ROOT) cargo test $(CARGO_FLAGS)'
$(run) sh -c 'cd components/$1 && TESTING_FS_ROOT=$(TESTING_FS_ROOT) cargo test $(CARGO_FLAGS) -- --test-threads=1'
.PHONY: unit-$1
endef
$(foreach component,$(ALL),$(eval $(call UNIT,$(component))))
Expand All @@ -203,8 +203,7 @@ UNEXAMINED_LINTS = clippy::cyclomatic_complexity \
clippy::redundant_field_names \
clippy::too_many_arguments \
clippy::trivially_copy_pass_by_ref \
clippy::wrong_self_convention \
renamed_and_removed_lints
clippy::wrong_self_convention

# Lints we disagree with and choose to keep in our code with no warning
ALLOWED_LINTS =
Expand Down Expand Up @@ -260,7 +259,8 @@ DENIED_LINTS = clippy::assign_op_pattern \
clippy::useless_format \
clippy::useless_let_if_seq \
clippy::useless_vec \
clippy::write_with_newline
clippy::write_with_newline \
renamed_and_removed_lints

lint: image ## executes the $1 component's linter checks
$(run) cargo clippy --all-targets --tests $(CARGO_FLAGS) -- \
Expand Down
3 changes: 2 additions & 1 deletion components/launcher-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name = "habitat-launcher-client"
version = "0.0.0"
edition = "2018"
authors = ["Jamie Winsor <reset@habitat.sh>"]
workspace = "../../"

[dependencies]
bincode = "*"
Expand All @@ -12,5 +13,5 @@ habitat-launcher-protocol = { path = "../launcher-protocol" }
ipc-channel = { git = "https://github.com/habitat-sh/ipc-channel", branch = "hbt-windows" }
libc = "*"
log = "*"
protobuf = "1.5.1"
prost = "*"
serde = "*"
77 changes: 36 additions & 41 deletions components/launcher-client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::collections::HashMap;
use std::fs;
use std::io;
use std::path::Path;

use crate::core::os::process::Pid;
use crate::protocol;
use crate::{
core::os::process::Pid,
error::{Error, Result},
protocol::{self, Error as ProtocolError},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's minor, but I've found it useful to have things under crate:: to actually be defined in the current crate. We end up doing this because we've got this over in lib.rs

use habitat_core as core;
use habitat_launcher_protocol as protocol;

Thus, core and protocol are actually symbols "in our crate".

For something like this, I like to do:

use crate::error::{Error, Result};
use habitat_core::os::process::Pid;
use habitat_launcher_protocol::{self as protocol, Error as ProtocolError};

(and often, that self as protocol can be refactored away with a bit more typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, will update

use ipc_channel::ipc::{IpcOneShotServer, IpcReceiver, IpcSender};
use protobuf;

use crate::error::{Error, Result};
use std::{collections::HashMap, fs, io, path::Path};

type Env = HashMap<String, String>;
type IpcServer = IpcOneShotServer<Vec<u8>>;
Expand All @@ -49,38 +45,36 @@ impl LauncherCli {
let tx = IpcSender::connect(pipe_to_launcher).map_err(Error::Connect)?;
let (ipc_srv, pipe_to_sup) = IpcServer::new().map_err(Error::BadPipe)?;
debug!("IpcServer::new() returned pipe_to_sup: {}", pipe_to_sup);
let mut cmd = protocol::Register::new();
cmd.set_pipe(pipe_to_sup);
let mut cmd = protocol::Register::default();
cmd.pipe = pipe_to_sup.clone();
Self::send(&tx, &cmd)?;
let (rx, raw) = ipc_srv.accept().map_err(|_| Error::AcceptConn)?;
Self::read::<protocol::NetOk>(&raw)?;
Ok(LauncherCli {
tx: tx,
rx: rx,
pipe: cmd.take_pipe(),
pipe: pipe_to_sup,
})
}

/// Read a launcher protocol message from a byte array
fn read<T>(bytes: &[u8]) -> Result<T>
where
T: protobuf::MessageStatic,
T: protocol::LauncherMessage,
{
let txn = protocol::NetTxn::from_bytes(bytes).map_err(Error::Deserialize)?;
let txn = protocol::NetTxn::from_bytes(bytes)?;
if txn.message_id() == "NetErr" {
let err = txn
.decode::<protocol::NetErr>()
.map_err(Error::Deserialize)?;
return Err(Error::Protocol(err));
let err = txn.decode::<protocol::NetErr>()?;
return Err(Error::Protocol(ProtocolError::NetErr(err)));
}
let msg = txn.decode::<T>().map_err(Error::Deserialize)?;
let msg = txn.decode::<T>()?;
Ok(msg)
}

/// Receive and read protocol message from an IpcReceiver
fn recv<T>(rx: &IpcReceiver<Vec<u8>>) -> Result<T>
where
T: protobuf::MessageStatic,
T: protocol::LauncherMessage,
{
match rx.recv() {
Ok(bytes) => Self::read(&bytes),
Expand All @@ -91,18 +85,18 @@ impl LauncherCli {
/// Send a command to a Launcher
fn send<T>(tx: &IpcSender<Vec<u8>>, message: &T) -> Result<()>
where
T: protobuf::MessageStatic,
T: protocol::LauncherMessage,
{
let txn = protocol::NetTxn::build(message).map_err(Error::Serialize)?;
let bytes = txn.to_bytes().map_err(Error::Serialize)?;
let txn = protocol::NetTxn::build(message)?;
let bytes = txn.to_bytes()?;
tx.send(bytes).map_err(Error::Send)?;
Ok(())
}

/// Receive and read protocol message from an IpcReceiver
fn try_recv<T>(rx: &IpcReceiver<Vec<u8>>) -> Result<Option<T>>
where
T: protobuf::MessageStatic,
T: protocol::LauncherMessage,
{
match rx.try_recv().map_err(|err| Error::from(*err)) {
Ok(bytes) => {
Expand All @@ -124,11 +118,11 @@ impl LauncherCli {

/// Restart a running process with the same arguments
pub fn restart(&self, pid: Pid) -> Result<Pid> {
let mut msg = protocol::Restart::new();
msg.set_pid(pid.into());
let mut msg = protocol::Restart::default();
msg.pid = pid.into();
Self::send(&self.tx, &msg)?;
let reply = Self::recv::<protocol::SpawnOk>(&self.rx)?;
Ok(reply.get_pid() as Pid)
Ok(reply.pid as Pid)
}

/// Send a process spawn command to the connected Launcher
Expand All @@ -154,8 +148,8 @@ impl LauncherCli {
G: ToString,
P: ToString,
{
let mut msg = protocol::Spawn::new();
msg.set_binary(bin.as_ref().to_path_buf().to_string_lossy().into_owned());
let mut msg = protocol::Spawn::default();
msg.binary = bin.as_ref().to_path_buf().to_string_lossy().into_owned();

// On Windows, we only expect user to be Some.
//
Expand All @@ -164,34 +158,35 @@ impl LauncherCli {
// used; names are only for backward compatibility with older
// Launchers.
if let Some(name) = user {
msg.set_svc_user(name.to_string());
msg.svc_user = Some(name.to_string());
}
if let Some(name) = group {
msg.set_svc_group(name.to_string());
msg.svc_group = Some(name.to_string());
}
if let Some(uid) = user_id {
msg.set_svc_user_id(uid);
msg.svc_user_id = Some(uid);
}
if let Some(gid) = group_id {
msg.set_svc_group_id(gid);
msg.svc_group_id = Some(gid);
}

// This is only for Windows
if let Some(password) = password {
msg.set_svc_password(password.to_string());
msg.svc_password = Some(password.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could streamline these a bit with this:

msg.svc_user = user.map(|u| u.to_string());
msg.svc_group = group.map(|g| g.to_string());
msg.svc_user_id = user_id;
msg.svc_group_id = group_id;

msg.svc_password = password.map(|p| p.to_string());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome, will do.

Copy link
Contributor

@christophermaier christophermaier Feb 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raskchanky Actually, if you wanna get really wild, try this:

let msg = protocol::Spawn {
    binary: bin.as_ref().to_path_buf().to_string_lossy().into_owned(),
    svc_user: user.map(|u| u.to_string()),
    svc_group: group.map(|g| g.to_string()),
    svc_user_id: user_id,
    svc_group_id: group_id,
    svc_password: password.map(|p| p.to_string()),
    env: env,
    id: id.to_string(),
    ..Default::default()
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern could be used on all these messages, actually. Kinda nice to reduce the mutability of everything to the bare minimum.

}
msg.set_env(env);
msg.set_id(id.to_string());

msg.env = env;
msg.id = id.to_string();
Self::send(&self.tx, &msg)?;
let reply = Self::recv::<protocol::SpawnOk>(&self.rx)?;
Ok(reply.get_pid() as Pid)
Ok(reply.pid as Pid)
}

pub fn terminate(&self, pid: Pid) -> Result<i32> {
let mut msg = protocol::Terminate::new();
msg.set_pid(pid.into());
let mut msg = protocol::Terminate::default();
msg.pid = pid.into();
Self::send(&self.tx, &msg)?;
let reply = Self::recv::<protocol::TerminateOk>(&self.rx)?;
Ok(reply.get_exit_code())
Ok(reply.exit_code)
}
}
23 changes: 8 additions & 15 deletions components/launcher-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,19 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::error;
use std::fmt;
use std::io;
use std::result;

use crate::protocol;
use ipc_channel;
use protobuf;
use std::{error, fmt, io, result};

#[derive(Debug)]
pub enum Error {
AcceptConn,
BadPipe(io::Error),
Connect(io::Error),
Deserialize(protobuf::ProtobufError),
IPCBincode(String),
IPCIO(io::ErrorKind),
Protocol(protocol::NetErr),
Protocol(protocol::Error),
Send(ipc_channel::Error),
Serialize(protobuf::ProtobufError),
}

pub type Result<T> = result::Result<T, Error>;
Expand All @@ -42,16 +35,12 @@ impl fmt::Display for Error {
Error::AcceptConn => "Unable to accept connection from Launcher".to_string(),
Error::BadPipe(ref e) => format!("Unable to open pipe to Launcher, {}", e),
Error::Connect(ref e) => format!("Unable to connect to Launcher's pipe, {}", e),
Error::Deserialize(ref e) => {
format!("Unable to deserialize message from Launcher, {}", e)
}
Error::IPCBincode(ref e) => {
format!("Unable to read message frame from Launcher, {}", e)
}
Error::IPCIO(ref e) => format!("Unable to receive message from Launcher, {:?}", e),
Error::Protocol(ref e) => format!("{}", e),
Error::Send(ref e) => format!("Unable to send to Launcher's pipe, {}", e),
Error::Serialize(ref e) => format!("Unable to serialize message to Launcher, {}", e),
};
write!(f, "{}", msg)
}
Expand All @@ -63,12 +52,10 @@ impl error::Error for Error {
Error::AcceptConn => "Unable to accept connection from Launcher",
Error::BadPipe(_) => "Unable to open pipe to Launcher",
Error::Connect(_) => "Unable to connect to Launcher's pipe",
Error::Deserialize(_) => "Unable to deserialize message from Launcher",
Error::IPCBincode(_) => "Unable to encode/decode message framing to/from Launcher",
Error::IPCIO(_) => "Unable to receive message from Launcher",
Error::Protocol(_) => "Received an error from Launcher",
Error::Send(_) => "Unable to send to Launcher's pipe",
Error::Serialize(_) => "Unable to serialize message to Launcher",
}
}
}
Expand All @@ -81,3 +68,9 @@ impl From<ipc_channel::ErrorKind> for Error {
}
}
}

impl From<protocol::Error> for Error {
fn from(err: protocol::Error) -> Error {
Error::Protocol(err)
}
}
10 changes: 5 additions & 5 deletions components/launcher-protocol/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ version = "0.0.0"
edition = "2018"
authors = ["Jamie Winsor <reset@habitat.sh>"]
build = "build.rs"
workspace = "../../"

[dependencies]
protobuf = "1.5.1"
bytes = "*"
prost = "*"
prost-derive = "*"
serde = "*"
serde_derive = "*"

[build-dependencies]
pkg-config = "*"

[features]
protocols = []
prost-build = "*"
23 changes: 20 additions & 3 deletions components/launcher-protocol/build.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,23 @@
// Inline common build protocols behavior
include!("../libbuild-protocols.rs");
use prost_build;

/// Automatically generate Rust code from our protobuf definitions at
/// compile time.
///
/// Generated code is deposited in `OUT_DIR` and automatically
/// `include!`-ed in our Rust modules, per standard Prost practice.

fn main() {
protocols::generate_if_feature_enabled();
let mut config = prost_build::Config::new();
config.type_attribute(".", "#[derive(Serialize, Deserialize)]");
config
.compile_protos(
&[
"protocols/error.proto",
"protocols/launcher.proto",
"protocols/net.proto",
"protocols/supervisor.proto",
],
&["protocols/"],
)
.unwrap()
}
2 changes: 1 addition & 1 deletion components/launcher-protocol/protocols/error.proto
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
syntax = "proto2";

package error;
package launcher.error;

enum ErrCode {
Unknown = 0;
Expand Down
2 changes: 1 addition & 1 deletion components/launcher-protocol/protocols/launcher.proto
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
syntax = "proto2";

package launcher;
package launcher.launcher;

message Register {
optional string pipe = 1;
Expand Down
2 changes: 1 addition & 1 deletion components/launcher-protocol/protocols/net.proto
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
syntax = "proto2";

package net;
package launcher.net;

message Envelope {
optional string message_id = 1;
Expand Down
2 changes: 1 addition & 1 deletion components/launcher-protocol/protocols/supervisor.proto
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
syntax = "proto2";

package supervisor;
package launcher.supervisor;

message Shutdown {}
Loading