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

chore: fix rust clippy warnings #432

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 36 additions & 0 deletions build/common/rust/cargo_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,40 @@ def __init__(self):
self.log_level: int


def run_clippy_static_checking(args: ArgumentInfo):
cmd = [
"cargo",
"clippy",
"--target-dir",
args.target_path,
"--target",
args.target,
]

if args.manifest_path != "":
cmd.append(
"--manifest-path",
)
cmd.append(
args.manifest_path,
)

if args.build_type == "release":
cmd.append("--release")

cmd += [
"--",
"-D",
"warnings",
]

returncode, logs = cmd_exec.run_cmd(cmd, args.log_level)
if returncode:
raise Exception(f"Failed to cargo clippy rust tests: {logs}")
else:
print(logs)


if __name__ == "__main__":
parser = argparse.ArgumentParser()

Expand Down Expand Up @@ -51,6 +85,8 @@ def __init__(self):
try:
os.chdir(args.project_path)

# run_clippy_static_checking(args)

cmd = [
"cargo",
"build",
Expand Down
29 changes: 29 additions & 0 deletions build/common/rust/cargo_build_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,33 @@ def copy_test_binaries(
)


def run_clippy_static_checking(args: ArgumentInfo):
cmd = [
"cargo",
"clippy",
"--target-dir",
args.target_path,
"--target",
args.target,
"--tests",
]

if args.build_type == "release":
cmd.append("--release")

cmd += [
"--",
"-D",
"warnings",
]

returncode, logs = cmd_exec.run_cmd(cmd, args.log_level)
if returncode:
raise Exception(f"Failed to cargo clippy rust tests: {logs}")
else:
print(logs)


if __name__ == "__main__":
parser = argparse.ArgumentParser()

Expand Down Expand Up @@ -127,6 +154,8 @@ def copy_test_binaries(
try:
os.chdir(args.project_path)

# run_clippy_static_checking(args)

# cargo build --tests: only compile the test source files, without
# running the test cases.
# cargo test: compile the test source files and run the test cases.
Expand Down
73 changes: 0 additions & 73 deletions core/src/ten_manager/src/designer/messages/compatible.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1020,79 +1020,6 @@ mod tests {
// assert_eq!(compatibles.data, expected_compatibles);
}

// TODO(Liu): Enable this when ManifestPropertyAttributes supports
// properties/items.
// #[actix_web::test]
async fn test_get_compatible_messages_data_required_superset() {
let mut designer_state = DesignerState {
base_dir: None,
all_pkgs: None,
tman_config: TmanConfig::default(),
};

// The first item is 'manifest.json', and the second item is
// 'property.json'.
let all_pkgs_json = vec![
(
include_str!("test_data_embed/app_manifest.json").to_string(),
include_str!("test_data_embed/app_property.json").to_string(),
),
(
include_str!("test_data_embed/extension_addon_1_manifest.json")
.to_string(),
"{}".to_string(),
),
(
include_str!("test_data_embed/extension_addon_2_manifest.json")
.to_string(),
"{}".to_string(),
),
];

let inject_ret =
inject_all_pkgs_for_mock(&mut designer_state, all_pkgs_json);
assert!(inject_ret.is_ok());

let designer_state = Arc::new(RwLock::new(designer_state));

let app = test::init_service(
App::new().app_data(web::Data::new(designer_state)).route(
"/api/designer/v1/messages/compatible",
web::post().to(get_compatible_messages),
),
)
.await;

// Define input data.
let input_data = json!({
"app": "localhost",
"graph": "default",
"extension_group": "extension_group_1",
"extension": "extension_1",
"msg_type": "data",
"msg_direction": "out",
"msg_name": "data2"
});

// Send request to the test server.
let req = test::TestRequest::post()
.uri("/api/designer/v1/messages/compatible")
.set_json(&input_data)
.to_request();

// Call the service and get the response.
let resp = test::call_service(&app, req).await;
assert!(resp.status().is_success());

let body = test::read_body(resp).await;
let body_str = std::str::from_utf8(&body).unwrap();

let compatibles: ApiResponse<Vec<DesignerCompatibleMsg>> =
serde_json::from_str(body_str).unwrap();

assert!(compatibles.data.is_empty());
}

#[actix_web::test]
async fn test_get_compatible_messages_video_target_has_property() {
let mut designer_state = DesignerState {
Expand Down
22 changes: 13 additions & 9 deletions core/src/ten_manager/src/solver/introducer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@ use crate::dep_and_candidate::get_pkg_info_from_candidates;
pub fn extract_introducer_relations_from_raw_solver_results(
results: &[String],
all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
) -> Result<HashMap<PkgInfo, (String, Option<PkgInfo>)>> {
) -> Result<HashMap<PkgBasicInfo, (String, Option<PkgInfo>)>> {
let re = Regex::new(
r#"introducer\("([^"]+)","([^"]+)","([^"]+)","([^"]+)","([^"]*)","([^"]*)"\)"#,
)
.unwrap();

let mut introducer_relations: HashMap<PkgInfo, (String, Option<PkgInfo>)> =
HashMap::new();
let mut introducer_relations: HashMap<
PkgBasicInfo,
(String, Option<PkgInfo>),
> = HashMap::new();

for result in results.iter() {
if let Some(caps) = re.captures(result) {
Expand Down Expand Up @@ -80,8 +82,10 @@ pub fn extract_introducer_relations_from_raw_solver_results(
Some(introducer_info.clone())
};

introducer_relations
.insert(pkg_info.clone(), (requested_version_str, introducer));
introducer_relations.insert(
(&pkg_info).into(),
(requested_version_str, introducer),
);
}
}

Expand All @@ -91,11 +95,11 @@ pub fn extract_introducer_relations_from_raw_solver_results(
pub fn get_dependency_chain(
introducer: &PkgInfo,
conflict_pkg_identity: &PkgTypeAndName,
introducer_relations: &HashMap<PkgInfo, (String, Option<PkgInfo>)>,
introducer_relations: &HashMap<PkgBasicInfo, (String, Option<PkgInfo>)>,
) -> Vec<(String, PkgTypeAndName)> {
let mut chain = Vec::new();
let mut current_pkg = introducer.clone();
let mut visited = HashSet::new();
let mut visited = HashSet::<PkgBasicInfo>::new();

// Add the leaf node (i.e., `introducer` depends on `pkg_name`) to the
// dependency chain first.
Expand All @@ -115,12 +119,12 @@ pub fn get_dependency_chain(
));

loop {
if !visited.insert(current_pkg.clone()) {
if !visited.insert((&current_pkg).into()) {
// Detected a cycle, break to prevent infinite loop.
break;
}

match introducer_relations.get(&current_pkg) {
match introducer_relations.get(&(&current_pkg).into()) {
Some((requested_version, Some(introducer_pkg))) => {
// The dependency chain is that `introducer_pkg` depends on
// `current_pkg`@`requested_version`, i.e., the
Expand Down
2 changes: 1 addition & 1 deletion core/src/ten_manager/src/solver/solver_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ fn print_dependency_chain(

pub fn print_conflict_info(
conflict_info: &ConflictInfo,
introducer_relations: &HashMap<PkgInfo, (String, Option<PkgInfo>)>,
introducer_relations: &HashMap<PkgBasicInfo, (String, Option<PkgInfo>)>,
all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
) -> Result<()> {
println!(
Expand Down
9 changes: 9 additions & 0 deletions core/src/ten_manager/src/solver/solver_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,16 @@ pub fn filter_solver_results_by_type_and_name<'a>(
let mut filtered_results: Vec<&PkgInfo> = vec![];

for result in solver_results.iter() {
// After Rust 1.82, there is an `is_none_or` method, and Clippy will
// suggest using `is_none_or` for simplicity. However, since Ten
// currently needs to rely on Rust 1.81 on macOS (mainly because debug
// mode on Mac enables ASAN, and rustc must match the versions of
// LLVM/Clang to prevent ASAN linking errors), we disable this specific
// Clippy warning here.
#[allow(clippy::unnecessary_map_or)]
let matches_type = pkg_type.map_or(true, |pt| result.pkg_type == *pt);

#[allow(clippy::unnecessary_map_or)]
let matches_name = name.map_or(true, |n| result.name == *n);

let matches = matches_type && matches_name;
Expand Down
Loading