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

Jump to bottom

report and skip broken symlinks #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

report and skip broken symlinks #136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 22 additions & 5 deletions src/build/packages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,18 @@ fn matches_filter(filter: &Option<regex::Regex>, path: &str) -> bool {
}
}

fn is_broken_symlink<P: AsRef<Path>>(path: P) -> bool {
if let Ok(metadata) = fs::symlink_metadata(&path) {
if metadata.file_type().is_symlink() {
return match fs::read_link(&path) {
Ok(link_target) => fs::metadata(&link_target).is_err(),
Err(_) => true,
};
}
}
false
}

pub fn read_folders(
filter: &Option<regex::Regex>,
package_dir: &Path,
Expand All @@ -142,6 +154,10 @@ pub fn read_folders(

for entry in fs::read_dir(package_dir.join(&path_buf))? {
let entry_path_buf = entry.map(|entry| entry.path())?;
if is_broken_symlink(&entry_path_buf) {
println!("Warning: '{}' is a broken symlink", entry_path_buf.display());
continue;
}
Comment on lines +157 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea, but could this check be moved to if an actual error is produced? I assume it'd be in the line below, let metadata = fs::metadata(&entry_path_buf)?;.

Maybe it doesn't matter in practice, but sometimes these FS APIs incur latency, so it'd be nice to avoid using it unless actually needed, which I guess is only if it actually errors.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - +1 @adnelson -- I think push these to both 169, as well as 471. I think both those could potentially be symlinked folders. Other than that, great stuff.

It reminds me that I should probably pick up the PR to add proper log levels again at some point.

Copy link
Member

Choose a reason for hiding this comment

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

@adnelson -- We've added some better loggin structure, feel free to rebase and update the PR.

If you don't feel like it - I'd be happy to take this over and finish it, I think this could be nice to have :)

let metadata = fs::metadata(&entry_path_buf)?;
let name = entry_path_buf.file_name().unwrap().to_str().unwrap().to_string();

Expand All @@ -150,7 +166,7 @@ pub fn read_folders(
if metadata.file_type().is_dir() && recurse {
match read_folders(filter, package_dir, &new_path, recurse) {
Ok(s) => map.extend(s),
Err(e) => println!("Error reading directory: {}", e),
Err(e) => println!("Error reading directory {}: {}", entry_path_buf.display(), e),
}
}

Expand Down Expand Up @@ -452,13 +468,14 @@ pub fn get_source_files(
if type_ != &Some("dev".to_string()) {
match read_folders(filter, package_dir, path_dir, recurse) {
Ok(files) => map.extend(files),
Err(_e) if type_ == &Some("dev".to_string()) => {
Err(e) if type_ == &Some("dev".to_string()) => {
println!(
"Could not read folder: {}... Probably ok as type is dev",
path_dir.to_string_lossy()
"Could not read folder {}: {}... Probably ok as type is dev",
path_dir.to_string_lossy(),
e
)
}
Err(_e) => println!("Could not read folder: {}...", path_dir.to_string_lossy()),
Err(e) => println!("Could not read folder {}: {}", path_dir.to_string_lossy(), e),
}
}

Expand Down
Loading