Make sure Cargo features work #76
Conversation
hug-dev
force-pushed
the
features-fix
branch
2 times, most recently
from
77ac1d0
to
c44a13d
Compare
src/bin/main.rs
Outdated
| @@ -71,7 +78,13 @@ fn main() -> Result<(), Error> { | |||
| config_file = ::std::fs::read_to_string(CONFIG_FILE_PATH) | |||
| .expect("Failed to read configuration file"); | |||
| config = toml::from_str(&config_file).expect("Failed to parse service configuration"); | |||
| front_end_handler = Arc::from(ServiceBuilder::build_service(&config)); | |||
| front_end_handler = Arc::from(match ServiceBuilder::build_service(&config) { | |||
There was a problem hiding this comment.
...build_service(&config).ok_or_else(|| Err(Error::new... )?...
| } | ||
| #[cfg(not(all(feature = "mbed-crypto-provider", feature = "pkcs11-provider")))] | ||
| _ => { | ||
| warn!( |
There was a problem hiding this comment.
I know we're not ready for a release yet, but this sounds like a case where we should panic instead of just printing a warning - if the config file defines a provider but that provider isn't enabled, the admin might be expecting some functionality to be available which will not be there
There was a problem hiding this comment.
we should panic instead of just printing a warning
That is what I did first but having it return an Option allows us to do the following things:
- even if one of the providers could not be instantiated, Parsec can still run with the other ones
- an admin might reload the configuration with a bad one, adding a provider that is not in the binary: in that case, it will print a warning without the whole service shutting down
- I added a feature in this commit that checks if no providers could be instantiated in which case the Parsec binary will exit with an error code: I think that is still a better way to exit than panicking which can be confusing.
IMO returning always seems cleaner than panicking when it is possible.
the admin might be expecting some functionality to be available which will not be there
You have a point there: I think it is better to solve this with the admin reading the logs and executing tests after Parsec has been installed than just panicking. I guess both options are possible and we will have to make a decision about this.
There was a problem hiding this comment.
I'm ok with this going in like this, we'll have to see if we get any feedback about it
Makes sure that Parsec builds with commands like: cargo build --no-default-features --features "mbed-crypto-provider" cargo build --no-default-features --features "tpm-provider" Also renames the "mbed" features to "mbed-crypto-provider" for consistency. Signed-off-by: Hugues de Valon <[email protected]>
Makes sure that Parsec builds with commands like:
Also renames the "mbed" features to "mbed-crypto-provider" for
consistency.
Exit the service with an error code if no provider could be instanciated.