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.

Sign up for GitHub

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

refactor: move compiler args from runner to compiler #121

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor: move compiler args from runner to compiler #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9795f4
refactor: move compiler args from runner to compiler
AndreCostaaa Sep 26, 2024
3d5e45a
feat: add absolute path utility fn
AndreCostaaa Sep 26, 2024
b027059
docs: add get full path comments
AndreCostaaa Sep 26, 2024
e36403f
test: create compiler tests
AndreCostaaa Sep 26, 2024
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
12 changes: 1 addition & 11 deletions src/core/compiler/compile_runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,7 @@ impl CompileRunner {
// added to the output_path before calling this function
pub fn new(compiler: &Compiler, exo: &Exo, output_path: &std::path::PathBuf) -> Option<Self> {
let cmd = compiler.cmd();
let mut args = compiler.args(&exo.files);
match output_path.to_str() {
Some(path) => {
// TODO this should probably somewhere else like `compiler` because this is
// specific to gcc/g++
args.push(String::from("-fdiagnostics-color=always"));
args.push(String::from("-o"));
args.push(String::from(path));
}
None => return None,
}
let args = compiler.args(&exo.files, output_path);
Some(Self {
runner: Runner::new(String::from(cmd), args),
})
Expand Down
165 changes: 158 additions & 7 deletions src/core/compiler/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::core::file_utils::file_utils::get_full_path;
use crate::core::file_utils::file_utils::get_absolute_path;

#[derive(Debug)]
pub enum Compiler {
Expand All @@ -16,15 +16,33 @@ impl Compiler {
}

/// Gets the correct arguments to launch the compiler
/// TODO maybe this should also be responsible for adding -o in gcc/g++
/// Would make it easier to add new compilers without changing `compile_runner`
pub fn args(&self, files: &Vec<std::path::PathBuf>) -> Vec<String> {
pub fn args(
&self,
files: &Vec<std::path::PathBuf>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we agree that these paths must be like introduction/hello-world/main.c (relative to course folder), because taking the absolute path based on current folder (project folder) would not point to real files otherwise (if we took just main.c, the path would be invalid) ?

Maybe adding a note about this would help future dev.

output_path: &std::path::PathBuf,
) -> Vec<String> {
match self {
Compiler::Gcc => Compiler::collect_files_with_extension(files, &["c"]),
Compiler::Gxx => Compiler::collect_files_with_extension(files, &["c", "cpp", "cc"]),
Compiler::Gcc => Compiler::gxx_args(files, output_path, &["c"]),
Compiler::Gxx => Compiler::gxx_args(files, output_path, &["c", "cpp", "cc"]),
}
}

/// Gcc/G++ args generator
fn gxx_args(
files: &Vec<std::path::PathBuf>,
output_path: &std::path::PathBuf,
extensions: &[&str],
) -> Vec<String> {
let path = output_path.to_str().unwrap_or("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe unwrap_or_default would be nicer IMO ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid creating yet another path variable that is not clear which kind of path, I would prefer to inline this in the String::from(). What do you think ?

let mut args = Compiler::collect_files_with_extension(files, extensions);
args.extend([
String::from("-fdiagnostics-color=always"),
String::from("-o"),
String::from(path),
]);
return args;
}

/// Collects the files in `files` that have an extension found in `allowed_extensions`
fn collect_files_with_extension(
files: &Vec<std::path::PathBuf>,
Expand All @@ -34,7 +52,7 @@ impl Compiler {
.iter()
.filter_map(|file| {
if Compiler::has_valid_extension(file, allowed_extensions) {
return get_full_path(file).ok();
return get_absolute_path(file).ok();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure to see why we need to check the path furthermore ? Why does returning Compiler::has_valid_extension(...) is not enough ?

}
None
})
Expand All @@ -54,3 +72,136 @@ impl Compiler {
false
}
}
#[cfg(test)]
mod tests {

use std::path::PathBuf;

use crate::core::file_utils::file_utils::get_absolute_path;

use super::Compiler;

#[test]
fn test_collect_files_extension_one_extension() {
let files = vec![
PathBuf::from("main.c"),
PathBuf::from("queue.c"),
PathBuf::from("queue.h"),
PathBuf::from("queue.cpp"),
];
let expected = vec![
String::from(
get_absolute_path(&PathBuf::from("main.c"))
.unwrap()
.to_str()
.unwrap(),
),
Comment on lines +93 to +98
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to refactor these pattern here and below (9 times) to a single line ? Maybe by creating a function get_absolute_path_string("main.c") or another name, just for testing.

String::from(
get_absolute_path(&PathBuf::from("queue.c"))
.unwrap()
.to_str()
.unwrap(),
),
];

let collected = Compiler::collect_files_with_extension(&files, &["c"]);
assert_eq!(expected, collected);
}

#[test]
fn test_collect_files_extension_multiple_extensions() {
let files = vec![
PathBuf::from("main.c"),
PathBuf::from("queue.c"),
PathBuf::from("queue.h"),
PathBuf::from("queue.cpp"),
];
let expected = vec![
String::from(
get_absolute_path(&PathBuf::from("main.c"))
.unwrap()
.to_str()
.unwrap(),
),
String::from(
get_absolute_path(&PathBuf::from("queue.c"))
.unwrap()
.to_str()
.unwrap(),
),
String::from(
get_absolute_path(&PathBuf::from("queue.cpp"))
.unwrap()
.to_str()
.unwrap(),
),
];

let collected = Compiler::collect_files_with_extension(&files, &["c", "cpp"]);

println!("{:#?}", collected);
assert_eq!(expected, collected);
}

#[test]
fn test_gxx_args_c() {
let files = vec![
PathBuf::from("main.c"),
PathBuf::from("queue.c"),
PathBuf::from("queue.h"),
];
let output_path_string = String::from("target");

let expected = vec![
String::from(
get_absolute_path(&PathBuf::from("main.c"))
.unwrap()
.to_str()
.unwrap(),
),
String::from(
get_absolute_path(&PathBuf::from("queue.c"))
.unwrap()
.to_str()
.unwrap(),
),
String::from("-fdiagnostics-color=always"),
String::from("-o"),
output_path_string.clone(),
];

let collected = Compiler::gxx_args(&files, &PathBuf::from(output_path_string), &["c"]);
assert_eq!(expected, collected);
}
#[test]
fn test_gxx_args_cpp() {
let files = vec![
PathBuf::from("main.c"),
PathBuf::from("queue.cpp"),
PathBuf::from("queue.h"),
];
let output_path_string = String::from("target");

let expected = vec![
String::from(
get_absolute_path(&PathBuf::from("main.c"))
.unwrap()
.to_str()
.unwrap(),
),
String::from(
get_absolute_path(&PathBuf::from("queue.cpp"))
.unwrap()
.to_str()
.unwrap(),
),
String::from("-fdiagnostics-color=always"),
String::from("-o"),
output_path_string.clone(),
];

let collected =
Compiler::gxx_args(&files, &PathBuf::from(output_path_string), &["c", "cpp"]);
assert_eq!(expected, collected);
}
}
8 changes: 7 additions & 1 deletion src/core/file_utils/file_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,16 @@ pub fn list_dir_files(dir: &std::path::PathBuf) -> Result<Vec<std::path::PathBuf
.collect())
}

// From https://stackoverflow.com/a/38384901
/// Gets the absolute path of path
/// This function checks if the file exists
pub fn get_full_path(path: &std::path::PathBuf) -> Result<std::path::PathBuf, io::Error> {
// From https://stackoverflow.com/a/38384901
dunce::canonicalize(path)
}
/// Gets the absolute path of path without checking if the path actually exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea to avoid useless syscalls

pub fn get_absolute_path(path: &std::path::PathBuf) -> Result<std::path::PathBuf, io::Error> {
std::path::absolute(path)
}

pub fn current_folder() -> Result<std::path::PathBuf, io::Error> {
std::env::current_dir()
Expand Down