Skip to content

Commit

Permalink
fix: cleanup unwraps (#327)
Browse files Browse the repository at this point in the history
  • Loading branch information
@swarnimarun
swarnimarun authored Nov 20, 2023
1 parent 83e4f79 commit a40984b
Show file tree
Hide file tree
Showing 13 changed files with 125 additions and 108 deletions.
17 changes: 9 additions & 8 deletions src/linux/env.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,35 +23,36 @@ pub fn default_env_vars(prefix: &Path) -> HashMap<String, String> {
// contains other QEMU env vars.
vars.insert(
"CFLAGS".to_string(),
std::env::var("CFLAGS").unwrap_or("".to_string()),
std::env::var("CFLAGS").unwrap_or_default(),
);
vars.insert(
"CXXFLAGS".to_string(),
std::env::var("CXXFLAGS").unwrap_or("".to_string()),
std::env::var("CXXFLAGS").unwrap_or_default(),
);
vars.insert(
"LDFLAGS".to_string(),
std::env::var("LDFLAGS").unwrap_or("".to_string()),
std::env::var("LDFLAGS").unwrap_or_default(),
);
vars.insert(
"QEMU_LD_PREFIX".to_string(),
std::env::var("QEMU_LD_PREFIX").unwrap_or("".to_string()),
std::env::var("QEMU_LD_PREFIX").unwrap_or_default(),
);
vars.insert(
"QEMU_UNAME".to_string(),
std::env::var("QEMU_UNAME").unwrap_or("".to_string()),
std::env::var("QEMU_UNAME").unwrap_or_default(),
);
vars.insert(
"DEJAGNU".to_string(),
std::env::var("DEJAGNU").unwrap_or("".to_string()),
std::env::var("DEJAGNU").unwrap_or_default(),
);
vars.insert(
"DISPLAY".to_string(),
std::env::var("DISPLAY").unwrap_or("".to_string()),
std::env::var("DISPLAY").unwrap_or_default(),
);
vars.insert(
"LD_RUN_PATH".to_string(),
std::env::var("LD_RUN_PATH").unwrap_or(prefix.join("lib").to_string_lossy().to_string()),
std::env::var("LD_RUN_PATH")
.unwrap_or_else(|_| prefix.join("lib").to_string_lossy().to_string()),
);
vars.insert(
"BUILD".to_string(),
Expand Down
25 changes: 16 additions & 9 deletions src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ use std::{
path::PathBuf,
str::{self, FromStr},
};
use tracing_subscriber::{filter::Directive, fmt, prelude::*, EnvFilter};
use tracing_subscriber::{
filter::{Directive, ParseError},
fmt,
prelude::*,
EnvFilter,
};

use rattler_build::{
build::run_build,
Expand Down Expand Up @@ -146,7 +151,7 @@ async fn main() -> miette::Result<()> {

// Setup tracing subscriber
tracing_subscriber::registry()
.with(get_default_env_filter(args.verbose.log_level_filter()))
.with(get_default_env_filter(args.verbose.log_level_filter()).into_diagnostic()?)
.with(
fmt::layer()
.with_writer(IndicatifWriter::new(multi_progress.clone()))
Expand Down Expand Up @@ -319,7 +324,7 @@ async fn run_build_from_args(args: BuildOpts, multi_progress: MultiProgress) ->
let channels = args
.channel
.clone()
.unwrap_or(vec!["conda-forge".to_string()]);
.unwrap_or_else(|| vec!["conda-forge".to_string()]);

let timestamp = chrono::Utc::now();
let output = rattler_build::metadata::Output {
Expand Down Expand Up @@ -401,16 +406,18 @@ async fn rebuild_from_args(args: RebuildOpts) -> miette::Result<()> {
}

/// Constructs a default [`EnvFilter`] that is used when the user did not specify a custom RUST_LOG.
pub fn get_default_env_filter(verbose: clap_verbosity_flag::LevelFilter) -> EnvFilter {
pub fn get_default_env_filter(
verbose: clap_verbosity_flag::LevelFilter,
) -> Result<EnvFilter, ParseError> {
let mut result = EnvFilter::new("rattler_build=info");

if verbose >= clap_verbosity_flag::LevelFilter::Trace {
result = result.add_directive(Directive::from_str("resolvo=info").unwrap());
result = result.add_directive(Directive::from_str("rattler=info").unwrap());
result = result.add_directive(Directive::from_str("resolvo=info")?);
result = result.add_directive(Directive::from_str("rattler=info")?);
} else {
result = result.add_directive(Directive::from_str("resolvo=warn").unwrap());
result = result.add_directive(Directive::from_str("rattler=warn").unwrap());
result = result.add_directive(Directive::from_str("resolvo=warn")?);
result = result.add_directive(Directive::from_str("rattler=warn")?);
}

result
Ok(result)
}
3 changes: 3 additions & 0 deletions src/packaging.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ use crate::{linux, post};

#[derive(Debug, thiserror::Error)]
pub enum PackagingError {
#[error("Failed to build glob from pattern")]
GlobError(#[from] globset::Error),

#[error("Dependencies are not yet finalized / resolved")]
DependenciesNotFinalized,

Expand Down
12 changes: 4 additions & 8 deletions src/post.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{

use rattler_conda_types::{PackageName, Platform};

use crate::{linux::link::SharedObject, macos::link::Dylib};
use crate::{linux::link::SharedObject, macos::link::Dylib, packaging::PackagingError};

#[derive(Debug, thiserror::Error)]
pub enum RelinkError {
Expand Down Expand Up @@ -90,10 +90,8 @@ pub fn python(
name: &PackageName,
version: &str,
paths: &HashSet<PathBuf>,
) -> Result<(), std::io::Error> {
let metadata_glob = globset::Glob::new("**/*.dist-info/METADATA")
.unwrap()
.compile_matcher();
) -> Result<(), PackagingError> {
let metadata_glob = globset::Glob::new("**/*.dist-info/METADATA")?.compile_matcher();

if let Some(p) = paths.iter().find(|p| metadata_glob.is_match(p)) {
// unwraps are OK because we already globbed
Expand All @@ -114,9 +112,7 @@ pub fn python(
}
}

let glob = globset::Glob::new("**/*.dist-info/INSTALLER")
.unwrap()
.compile_matcher();
let glob = globset::Glob::new("**/*.dist-info/INSTALLER")?.compile_matcher();
for p in paths {
if glob.is_match(p) {
fs::write(p, "conda\n")?;
Expand Down
6 changes: 3 additions & 3 deletions src/render/pin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ impl Pin {
let min_pin = self
.min_pin
.clone()
.unwrap_or(PinExpression("x.x.x.x.x.x".to_string()));
.unwrap_or_else(|| PinExpression("x.x.x.x.x.x".to_string()));
// number of digits in pin expression
let pin_digits = min_pin.0.chars().filter(|c| *c == 'x').count();
if pin_digits == 0 {
Expand All @@ -107,7 +107,7 @@ impl Pin {
let max_pin = self
.max_pin
.clone()
.unwrap_or(PinExpression("x".to_string()));
.unwrap_or_else(|| PinExpression("x".to_string()));

// number of digits in pin expression
let pin_digits = max_pin.0.chars().filter(|c| *c == 'x').count();
Expand All @@ -129,7 +129,7 @@ impl Pin {
// increment last digit
let last = pin
.pop()
.unwrap_or("0".to_string())
.unwrap_or_else(|| "0".to_string())
.parse::<u64>()
.map_err(|_| PinError::CouldNotPin(version_str.clone()))?
+ 1;
Expand Down
99 changes: 45 additions & 54 deletions src/selectors.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{collections::BTreeMap, str::FromStr};

use crate::recipe::jinja::Env;
use crate::{recipe::jinja::Env, variant_config::VariantError};

use minijinja::{value::Value, Environment};
use rattler_conda_types::{Platform, Version, VersionSpec};
Expand Down Expand Up @@ -76,14 +76,27 @@ impl Default for SelectorConfig {
}
}

pub fn eval_selector<S: Into<String>>(selector: S, selector_config: &SelectorConfig) -> bool {
pub fn eval_selector<S: Into<String>>(
selector: S,
selector_config: &SelectorConfig,
) -> Result<bool, VariantError> {
let mut env = Environment::new();

env.add_function("cmp", |a: &Value, spec: &str| {
if let Some(version) = a.as_str() {
// check if version matches spec
let version = Version::from_str(version).unwrap();
let version_spec = VersionSpec::from_str(spec).unwrap();
let version = Version::from_str(version).map_err(|_| {
minijinja::Error::new(
minijinja::ErrorKind::InvalidOperation,
"bad version provided for cmp",
)
})?;
let version_spec = VersionSpec::from_str(spec).map_err(|_| {
minijinja::Error::new(
minijinja::ErrorKind::InvalidOperation,
"bad version spec provided for cmp",
)
})?;
Ok(version_spec.matches(&version))
} else {
// if a is undefined, we are currently searching for all variants and thus return true
Expand All @@ -99,10 +112,10 @@ pub fn eval_selector<S: Into<String>>(selector: S, selector_config: &SelectorCon
.and_then(|selector| selector.strip_suffix(')'))
.expect("Could not strip sel( ... ). Check your brackets.");

let expr = env.compile_expression(selector).unwrap();
let expr = env.compile_expression(selector)?;
let ctx = selector_config.clone().into_context();
let result = expr.eval(ctx).unwrap();
result.is_true()
let result = expr.eval(ctx)?;
Ok(result.is_true())
}

/// Flatten a YAML value, returning a new value with selectors evaluated and removed.
Expand Down Expand Up @@ -153,7 +166,7 @@ pub fn flatten_selectors(
}

if val.is_mapping() {
let only_selectors = val.as_mapping().unwrap().iter().all(|(k, _)| {
let only_selectors = val.as_mapping()?.iter().all(|(k, _)| {
if let YamlValue::String(key) = k {
key.starts_with("sel(")
} else {
Expand All @@ -162,17 +175,17 @@ pub fn flatten_selectors(
});

if only_selectors {
for (k, v) in val.as_mapping_mut().unwrap().iter_mut() {
for (k, v) in val.as_mapping_mut()?.iter_mut() {
if let YamlValue::String(key) = k {
if eval_selector(key, selector_config) {
if eval_selector(key, selector_config).ok()? {
return flatten_selectors(v, selector_config);
}
}
}
return None;
}

for (k, v) in val.as_mapping_mut().unwrap().iter_mut() {
for (k, v) in val.as_mapping_mut()?.iter_mut() {
if let YamlValue::String(key) = k {
if key.starts_with("sel(") {
panic!(
Expand All @@ -188,28 +201,21 @@ pub fn flatten_selectors(

if val.is_sequence() {
let new_val: Vec<YamlValue> = val
.as_sequence_mut()
.unwrap()
.as_sequence_mut()?
.iter_mut()
.filter_map(|el| flatten_selectors(el, selector_config))
.collect();

// This does not yet work for lists of list with selectors (it flattens them)
// This is relevant for zip_keys, which is a list of lists of strings.
if new_val.iter().ne(val.as_sequence().unwrap().iter()) {
if new_val.iter().ne(val.as_sequence()?.iter()) {
// flatten down list of lists
let new_val = new_val
.into_iter()
.flat_map(|el| {
if el.is_sequence() {
el.as_sequence().unwrap().clone()
} else {
vec![el]
}
})
.flat_map(|el| el.as_sequence().cloned().unwrap_or_else(|| vec![el]))
.collect::<Vec<_>>();

return Some(serde_yaml::to_value(new_val).unwrap());
return serde_yaml::to_value(new_val).ok();
}

return Some(val.clone());
Expand Down Expand Up @@ -246,7 +252,7 @@ pub fn flatten_toplevel(
for (k, v) in val.as_mapping_mut().unwrap().iter_mut() {
if let YamlValue::String(key) = k {
if key.starts_with("sel(") {
if eval_selector(key, selector_config) {
if eval_selector(key, selector_config).ok()? {
if let Some(inner_map) = flatten_selectors(v, selector_config) {
for (k, v) in inner_map.as_mapping().unwrap().iter() {
new_val.insert(k.as_str().unwrap().to_string(), v.clone());
Expand Down Expand Up @@ -279,21 +285,15 @@ mod tests {
hash: None,
variant: Default::default(),
};
assert!(eval_selector("sel(unix)", &selector_config));
assert!(!eval_selector("sel(win)", &selector_config));
assert!(!eval_selector("sel(osx)", &selector_config));
assert!(eval_selector("sel(unix and not win)", &selector_config));
assert!(!eval_selector("sel(unix and not linux)", &selector_config));
assert!(eval_selector(
"sel((unix and not osx) or win)",
&selector_config
));
assert!(eval_selector(
"sel((unix and not osx) or win or osx)",
&selector_config
));
assert!(eval_selector("sel(linux and x86_64)", &selector_config));
assert!(!eval_selector("sel(linux and aarch64)", &selector_config));
assert!(eval_selector("sel(unix)", &selector_config).unwrap());
assert!(!eval_selector("sel(win)", &selector_config).unwrap());
assert!(!eval_selector("sel(osx)", &selector_config).unwrap());
assert!(eval_selector("sel(unix and not win)", &selector_config).unwrap());
assert!(!eval_selector("sel(unix and not linux)", &selector_config).unwrap());
assert!(eval_selector("sel((unix and not osx) or win)", &selector_config).unwrap());
assert!(eval_selector("sel((unix and not osx) or win or osx)", &selector_config).unwrap());
assert!(eval_selector("sel(linux and x86_64)", &selector_config).unwrap());
assert!(!eval_selector("sel(linux and aarch64)", &selector_config).unwrap());
}

#[test]
Expand All @@ -307,22 +307,13 @@ mod tests {
variant,
};

assert!(eval_selector("sel(cmp(python, '==3.7'))", &selector_config));
assert!(eval_selector("sel(cmp(python, '>=3.7'))", &selector_config));
assert!(eval_selector(
"sel(cmp(python, '>=3.7,<3.9'))",
&selector_config
));

assert!(!eval_selector(
"sel(cmp(python, '!=3.7'))",
&selector_config
));
assert!(!eval_selector("sel(cmp(python, '<3.7'))", &selector_config));
assert!(!eval_selector(
"sel(cmp(python, '>3.5,<3.7'))",
&selector_config
));
assert!(eval_selector("sel(cmp(python, '==3.7'))", &selector_config).unwrap());
assert!(eval_selector("sel(cmp(python, '>=3.7'))", &selector_config).unwrap());
assert!(eval_selector("sel(cmp(python, '>=3.7,<3.9'))", &selector_config).unwrap());

assert!(!eval_selector("sel(cmp(python, '!=3.7'))", &selector_config).unwrap());
assert!(!eval_selector("sel(cmp(python, '<3.7'))", &selector_config).unwrap());
assert!(!eval_selector("sel(cmp(python, '>3.5,<3.7'))", &selector_config).unwrap());
}

macro_rules! set_snapshot_suffix {
Expand Down
4 changes: 2 additions & 2 deletions src/source/copy_dir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<'a> CopyDir<'a> {

pub fn run(self) -> Result<CopyDirResult<'a>, SourceError> {
// Create the to path because we're going to copy the contents only
create_dir_all(self.to_path).unwrap();
create_dir_all(self.to_path)?;

let (folders, globs) = self
.include_globs
Expand Down Expand Up @@ -169,7 +169,7 @@ impl<'a> CopyDir<'a> {
.unwrap_or(false)
{
// if the dir is empty, check if we should create it anyways
if entry.path().read_dir().unwrap().next().is_some()
if entry.path().read_dir().ok()?.next().is_some()
|| !result.include_globs().is_empty()
{
return None;
Expand Down
Loading

0 comments on commit a40984b

Please sign in to comment.