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

Conversation

raskchanky
Copy link
Contributor

This is the last of the crates in the habitat repo to be converted from
protobuf to prost. Note that unlike the supervisor and
butterfly, I decided against including the generated code in the repo.

It's also worth noting that unlike our approach with the protobuf crate,
we're not actually using the structs generated by prost except at the
edges. Prost rightfully uses Options to represent optional fields and
since all of our protobuf messages have all optional fields, the
ergonomics of using Prost generated structs is sub-optimal. For this
reason, we have our own set of internal structs that mirror the Prost
ones that we use in the launcher and launcher-client crates, and
launcher-protocol is the only crate that knows about the Prost generated
structs.

Closes #6073

Signed-off-by: Josh Black raskchanky@gmail.com

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Copy link
Contributor

@baumanj baumanj left a comment

Choose a reason for hiding this comment

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

Looks good to me, but if you're interested I think we can make it a bit simpler and avoid the need for macros

pub trait LauncherMessage: MessageStatic + Clone + Default + fmt::Debug {
fn to_bytes(&self) -> Result<Vec<u8>>;
fn from_bytes(bytes: &[u8]) -> Result<Self>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an alternate approach that doesn't need macros:

We can consolidate the MessageStatic, and FromProto traits into LauncherMessage, and by adding an associated type to hold the Prost-generated struct, we get:

pub trait LauncherMessage
where
    Self: Clone + fmt::Debug,
{
    type Generated: prost::Message + Default + From<Self>;
    const MESSAGE_ID: &'static str;

    fn from_proto(value: Self::Generated) -> Result<Self>;

    fn from_bytes(bytes: &[u8]) -> Result<Self> {
        let decoded = Self::Generated::decode(bytes)?;
        Self::from_proto(decoded)
    }

    fn to_bytes(&self) -> Result<Vec<u8>> {
        let envelope = Self::Generated::from(self.clone());
        let mut buf = bytes::BytesMut::with_capacity(envelope.encoded_len());
        envelope.encode(&mut buf)?;
        Ok(buf.to_vec())
    }
}

The impl changes follow from that, but here's a full diff if you want to go that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly the solution I was trying to find from the beginning. Thank you so much for this.

This is the last of the crates in the habitat repo to be converted from
protobuf to prost.  Note that unlike the supervisor and
butterfly, I decided against including the generated code in the repo.

It's also worth noting that unlike our approach with the protobuf crate,
we're not actually using the structs generated by prost except at the
edges. Prost rightfully uses Options to represent optional fields and
since all of our protobuf messages have all optional fields, the
ergonomics of using Prost generated structs is sub-optimal. For this
reason, we have our own set of internal structs that mirror the Prost
ones that we use in the launcher and launcher-client crates, and
launcher-protocol is the only crate that knows about the Prost generated
structs.

Signed-off-by: Josh Black <raskchanky@gmail.com>
…sistent

Currently, I can't reproduce either of these locally:
rumor::election::five_members_elect_a_new_leader_when_they_are_quorum_partitioned
rumor::departure::two_members_share_departures

But they are consistently failing on travis

Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

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

Looks good... I had a couple suggestions, but I'm eager to extend this pattern to our other protocol crates.
tenor-174581876

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

}

// 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.

Signed-off-by: Josh Black <raskchanky@gmail.com>
@christophermaier
Copy link
Contributor

@raskchanky Looks great! Once the release is out, let's merge this!

Appveyor timed out (sigh)... so I restarted it.

@raskchanky raskchanky merged commit f15f152 into master Feb 6, 2019
@raskchanky raskchanky deleted the jb/launcher-prost branch February 6, 2019 22:53
christophermaier added a commit that referenced this pull request Feb 13, 2019
With the transition to always-compiled Protobuf code in our launcher
protocol crate (see #6105), we need to add the Protobuf compiler to
our package build process.

I suspect this works unmodified on Windows because the Habitat build
process is necessarily less of a "clean room" affair. The
`prost_build` crate includes a `protoc` binary that it can use, but
this doesn't work in our Linux builds because it's linked to
non-Habitat libraries.

Signed-off-by: Christopher Maier <cmaier@chef.io>
christophermaier added a commit that referenced this pull request Feb 14, 2019
Recent refactoring of the Launcher protocol crate (see #6105) has
unearthed the fact that the `txn_id` field in the Launcher protocol
envelope was never meaningfully used. That refactoring ended up
treating an Envelope without a `txn_id` as invalid, which ended up
breaking compatibility because nothing actually sent an envelope with
a `txn_id` in the first place.

Since `txn_id` was never actually used, it is now removed. The field
name and number are now treated as reserved so they can never be
reused in the future.

Signed-off-by: Christopher Maier <cmaier@chef.io>
@christophermaier christophermaier added Type: Chore Issues for general code and infrastructure maintenance and removed C-chore labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Chore Issues for general code and infrastructure maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert launcher-protocol crate to prost
4 participants